Skip to content
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

Annotation for constant and global variables #124

Merged
merged 15 commits into from Jul 26, 2020
Merged

Annotation for constant and global variables #124

merged 15 commits into from Jul 26, 2020

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Jul 14, 2020

Detailed description
This PR implements annotations for constant variables and global variables. The logic used to determine if a variable is global or constant is taken from the following existing code in variable.cc. https://github.com/thestr4ng3r/ghidra/blob/6c10f36f06468f866188cccf960c019779fb9028/Ghidra/Features/Decompiler/src/decompile/cpp/variable.cc#L452-L470
As discussed in the Telegram channel, I'm making constant variable annotation only when address space is of name "ram". However, I couldn't find such a constant variable that satisfies this condition in the examples I have tried. I am not sure if such a constant variable would ever exist. Maybe all we want is global variables for the action to add/delete/rename flags in Cutter.

...

Test plan

  • 1. Fetch and compile PR #17281 from radare2 and compile it before compiling this PR.

  • 2. Test if all global and constant variables are identified as expected by the AnnotateVariable function.

  • You can use the output of pdgj for testing if the variables and offsets are annotated correctly. The binary used in the new tests made added in this PR can be used for testing.

  • 3. Check code and make sure my logic for determining if a variable is global/constant is correct.

...

Closing issues

...

Copy link
Member

@karliss karliss left a comment

Please make a test. You can probably do it using JSON printing command.

src/CodeXMLParse.cpp Outdated Show resolved Hide resolved
src/CodeXMLParse.cpp Outdated Show resolved Hide resolved
@NirmalManoj NirmalManoj requested a review from karliss Jul 20, 2020
@karliss
Copy link
Member

karliss commented Jul 20, 2020

How about running the json output in the test through ~{} so that it gets pretty printed? It will be easier to inspect it that way and see the changes.

@karliss
Copy link
Member

karliss commented Jul 20, 2020

As for the Jessie test failure take a look at output log. It is complaining about missing names, most like it isn't configured to use correct r2 version.

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jul 20, 2020

I have modified the test to have the output for pdgj~{}. About Jessie, I think it's failing when it compiles Ghidra. I don't want it to be a blocker for this PR.

NAME=global variable annotation
FILE=bins/dectest32
EXPECT=<<EOF
{"code":"\nundefined4 sym.get_global_array_entry(void)\n{\n return *(undefined4 *)0x804c034;\n}\n","annotations":[{"start":1,"end":11,"type":"syntax_highlight","syntax_highlight":"datatype"},{"start":12,"end":38,"type":"function_name","name":"sym.get_global_array_entry","offset":134517184},{"start":12,"end":38,"type":"offset","offset":134517184},{"start":12,"end":38,"type":"syntax_highlight","syntax_highlight":"function_name"},{"start":39,"end":43,"type":"syntax_highlight","syntax_highlight":"keyword"},{"start":51,"end":57,"type":"offset","offset":134517193},{"start":51,"end":57,"type":"syntax_highlight","syntax_highlight":"keyword"},{"start":58,"end":59,"type":"offset","offset":134517187},{"start":60,"end":70,"type":"syntax_highlight","syntax_highlight":"datatype"},{"start":73,"end":82,"type":"global_variable","offset":134529076},{"start":73,"end":82,"type":"syntax_highlight","syntax_highlight":"constant_variable"},{"start":51,"end":82,"type":"offset","offset":134517193}]}
Copy link
Member

@karliss karliss Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the code you have two different kind of references, but test has only one of them.

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I noted in the PR description, I couldn't find a constant variable that satisfies all conditions. Please tell me how to find such a constant variable or give me a binary with such a constant variable. Current code checks if something is a global variable in an if condition and checks if it's constant in the next else if. I can change this order if it's required, but still, I couldn't find a constant that's in the address space "ram".

Copy link
Member

@karliss karliss Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't guess if you don't have an example of such variable then there is no point writing code for handling it since you don't actually know how it looks in xml or ghidra structures and how to correctly handle it.

src/CodeXMLParse.cpp Show resolved Hide resolved
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jul 21, 2020

XML files used in the comment
Case 1: first.tar.gz
Case 2: second.tar.gz
Case 3: third.tar.gz

NirmalManoj added 2 commits Jul 22, 2020
separate functions to annotate global and constant variables
@NirmalManoj NirmalManoj requested a review from karliss Jul 22, 2020
Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

Looks good! Thank you :)

@karliss are you satisfy with the changes for your requests?

@karliss
Copy link
Member

karliss commented Jul 26, 2020

Jessie test scripts need to be fixed to use appropriate r2 version.

@NirmalManoj NirmalManoj merged commit 841a458 into rizinorg:decompiler-refactoring Jul 26, 2020
3 checks passed
@ITAYC0HEN
Copy link
Member

ITAYC0HEN commented Jul 26, 2020

Congratulations!

@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Jul 29, 2020
thestr4ng3r pushed a commit that referenced this pull request Aug 11, 2020
* annotation function for constant and global variables
* dictionary that maps varref to Varnode
thestr4ng3r pushed a commit that referenced this pull request Aug 11, 2020
* annotation function for constant and global variables
* dictionary that maps varref to Varnode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants