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

Open
#5,805 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
25/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Stale
Tech stack
cpp

Research direction

Start by reproducing the reported behavior with the attached vmquacks_combinator.zip sample, especially its main function, and inspect the Binary Ninja debug report for the identified loop region. Compare the generated while (true) structure with the requested do-while-style alternatives; done means the relevant control-flow cases are selected without adding unnecessary indentation.

Written by the indexing model from the issue text.

Description

Component: Core Core: HLIL Effort: Medium IL Optimization Impact: Low

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

Dominant language
C++
Stars
1.3k
Forks
298
Avg merge
5d 5h
Merged PRs (30d)
19

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from Vector35/binaryninja-api

All issues in Vector35/binaryninja-api

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.