Refactor of elf.c and remove reference to section name in get_import_addr #16530

ghost · 2020-04-11T15:45:56Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description
~~2 tests fail.~~
symbols with no sections header information 3: i believe the test is wrong, before the refacto the return addr of get_import_addr was always -1 because there was no section name.

ELF: imports partial: i believe the return value (get_import_addr_x86_manual) when to .plt.got entry can't be found should be -1 not a garbage value.

PS: if the return value of get_import_addr_x86_manual is the variable plt_sym_addr another test fail.

Test plan
Run tests

Closing issues
#12732

Instead of using a full matrix, just add the builds we care about

ret2libc

Please remove all the style-only changes from this PR, then I'll review. Anyway, the results about symbols with no sections header information 3: look better in your PR than in master, so that's already an improvement :)

libr/bin/format/elf/elf.h

libr/bin/p/bin_elf.inc

libr/bin/format/elf/elf.c

ret2libc · 2020-04-14T08:28:56Z

Ah, @08a please rebase on top of current master as well.

ghost · 2020-04-14T09:56:40Z

What do you think of ELF: imports partial test?

…LFOBJ

ghost · 2020-04-14T10:01:56Z

Should i rewrite symbols with no sections header information 3?

ret2libc · 2020-04-14T10:23:34Z

Should i rewrite symbols with no sections header information 3?

Yes, you should fix the test. Your results seem good.

What do you think of ELF: imports partial test?

I see these changes:

--- /tmp/old    2020-04-14 12:15:46.398818567 +0200
+++ /tmp/new    2020-04-14 12:15:13.556235229 +0200
@@ -17,7 +17,7 @@
 15  0x00051fc8 GLOBAL FUNC       unlink
 16  0x00051fd0 GLOBAL FUNC       strncpy
 17  0x00051fd8 GLOBAL FUNC       strncmp
-18  0x00443a32 WEAK   NOTYPE     _ITM_deregisterTMCloneTable
+18  0x00000000 WEAK   NOTYPE     _ITM_deregisterTMCloneTable
 19  0x00051fe0 GLOBAL FUNC       _exit
 20  0x00051fe8 GLOBAL FUNC       strcpy
 21  0x00051ff0 GLOBAL FUNC       mkdir
@@ -85,7 +85,7 @@
 83  0x000521e0 GLOBAL FUNC       ftello64
 84  0x000521e8 GLOBAL FUNC       acos
 85  0x000521f0 GLOBAL FUNC       read
-86  0x00443a32 GLOBAL FUNC       __libc_start_main
+86  0x00000000 GLOBAL FUNC       __libc_start_main
 87  0x000521f8 GLOBAL FUNC       srand
 88  0x00052200 GLOBAL FUNC       memcmp
 89  0x00052208 GLOBAL FUNC       fgets
@@ -106,7 +106,7 @@
 104 0x00052280 GLOBAL FUNC       sigemptyset
 105 0x00052288 GLOBAL FUNC       ftell
 106 0x00052290 GLOBAL FUNC       feof
-107 0x00443a32 WEAK   NOTYPE     __gmon_start__
+107 0x00000000 WEAK   NOTYPE     __gmon_start__
 108 0x00052298 GLOBAL FUNC       umask
 109 0x000522a0 GLOBAL FUNC       fopen64
 110 0x000522a8 GLOBAL FUNC       strtol
@@ -168,7 +168,7 @@
 166 0x00052468 GLOBAL FUNC       perror
 167 0x00052470 GLOBAL FUNC       strtok
 168 0x00052478 GLOBAL FUNC       sysconf
-169 0x00443a32 WEAK   NOTYPE     _Jv_RegisterClasses
+169 0x00000000 WEAK   NOTYPE     _Jv_RegisterClasses
 170 0x00052480 GLOBAL FUNC       towlower
 171 0x00052488 GLOBAL FUNC       rename
 172 0x00052490 GLOBAL FUNC       tgetstr
@@ -190,7 +190,7 @@
 188 0x00052510 GLOBAL FUNC       fwrite
 189 0x00052518 GLOBAL FUNC       lseek64
 190 0x00052520 GLOBAL FUNC       __fprintf_chk
-191 0x00443a32 WEAK   NOTYPE     _ITM_registerTMCloneTable
+191 0x00000000 WEAK   NOTYPE     _ITM_registerTMCloneTable
 192 0x00052528 GLOBAL FUNC       sqrt
 193 0x00052530 GLOBAL FUNC       sigaltstack
 194 0x00052538 GLOBAL FUNC       strerror

I think your changes are still good even in this case. The old values may interpreted as real addresses, but they are not. They are just useless values, so I think it's better to have 0s. IDA creates a fake section for those entries, but I think 0 for now is good. Does that sound good?

ghost · 2020-04-14T10:24:49Z

I think your changes are still good even in this case. The old values may interpreted as real addresses, but they are not. They are just useless values, so I think it's better to have 0s. IDA creates a fake section for those entries, but I think 0 for now is good. Does that sound good?

perfect i will fix that

ghost · 2020-04-20T11:46:20Z

Okay, thanks for your feedback.

