a couple of points

[deepzero]

Hey,

 

I spent some time reading through the source. I am not a huge fan of it, but i guess we'll have to work with what we have.

 

I forked the project, because i havnt used bitbucket before and didnt want to break anything. A pull request has been issued to exodia, the first batch of fixes should be trivial. I have also created a couple of "issues" on the original repo.

 

Once the pull request has been merged, i'll continue to shoot bugfixes into the bug fix branch, which can then be merged to master, after a second pair of eyes singed off on it.

 

 

 

Several points:

 

- NULL should only be used for pointers, we should use 0 for normal int variables.

- pointer should always be initialized to zero, when declared.

- are we coding in C or C++ ? If c++, we should scope our variables better.

- is this a MFC project? If not, the resource file breaks cimpilation on non-mfc compilers, something i plan to fix shortly.

- winapi BOOL returns should not be compared to the TRUE constant. Fixes are in my pull request.

- one should not rely on winapis initializing data. fixes in my pull requests.

- there are a couple of expressions, that are contradictory, ie always true or false. I have fixes for some, reported some more in the issues tab and there are yet more.

- i suggest we create our own IsStrEqualA() and ISStrEqualW() functions, and use that instead of the different lstrcm* != 0 functions

- is there any branch management? Right now it looks like everything is going straight to master.

 

Currently there is massive code duplication, because the Ascii/widechar version have their own body, which is exactly the same. We should implement widechar only and convert ascii input to wchar strings, then call the widechar version. Not sure why there is code duplication rihgt now.

 

And now the mega-badboy:

 

RtlMoveMemory is much slower than RtlCopyMemory or memcpy and should only be used in special cases. Here it is used **even for normal variable assignment**. Ie, RtlMovememory is used to change pointer or integer variables. This is **bad**.

 

 

Furthermore, when that is done, the size seems arbitrarily hardocded to either 4 or 8, which creates a buffer over or underflow with the respective other platform. if anything, it should be sizeof(void*).

 

I suggest we slowly replace RtlMovemem/RtlCopyMem by memcpy() and memmove(). These are better anyways and we see which calls still need to be changed.

 

This needs to be addressed. Many more bugs to fix.

 

edit: similarly:

 

RtlZeroMemory(FileMapVA, sizeof ULONG_PTR);

we should either pass a properly typed pointer, or not touch this at all, we risk subtle memory corruptions here.

 

 

 

p.s. where are the test cases?

Edited January 15, 2014 by deepzero

[cypher]

Thank you very much for the PR and those many fixes !

 

Unfortunately, there are no unit tests. So far, we tested our fixes/enhacements with private unpacker/debugger codes as we basically only changed what we were needing at a specific moment. mr.exodia tested much with his x64 dbg during development.

However the SDK examples by reversingLabs cover alot functions and could probably be turned into unit-tests easily.

 

There currently isnt any branch management. Basically we were always working on master. My Scylla-integration was the first time branching But as we grow in number of developers, we definitely need more management. I suggest using git-flow !

 

Misc:

- I dont know why ReversingLabs uses Rtl* all over the place...

- not MFC afaik. Tho there are some helper functions that can auto-create some GUI things for rapid Unpacker-Development. Unsure if they rely on MFC

- TE is compiled as C++ project. We didnt change that.

 

As a general "rule" I think everyone should change and fix by their best knowledge and test it before pushing.

As there is no QA-control here, everyone should not hesitate to point out what others could improve in their code.

Edited January 15, 2014 by cypher