Visual clue for non-continuous control flow in the linear view

Vector35 / binaryninja-api

Public API, examples, documentation and issues for Binary Ninja

https://binary.ninja/

MIT License

893 stars 200 forks source link

Visual clue for non-continuous control flow in the linear view #5771

Open seekbytes opened 1 month ago

seekbytes commented 1 month ago

I'm not aware of how binary ninja disassembles a chunk of bytes for x86, but it seems that for a buffer containing more than one instructions "plausible" binary ninja added them without choosing which one to use.

e8d54d0e00         call    @__security_check_cookie@4
8be5               mov     esp, ebp
5d                 pop     ebp
c3                 retn

but this can overlap with

008be55dc3e8       add     byte [ebx-0x173ca21b], cl  {0xe8c35de8}  {0xe8c35de8}
7156               jno     0x1000fa84

Unfortunately I can't share the binary but you should have all the elements needed to understand the root cause. If not please, let me know

xusheng6 commented 1 month ago

Thanks for the bug report. However, the actual situation is different from what you guessed. We support overlapping instructions and we believe this is how binary ninja is superior than many of our competitors.

The dissembler is NOT processing these instructions blindly. The overlapped instructions get defined for a reason, e.g., there is a jump/call into it, etc.

In this case, this is probably not the best outcome though I do not see why it is happening. Could you please show the instructions in a broader context., e.g., a function, to allow us to better understand the situation?

seekbytes commented 1 month ago

Thanks for the bug report. However, the actual situation is different from what you guessed. We support overlapping instructions and we believe this is how binary ninja is superior than many of our competitors.

Sorry, I thought for a reason bninja didn't support it. One thing that might help in the UX is to differentiate the overlapped instructions from the one that aren't "overlapped" because otherwise I am really confused on what it's tracked in the next levels of IL (and how do you merge information from the overlapped information from disassembly?)

xusheng6 commented 1 month ago

I just saw the screenshot you provided. Could you please select the line at 0x1000fa26 (the wrong instruction) and press space. Then, in the graph view, see whether there is any control flow reaching that address? If so, that is the reason why we decide to dissemble that instruction

xusheng6 commented 1 month ago

Thanks for the bug report. However, the actual situation is different from what you guessed. We support overlapping instructions and we believe this is how binary ninja is superior than many of our competitors.

Sorry, I thought for a reason bninja didn't support it. One thing that might help in the UX is to differentiate the overlapped instructions from the one that aren't "overlapped" because otherwise I am really confused on what it's tracked in the next levels of IL (and how do you merge information from the overlapped information from disassembly?)

We do not merge the info -- they are treated as different instructions. There is also no reason to do so

seekbytes commented 1 month ago

I just saw the screenshot you provided. Could you please select the line at 0x1000fa26 (the wrong instruction) and press space. Then, in the graph view, see whether there is any control flow reaching that address? If so, that is the reason why we decide to dissemble that instruction

yes there's definitely some conditional jump instructions that refer to it 0x1000fa26, so I guess it's correct behavior then, maybe I would add something 'graphical' only to make those different from the others instructions. Sorry for the stupid considerations.

xusheng6 commented 1 month ago

I just saw the screenshot you provided. Could you please select the line at 0x1000fa26 (the wrong instruction) and press space. Then, in the graph view, see whether there is any control flow reaching that address? If so, that is the reason why we decide to dissemble that instruction

yes there's definitely some conditional jump instructions that refer to it 0x1000fa26, so I guess it's correct behavior then, maybe I would add something 'graphical' only to make those different from the others instructions. Sorry for the stupid considerations.

No worries, you are all good. Thanks again for bringing this up. We will see if we want to make some visual distinction to make it more obvious

xusheng6 commented 1 month ago

We are very unlikely to implement this on its own, and I believe this will be addressed when #965 is done