For the position, i choose to not include this information inside the struct, because i didn't find any documentation which explain that the position inside the relocation table is the same that the position inside the got. So we need to recompute every time the position (the cost is not really expensive).

I understand that the majority of the code can be confusing so i will try to explain my choices.

get_import_addr_arm

In arm elf, the plt_addr is stored inside the got_entry, so i can read a random entry and get the based addr of the plt section.
The position is compute with this formule:
(relocation_rva - got_base_addr - nb_empty_entry * size_got_entry) / size_got_entry
Indeed the entry number 1, 2, 3 are not relevant.

get_import_addr_mips

In mips the got addr is stored inside a special dynamic entry anmed DT_MIPS_PLTGOT (when there is some relocation)
The position computation is like the arm one (expect that there is only 2 irrelevant entry).

get_import_addr_riscv

Riscv is like arm (get_got_entry).

get_import_sparc

We only support R_SPARC_JMP_SLOT. If the relocation is supported the entry inside the got return almost the exact position in the plt of the import_addr we only need some small adjustment (-size of the instruction to jump). It is cleaner than use some position.

get_import_ppc

Almost the same, pos is from the base addr of the plt (since the rva has a ref inside the plt).

get_import_x86

less modified, the code is self-explanatory.

Conclusion

As you can see i tried to be more logic. I understand that your are scared that the new code is broken.
The main problem is not that we have a new code but that you don't trust the test suite. Maybe it is a small sign that the test suite is not exhaustive. I personally don't have all the competence to add tests but if you know someone that is willing to do, let me know.

I will try to reduce again the diff size.

ghost · 2020-04-20T12:02:56Z

There is to much dependencies between to many function, it is why the size a the diff is so big.
I don't know how to make it smaller.

ghost · 2020-04-20T12:04:18Z

I believe i revert all the esthetic change, now all we have is algorithm modification. imho algorihm that should have been changed long time ago.

ret2libc

For the position, i choose to not include this information inside the struct, because i didn't find any documentation which explain that the position inside the relocation table is the same that the position inside the got. So we need to recompute every time the position (the cost is not really expensive).

Ok.

I understand that the majority of the code can be confusing so i will try to explain my choices.

Thanks a lot! I really appreciate the explanation!

I understand that your are scared that the new code is broken. The main problem is not that we have a new code but that you don't trust the test suite. Maybe it is a small sign that the test suite is not exhaustive

Of course I don't trust the test suite :) It's hard to cover everything. I think, for example, we don't have any MIPS bin with relocations.

I personally don't have all the competence to add tests but if you know someone that is willing to do, let me know.

Yeah, me neither. That's why I try to touch things I need and never "too much".

Anyway, overall the PR looks good to me, I just added some small comments here and there. Please try to address them, then we are almost ready to merge I think. Thanks again for your hard work!

libr/bin/format/elf/elf.c

ghost · 2020-04-21T09:29:15Z

Of course I don't trust the test suite :) It's hard to cover everything. I think, for example, we don't have any MIPS bin with relocations.

I found one bin with relocation.

readelf --use-dynamic -r test/bins/elf/analysis/mipsbe-ubusd

ret2libc · 2020-04-21T09:34:26Z

Of course I don't trust the test suite :) It's hard to cover everything. I think, for example, we don't have any MIPS bin with relocations.

I found one bin with relocation.
readelf --use-dynamic -r test/bins/elf/analysis/mipsbe-ubusd

