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

[WIP] Show unicode strings in disasm #10555

Merged
merged 5 commits into from Jul 4, 2018
Merged

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented Jun 30, 2018

Shows flag realnames instead of names in disasm if scr.utf8 is enabled.
String flag realnames are now unfiltered version of flag names.

2018-06-30-150620_773x74_scrot

To test this:
download this binary http://xvilka.me/chinese and then:

r2 -A chinese
> e scr.utf8=1
> s 0x0040055a
> pd 1

This is a [WIP] pr because it doesn't affect only unicode languages, but also english binaries, in fact if utf8 is enabled instead of
str.A_string_with_spaces
we will have
str.A string with spaces

However, all commands will still need normal flag names. Please test this, and tell me if it creates confusion.

@cyanpencil cyanpencil force-pushed the cyanpencil:unicode-strings branch 5 times, most recently from a303cba to 9bce8cf Jun 30, 2018
@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 1, 2018

@cyanpencil cyanpencil requested review from XVilka and radare Jul 1, 2018
@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 2, 2018

@cyanpencil maybe wait until you do the character lengths, then merge - so far it looks very simple and good.

@radare
Copy link
Collaborator

@radare radare commented Jul 2, 2018

maybe if we do this we can kill the str. and use quotes around the string, and flags or refs can be followed by using Vr (toggle leahints instead of jmphints), so the option to make this happen doesnt needs to be enabled by default or break anything

@radare
Copy link
Collaborator

@radare radare commented Jul 2, 2018

dont reuse utf8 option for this, add another option pls, and make this option live inside RFlag, so you dont have to pass that option as argument all the time

@radare radare added this to the 2.8.0 milestone Jul 2, 2018
@radare
Copy link
Collaborator

@radare radare commented Jul 2, 2018

this new option can be named. asm.flags.real or

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 2, 2018

Good idea about using "string" instead of str.string, this makes more sense, agree.

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 2, 2018

@radare why 2.8.0? if we put it in 2.7.0 but disabled by default, it might be better for testing

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 2, 2018

Ok, I'll change the utf8 option and use "string" instead of str.string. Also will leave it Work In Progress until the character legth pr is not merged (I'll push it today, almost finished)

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 2, 2018

Done, now this should work without changing the r2r tests, since asm.flags.real is disabled by default.

@cyanpencil cyanpencil force-pushed the cyanpencil:unicode-strings branch from c22c6e1 to df1920d Jul 2, 2018
@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 2, 2018

There are some testcases broken like this:

-            ;-- str.Hello World:
+            ;-- "Hello World":
@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 4, 2018

Strange, it shows 29 broken tests.

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 4, 2018

Can you please rebase both r2r and this pull requests?

@radare
Copy link
Collaborator

@radare radare commented Jul 4, 2018

uhm, not sure if red because of the pr or master being red. we sohuld greenify travis first

@ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Jul 4, 2018

@cyanpencil what happens if there are control chars in the string? Newline, delete chars, etc.

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 4, 2018

@XVilka yes I made a mess in the last commit + with rebasing, am currently fixing

@ret2libc I quickly tested on a binary that has a string like this "A\x01\x02...\x1f" (which should be all control chars) and r2 correctly escaped them by showing "A\a\b\t\n...", so I suppose it works, but didn't test it much more

@cyanpencil cyanpencil force-pushed the cyanpencil:unicode-strings branch from 42cbb5d to e79a85e Jul 4, 2018
@cyanpencil cyanpencil force-pushed the cyanpencil:unicode-strings branch from e79a85e to 6a4f901 Jul 4, 2018
@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 4, 2018

Ok green again.
P.s: I also updated the r2r pr: https://github.com/radare/radare2-regressions/pull/1383

@XVilka XVilka merged commit b747592 into radareorg:master Jul 4, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 4, 2018

Merged. It will be easier to test now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants