Speed up PE thunk and float search #70
Conversation
|
|
||
| # TODO: Should check any section that has code, not just .text |
There was a problem hiding this comment.
Is this TODO still relevant? Do you maybe want to add a follow-up ticket? I realised that there are more instances of this TODO around
| @@ -22,6 +23,14 @@ | |||
|
|
|||
| # pylint: disable=too-many-lines | |||
|
|
|||
| # Match 6 byte absolute jump instructions. | |||
There was a problem hiding this comment.
Just a suggestion (since this would also have applied to the old code): Since the reader (like me) might not be too familiar with the machine code instructions, maybe it would make sense to add some more information here? Like only those types of jumps are affected by the reloc table.
|
|
||
| # Match floating point instructions. | ||
| float_re = re.compile( | ||
| rb"([\xd8\xd9\xdc\xdd])([\x05\x0d\x15\x1d\x25\x2d\x35\x3d])(.{4})", flags=re.S |
There was a problem hiding this comment.
Same here - I couldn't tell from looking at it why this list is correct or complete.
There was a problem hiding this comment.
You're right, this needs more information. I forget how I arrived at this list. Maybe just from reading the opcodes and cutting it down to the instructions that reference a pointer
| # We will check against possible float opcodes | ||
| raw = text.read_virtual(addr - 2, 6) | ||
| (opcode, opcode_ext, const_addr) = struct.unpack("<BBL", raw) | ||
| for opcode, _, const_addr_bytes in float_re.findall(text_raw): |
There was a problem hiding this comment.
Does the second part need a match group if we are not interested in it?
| if const_addr in seen_addrs: | ||
| continue | ||
|
|
||
| if opcode in (b"\xD8", b"\xD9"): |
There was a problem hiding this comment.
Casing inconsistent with regex above; I'd go with all upper or all lower case for hex codes
There was a problem hiding this comment.
That was a black adjustment in my stashed file, but now it doesn't seem to care if they're lower case.
| text_raw = bytes(text_sect.view) | ||
|
|
||
| # Find all 6 byte absolute jumps: FF 25 (addr) | ||
| for match in abs_jump_re.finditer(text_raw): |
There was a problem hiding this comment.
findall above, finditer here - we can probably use finditer above as well
| text_raw = bytes(text_sect.view) | ||
|
|
||
| # Find all 6 byte absolute jumps: FF 25 (addr) | ||
| for match in abs_jump_re.finditer(text_raw): |
There was a problem hiding this comment.
One potential issue (that doesn't happen for isle, but it could happen): finditer and findall look for non-overlapping matches. So there could be an instruction right before the jump that happens to have a FF 25 in its arguments, which then causes the subsequent jump to not be recognized. While taking a brief look I found this StackOverflow thread: https://stackoverflow.com/q/5616822/10666668
If you find a fix for that, it'd also be great to add a unit test. I am also not sure if the old version would handle that correctly or not.
|
Looking at the big picture, would it be better to move these two functions outside of |
|
Good point! We should separate the container formats (PE/ELF/MACHO/...) from the architectures (x86/x86_64/..) |
|
Haven't forgotten this. I'm going to move each of these functions to these to their own module which (I think) is a much better design. Since both of the interactions with the "compare core" and the database are just calling each of these functions, we could (eventually) move that code there too. The situation with the FPU instructions is more complicated than I had thought when I did the first version of this code. I think I landed on an okay subset of instructions but there are a few more we should get. |
When searching for thunks and floating point constants, we are looking for a specific kind of instruction. A binary regex makes sense and is much faster than the hacky approach we had before.
The float method would return duplicates where the same constant was used in multiple locations. That's now fixed.
I removed
populate_thunksand there is now justthunks, a@cached_property.