Nice catch! I did not find it because I was using rabin2 :) And rabin2, both on master and in your PR, does not find them :( Anyway, as that's not a regression I don't consider it blocking for this PR

lgtm-com · 2020-04-21T11:16:47Z

This pull request introduces 2 alerts when merging 06cd49b into 1b56d63 - view on LGTM.com

new alerts:

2 for Comparison result is always the same

ghost · 2020-04-21T11:20:33Z

* 2 for Comparison result is always the same

I don't see why lgtm isn't happy? Each comparison are valid.

ret2libc · 2020-04-21T11:23:59Z

* 2 for Comparison result is always the same
I don't see why lgtm isn't happy? Each comparison are valid.

Oh, tricky one :D

ut64 base = r_buf_read_ble32_at (bin->b, p_plt_addr, bin->endian);
if (base == UT64_MAX) {

r_buf_read_ble32_at returns a ut32 value, not a ut64 one. And in case there are errors, it returns UT32_MAX, not UT64_MAX. So LGTM is correctly complaining about this.

ut64 addr = BREADWORD (bin->b, p_sym_got_addr);
return (!addr || addr == UT64_MAX) ? UT64_MAX : addr;

When R_BIN_ELF64 is not defined, BREADWORD will use the r_buf_read_ble32_at, which, again, returns UT32_MAX in case of error and not UT64_MAX.

ghost · 2020-04-21T12:53:11Z

@ret2libc It is fixed.

radare

lgtm

ret2libc · 2020-04-21T13:26:58Z

I will review this later and merge if ok.

ret2libc

LGTM! Thanks a lot for the great work, impressive!

PR: radareorg#16530

ret2libc and others added 19 commits April 7, 2020 09:44

Add GitHub actions for push to master as well

f39f8bd

Instead of using a full matrix, just add the builds we care about

--wip--: small refacto

e7e542e

--wip--

6225149

Change struct layout

d17349f

struct dynamic_info inside the ELFOBJ

a1a0768

--wip--

1bf49ac

/usr/bin/ls working

0cda939

Use the rva need to solve when blanck

2c307a9

Fix various bug the only feature is find in the plt when bind now

7379742

--wip--: ppc

b3390b1

Fix power pc

a810934

Fix arm

ab1773d

No pre incrementation in a for loop

19ef507

--wip--: mips

197193f

Fix mips

60bf888

removed until the test is validated

971ef33

Fix riscv

33405b2

Fix .plt.sec

7f4f293

--wip--: relro

4f5e78f

github-actions bot added the RBin label Apr 11, 2020

radare added this to the 4.5.0 milestone Apr 11, 2020

ret2libc self-assigned this Apr 14, 2020

ret2libc suggested changes Apr 14, 2020

View reviewed changes

libr/bin/format/elf/elf.h Outdated Show resolved Hide resolved

libr/bin/p/bin_elf.inc Outdated Show resolved Hide resolved

libr/bin/format/elf/elf.c Outdated Show resolved Hide resolved

Fix pre-commit hook error and merge RBinElfSymbolInfo struct inside E…

f1677f7

…LFOBJ

Rebase from master upstream

729143c

Remove hardcoded value get_import_ppc

a08dfce

Alexis Ehret added 4 commits April 20, 2020 13:50

Remove useless change

f571d44

Change position ptr symbol

92a15dd

1 line get_next_not_analyzed_offset return

41a239e

Remove one useless var

17ef21a

ret2libc suggested changes Apr 21, 2020

View reviewed changes

Alexis Ehret added 2 commits April 21, 2020 11:31

Fix return statement line 2726

11f58d5

define offset outside the loop

a16888e

Alexis Ehret added 3 commits April 21, 2020 12:24

Add BREADWORD and use it

f953253

Add COMPUTE_PLTGOT_POSITION macro

e5785b6

Add definition macro for offset and size

06cd49b

ghost requested a review from ret2libc April 21, 2020 11:15

Fix lgtm issues

8a5efc8

radare approved these changes Apr 21, 2020

View reviewed changes

ret2libc marked this pull request as ready for review April 21, 2020 13:26

ret2libc changed the title ~~WIP - Refacto of elf.c and remove reference to section name in get_import_addr~~ Refactor of elf.c and remove reference to section name in get_import_addr Apr 21, 2020

ret2libc approved these changes Apr 22, 2020

View reviewed changes

ret2libc merged commit 6124df4 into radareorg:master Apr 22, 2020

ghost deleted the refacto_elf.c branch April 22, 2020 14:13

Emi1305 pushed a commit to Emi1305/radare2 that referenced this pull request Jul 12, 2020

Use DYNAMIC info in get_import_addr instead of just section names ##ELF

5dca68e

PR: radareorg#16530

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of elf.c and remove reference to section name in get_import_addr #16530

Refactor of elf.c and remove reference to section name in get_import_addr #16530

ghost commented Apr 11, 2020 •

edited by ghost

Loading

ret2libc left a comment

ret2libc commented Apr 14, 2020

ghost commented Apr 14, 2020

ghost commented Apr 14, 2020

ret2libc commented Apr 14, 2020

ghost commented Apr 14, 2020

ghost commented Apr 20, 2020

ghost commented Apr 20, 2020

ghost commented Apr 20, 2020

ret2libc left a comment

ghost commented Apr 21, 2020

ret2libc commented Apr 21, 2020

lgtm-com bot commented Apr 21, 2020

ghost commented Apr 21, 2020

ret2libc commented Apr 21, 2020

ghost commented Apr 21, 2020

radare left a comment

ret2libc commented Apr 21, 2020

ret2libc left a comment

Refactor of elf.c and remove reference to section name in get_import_addr #16530

Refactor of elf.c and remove reference to section name in get_import_addr #16530

Conversation

ghost commented Apr 11, 2020 • edited by ghost Loading

ret2libc left a comment

Choose a reason for hiding this comment

ret2libc commented Apr 14, 2020

ghost commented Apr 14, 2020

ghost commented Apr 14, 2020

ret2libc commented Apr 14, 2020

ghost commented Apr 14, 2020

ghost commented Apr 20, 2020

get_import_addr_arm

get_import_addr_mips

get_import_addr_riscv

get_import_sparc

get_import_ppc

get_import_x86

Conclusion

ghost commented Apr 20, 2020

ghost commented Apr 20, 2020

ret2libc left a comment

Choose a reason for hiding this comment

ghost commented Apr 21, 2020

ret2libc commented Apr 21, 2020

lgtm-com bot commented Apr 21, 2020

ghost commented Apr 21, 2020

ret2libc commented Apr 21, 2020

ghost commented Apr 21, 2020

radare left a comment

Choose a reason for hiding this comment

ret2libc commented Apr 21, 2020

ret2libc left a comment

Choose a reason for hiding this comment

ghost commented Apr 11, 2020 •

edited by ghost

Loading