Vector35 / binaryninja-api

Public API, examples, documentation and issues for Binary Ninja
https://binary.ninja/
MIT License
899 stars 203 forks source link

Bad `while (true)` loop recognition leads to excessive indentation level #5805

Open JANlittle opened 1 month ago

JANlittle commented 1 month ago

What is the feature you'd like to have? When identifying the loop region, consider reverse the condition to choose anorther node when the node is too large or the max indentation level inside the node is too high; When the node is small or the max indentation level inside the node is low, embed the loop region. This helps to convert certain while (true) loop into do-while loop, reduce indentation levels, and improve readability.

Is your feature request related to a problem? It seems that BN is too inclined to generate while loops. I found that in the following situation, BN(Personal 4.1.5747-Stable) generates complex while (true) loops, while IDA generates do-while loops, significantly reducing complexity: 图片 图片 In BN debug report, the above area will be identified as the following loop region: 图片 图片 Eventually, a frustrating while (true) loop was generated, causing the majority of the code following the function to have an extra indentation level😢: 图片 In my personal opinion, when identifying the loop body, if the conditional node(as shown in the blue border, the default branch is true) points to a node that is too large, the condition should be reversed or the branch should be switched into false to select other smaller node. If the node pointed to by the branch is small, it can be considered to include it in the loop body instead of using the default break statement. At the same time, adding more diverse loop structure pattern matching should be able to correctly transform this situation.

Are any alternative solutions acceptable? Does BN seem to not support user intervention in its control flow structuring process?🥲

Additional Information: Here is the sample(also a CTF chall lol), which main funcstion contains this situation. vmquacks_combinator.zip

psifertex commented 1 month ago

Thanks for the report, linking to dogbolt version of that binary:

https://dogbolt.org/?id=d280dc2c-d651-4a12-8768-1aa8141f5d98#BinaryNinja=1493&Ghidra=1452&Hex-Rays=913