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

Improve realname support for symbols #15702

Merged
merged 50 commits into from
Jan 12, 2020
Merged

Conversation

ITAYC0HEN
Copy link
Contributor

@ITAYC0HEN ITAYC0HEN commented Dec 24, 2019

These changes aim to modify the way symbols are structured and look on the disassembly and across different places. More specifically, when asm.flags.real is set to true.

For example, this PR is aimed to change this:

call  dword   [sym.imp.user32.dll_CreateWindowExA]

To this:

call  dword  [CreateWindowExA]

In addition, it also adds the "libname" to each import. In general, it improves consistency across symbols in r2.

@github-actions github-actions bot added the RBin label Dec 24, 2019
@ITAYC0HEN
Copy link
Contributor Author

Current status

Disassembly:
Full flag looks like this: imp.<libname>.<funcname>
Realflag looks like this: <funcname>

image

Imports (iij)
"name" reduced to the function name. "libname" member was added to describe the library. This will later be used to differentiate between imports of the same name from different libraries.

image

Flags
The flag shows the full symbol name with library and symbol type. The real name is set only to the function name
image

@thestr4ng3r thestr4ng3r self-assigned this Dec 25, 2019
@XVilka
Copy link
Contributor

XVilka commented Dec 27, 2019

Could you please also add a few tests?

@XVilka XVilka added this to the 4.2.0 (Arctic World Archive) milestone Dec 27, 2019
@radare radare changed the title [WIP] Improve realname support for symbols WIP: Improve realname support for symbols Dec 29, 2019
@ITAYC0HEN
Copy link
Contributor Author

What do you think of this direction? before we start the work on all plugins

@@ -227,7 +228,8 @@ static RList* symbols(RBinFile *bf) {
break;
}
//strncpy (ptr->name, (char*)symbols[i].name, R_BIN_SIZEOF_STRINGS);
ptr->name = r_str_newf ("imp.%s", imports[i].name);
ptr->name = r_str_newf ("%s", imports[i].name);
Copy link
Contributor

Choose a reason for hiding this comment

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

strdup?

@ret2libc
Copy link
Contributor

ret2libc commented Jan 2, 2020

I’m very in favor of this change! I like it

@ITAYC0HEN
Copy link
Contributor Author

ITAYC0HEN commented Jan 2, 2020

If the user will see call LoadLibraryA we need to find a way to let users know how to navigate to this because s LoadLibraryA won't work. What do you think?

A suggested seek mechanism can be found below:

If only one flag contains this substring (e.g. only imp.kernel32_dll.LoadLibraryA will contain LoadLibraryA), then the seek will be to this address.

If there are more then one option, then r2 will show:

Multiple flags matched 'LoadLibraryA'. Did you mean to one of the following flags that contain this substring:
0x4001234    imp.kernel32_dll.LoadLibraryA
0x7e01133    imp.someOther_dll.LoadLibraryA

@radare
Copy link
Collaborator

radare commented Jan 3, 2020

iij should contain flagname

@radare
Copy link
Collaborator

radare commented Jan 3, 2020

pls rebase

@radare
Copy link
Collaborator

radare commented Jan 3, 2020 via email

@radare
Copy link
Collaborator

radare commented Jan 7, 2020

some of the failing tests.. this doesnt looks good to me, but the idea to have an option to shorten those symbol names lgtm, about navigation, just use hotkeys

Screenshot 2020-01-07 at 23 16 45

Screenshot 2020-01-07 at 23 16 53

Screenshot 2020-01-07 at 23 17 06

@@ -1704,12 +1709,12 @@ static int bin_relocs(RCore *r, int mode, int va) {
}

#define MYDB 1
/* this is a hacky workaround that needs proper refactoring in Rbin to use Sdb */
/* this is a VERY VERY VERY hacky and bad workaround that needs proper refactoring in Rbin to use Sdb */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lollll

@ITAYC0HEN ITAYC0HEN marked this pull request as ready for review January 9, 2020 06:56
@thestr4ng3r
Copy link
Contributor

There is a slight difference in flagnames with demangling currently:
master:

name (bin.demangle=false): sym.imp.MSVCP100.dll___Osfx___basic_ostream_DU__char_traits_D_std___std__QAEXXZ
name (bin.demangle=true): sym.public:_void___thiscall_std::basic_ostream_char__struct_std::char_traits_char__::_Osfx_void

this pr:

name (bin.demangle=false): sym.imp.MSVCP100.dll___Osfx___basic_ostream_DU__char_traits_D_std___std__QAEXXZ
name (bin.demangle=true): sym.imp.MSVCP100.dll_public:_void___thiscall_std::basic_ostream_char__struct_std::char_traits_char__::_Osfx_void

Note that the imp.MSVCP100.dll_ part is missing on master. I think the new behavior makes much more sense since demangling should have no influence on what prefixes there are. This is important to understand regarding the test updates.

@ITAYC0HEN
Copy link
Contributor Author

Note that the imp.MSVCP100.dll_ part is missing on master. I think the new behavior makes much more sense since demangling should have no influence on what prefixes there are. This is important to understand regarding the test updates.

I completely agree :)

@XVilka
Copy link
Contributor

XVilka commented Jan 10, 2020

Please rebase on top of the currently green master, thanks!

@ITAYC0HEN ITAYC0HEN changed the title WIP: Improve realname support for symbols Improve realname support for symbols Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants