-
-
Notifications
You must be signed in to change notification settings - Fork 432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reworked absolute address handling (Fixes #471) #473
Conversation
In the latest set of changes I've finally dealt with the source of all address-related problems. It's this piece of code from if (dec_op->encoding == ZYDIS_OPERAND_ENCODING_DISP16_32_64)
{
ZydisCalcAbsoluteAddress(instruction, dec_op, 0,
(ZyanU64 *)&enc_op->mem.displacement);
}
else
{
enc_op->mem.displacement = dec_op->mem.disp.has_displacement ?
dec_op->mem.disp.value : 0;
} It opened the gates of hell because it introduces special treatment for absolute addresses coming from Now things are done The Right Way ™️. All displacements are the same and they are handled just like in other parts of Zydis (decoder and utils like This took way more time and effort than I've originally anticipated for something that initially seemed like a minor oversight but I think the changes are worth it. New behaviors are truly uniform and shield the user from all the ugly quirks of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This looks way better from a usability perspective 🙂 Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a coarse glance! Apologies for the delay: I didn't find the time to review this properly. Realistically you know this piece of code better than both Florian and me anyway, so let's not delay this any further. :)
This PR fixes all known issues around treating signed displacements as absolute addresses. Address size hint is now used for disambiguation where needed (rare cases that essentially force address override prefix). This also lead to a simplification of the fuzzer as verification of memory operands was always quite messy due to multiple issues that it had to deal with (signedness, 16-bit truncation, MIB, etc.). Now it's finally replaced with simpler and more robust code.
Fixes #471