Fix null pointer dereference in getName/getString #127
Conversation
Whenever getString or getName are called with an act such that act->p.String is a NULL pointer, a NULL pointer dereference might happen (strlen(act->p.string) is called). In this commit we add checks at the beginning of the PUSH_STRING block so that a warning is displayed and an empty string is returned in this case. This patch fixes libming#121.
|
I have just added another commit addressing #116. |
|
snprintf returns number of characters that would have been printed
out _excluding_ the terminating NULL so I think you'll want to
increment the return of it by 1 before assigning to "needed_length"
|
getString is allocating a 4-bytes buffer to store an 'R' and an 8-bit number. t=malloc(4); /* Rdd */ sprintf(t,"R%d", act->p.RegisterNumber ); return t; Since up to three digits can be required to store the 8-bit number, the buffer has to be 5 bytes long. In this commit we also fix the PUSH_DOUBLE case by dynamically computing the required buffer size. This commit fixes libming#116 (CVE-2018-7867).
|
Thanks Sandro for the feedback. I have pushed an updated patch which also handles the case where |
|
Oh well, wait, looks like something is still going wrong. I'm going to push an updated version of this PR soon. |
Until now, the element duplication in pushdup was performed via t->val = Stack->val. While this is perfectly fine for integer/double/register values, this may create nasty, hard to debug issues with Strings. In fact, when called with a String at the top of the stack, pushdup would only push *a reference* to the same String element (shallow copy), later allowing to modify several stack elements at once, which may potentially lead to NULL pointer dereferences or any other unspecified impact. In this patch we implement deep copy in pushdup: * If the type of the stack element is 's' (for String), we allocate a new buffer and copy the String into it. * Otherwise we simply proceed as before, that is we do t->val = Stack->val which is perfectly fine since we are not dealing with pointers. This patch is the last part of the patch for libming#121 (fixes libming#121), which should now be completely fixed.
|
It should be fine now. There is a fairly high amount of changes, so even if I already tested everything, a careful review would be nice. Thanks ! |
|
Any chance you could look at re-introducing the now-broken
"make gen" rule under test/ directory ?
The rule is still present in test/Makefile.inc but that Makefile
is not included by test/Makefile.am. The "make gen" rule is
referenced by test/README and Makefile.inc itself has some
interesting comments/documentation.
I'm thinking it could be useful to test all the changes you
are doing in the decompiler.
|
|
While I indeed think writing unit tests would be a very important step towards more stability in libming, this is most likely not something that I can do as part of my Debian LTS duties, and I don't have much spare time currently... Is there any bug report on this build system issue ? If I can get a precise description of the problem, I'll maybe check if can do something. |
|
But anyways, this is not something I'd do in this PR. |
|
Ok thanks, tests postponed |
|
Thanks ! |
Whenever
getStringorgetNameare called with an act such thatact->p.Stringis a NULL pointer, a NULL pointer dereference might happen (strlen(act->p.string)is called).In this commit we add checks at the beginning of the
PUSH_STRINGblock so that a warning is displayed and an empty string is returned in this case.This PR (partially) fixes #121. In fact I am still working on another patch which would involve patching
pushdupto do deep copies instead of shallow copies, but before I will have to analyze the specification in order to be able to clearly state which interpretation is the right one.