When computing relocations, use DYNAMIC segment if available #16419
Conversation
|
I am testing to see why it failed, and i was wandering if one test could be wrong? file: ./test/new/db/cmd/cmd_i ../bins/elf/ifunc_rel64 hasn't any .dynamic section. Readelf believe that there are some relocations, but the ld don't do any relocation on this file. |
|
And there is the problem of the .obj file... |
|
In the case of ./test/bins/elf/radare2.c.obj which relocations r2 need? |
|
I think what happens in this case is that the function is statically linked (as The thing is that we still want the information provided by the sections because, as you said, in some cases they are the only information we have and in other cases they may enhance what we already have. However, I think care must be taken to ensure that first we load info from the dynamic segment and only later we enhance those info with the section information (if there are conflicts between info provided by DYNAMIC segment and .rel sections, DYNAMIC should win). Does that make sense? What do you think? |
|
Okay, that doesn't seems to be to complicated but how could we detect collision? The second problem is detecting valid rel or rela section. If a section use 'rel' or 'rela' in his name it doesn't guarantee that the section hold relocation entries. Maybe we could list all the section's names with a special meaning? |
I think you can first get relocations from DYNAMIC (like what you are doing right now), then extract them from sections (just get the old code before your change, it should work well enough). While getting relocations from sections, check if there is already one at the same
There is nothing you can do about this. The only way to differentiate sections is with their names AFAIK. |
|
The problem with the vaddr is that some debug reloc could have the same addr than .dynamic reloc. |
|
There is two similar function 'Elf_(r_bin_elf_get_relocs)' and 'rel_cache_new'. Should i merge them? |
|
We could use the new array of reloc to generate the hash map. |
|
This pull request fixes 1 alert when merging 357ce4d into e484762 - view on LGTM.com fixed alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #16419 +/- ##
==========================================
+ Coverage 38.79% 39.04% +0.24%
==========================================
Files 994 994
Lines 324022 324528 +506
==========================================
+ Hits 125697 126696 +999
+ Misses 198325 197832 -493
Continue to review full report at Codecov.
|
|
@08a I think this is not working as expected. I tried the tests in https://github.com/radareorg/radare2-regressions/commit/19ccc45154ca69041b8432147197738a453419e5 and it is still failing. Are you using the info from dynamic? |
|
can you add a test? |
|
The https://github.com/radareorg/radare2-regressions/commit/19ccc45154ca69041b8432147197738a453419e5 can't work on __libc_start_main because there is no relocation related to this symbol. |
import sys
import lief
def rename_section_rela(filename, output):
binary = lief.parse(filename)
for i, section in enumerate(binary.sections):
if ("rel" in section.name):
section.name = f"section_number_{i}"
binary.write(output);
if __name__ == "__main__":
if len(sys.argv) != 3:
print("Usage: {} <elf binary> <output>".format(sys.argv[0]))
sys.exit(1)
rename_section_rela(sys.argv[1], sys.argv[2])I used this small script on this simple binary #include <stdio.h>
int main(void) {
printf("Hello World!!!");
return 0;
}I will send a pr on https://github.com/radareorg/radare2-testbins |
ghost
mentioned this pull request
|
This pull request fixes 1 alert when merging c3a6e7a into 6d633a9 - view on LGTM.com fixed alerts:
|
Somehow I uploaded a wrong binary in that commit :/ As you say, that bin does not work, sorry about that. Could you please update it so that it works or just remove it if you already have added another test for this behaviour? I will review your code today/tomorrow, but the results look good! Thanks :) |
|
I need some advice for this pr. I believe some refacto can be done.
|
|
This pull request fixes 1 alert when merging 5be706a into 38fa548 - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Thanks a lot! I had a look at the code, overall it looks very good. I added comments here and there. Tell me what you think.
About the refactoring you mentioned, I think they are valid points, but I would do those things after merging this PR, to avoid changing too many things at the same time with the risk of breaking and never merging the change. Ok?
| return bin->g_sections[pos].info < bin->ehdr.e_shnum && bin->shdr; | ||
| } | ||
|
|
||
| static size_t get_next_not_analysed_offset(size_t section_offset, size_t offset, |
There was a problem hiding this comment.
i have an hard time following this function :(
can it be simplified a bit? also, i'm not sure it's right that we check for all the if cases when we are really working only with one type of rel (e.g. if i'm iterating over rela entries, this function may still check info->addr_jmprel - base_addr <= g_offset && g_offset < info->addr_jmprel + info->jmprel_size - base_addr.
There was a problem hiding this comment.
The main problem is that we could have obfuscated binary like that
section: rel.plt
false_relocation | real rela table | jmprel table | false rel table
We can't trust the type of relocation in the name.
Maybe we could had an option to load or not the relocations record from section?
It could be an start's option.
ghost
requested a review
from
There was a problem hiding this comment.
Just fix the last few things and I think we can remove the WIP status and merge it. Are you ok with the current state of things? As you said, it's not only about this because there are other parts of the code that still use section names, but those can be done separately. Great work btw :)
|
I am satisfied with the state of the PR, later today or tomorrow i will send an other PR to add some re-factorization. |
ret2libc
changed the title
|
This pull request fixes 1 alert when merging 4c3918f into a52506a - view on LGTM.com fixed alerts:
|
Your checklist for this pull request
Detailed description
I start working on the dynamic section, the ir command found all the relocations. I don't know why the pd 15 command doesn't use all the relocs (new/db/formats/elf/reloc). I used readelf to test the relocation. Maybe later we could move the struct dynamic_relocation_section in the struct ELFOBJ?
Test plan
Closing issues
#12732