Page MenuHomePhabricator

[llvm-readobj] Little clean up inside `parseDynamicTable`
ClosedPublic

Authored by Higuoxing on Dec 5 2018, 7:53 AM.

Details

Summary

This anoymous function actually has same logic with Obj->toMappedAddr.

Besides, I have a question on resolving illegal value. gnu-readelf, gnu-objdump and llvm-objdump could parse the test file 'test/tools/llvm-objdump/Inputs/private-headers-x86_64.elf', but llvm-readobj will fail when parse DT_RELR segment. Because, the value is 0x87654321 which is illegal. So, shall we do this clean up rather then remove the checking statements inside anoymous function?

if (Delta >= Phdr.p_filesz)
    return createError("Virtual address is not in any segment");

Diff Detail

Event Timeline

Higuoxing created this revision.Dec 5 2018, 7:53 AM
Higuoxing edited the summary of this revision. (Show Details)Dec 5 2018, 7:53 AM
jhenderson accepted this revision.Dec 7 2018, 1:52 AM

LGTM for this change. The refactor is certainly good. I feel like I might have suggested this in a previous review a while back, but it obviously never happened (see D49016).

As for whether or not we should error in this case, the problem is that things like the Hash table are looked up using this function, and then an attempt may be made to dump the hash table from the loaded address. Potentially, this could lead to trying to access invalid memory, which is obviously bad. So perhaps before making any decision, you should look up what the implications would be of removing the error?

This revision is now accepted and ready to land.Dec 7 2018, 1:52 AM

LGTM for this change. The refactor is certainly good. I feel like I might have suggested this in a previous review a while back, but it obviously never happened (see D49016).

As for whether or not we should error in this case, the problem is that things like the Hash table are looked up using this function, and then an attempt may be made to dump the hash table from the loaded address. Potentially, this could lead to trying to access invalid memory, which is obviously bad. So perhaps before making any decision, you should look up what the implications would be of removing the error?

Thanks for reviewing! I will look into it : )

This revision was automatically updated to reflect the committed changes.

FWIW: The check you mention in ELF.cpp is triggered by the following test script. This causes llvm-readelf to generate an unexpected error message, LLVM ERROR: Virtual address is not in any segment

cat > 1.c << \!
int foo = 10;
int main() { return foo; }
!

cat > script.t << \!
PHDRS {
  CODE_RO PT_LOAD;
  DATA PT_LOAD;
  GOT PT_LOAD;
  DYN PT_LOAD;
  DYNAMIC PT_DYNAMIC;
  RELADYN PT_LOAD;
}
SECTIONS {
  .text : { *(.text) } :CODE_RO
  .data : { *(.data) } :DATA
  .dynamic : { *(.dynamic) }: DYN : DYNAMIC
  .rela.dyn : {*(.rel.dyn)} : RELADYN
}
!

gcc -c 1.c -fPIC
ld -shared 1.o -T script.t -Bsymbolic -pie -soname foo -o foo
echo llvm-readelf:
llvm-readelf -d foo
echo readelf:
readelf -d foo

FWIW: The check you mention in ELF.cpp is triggered by the following test script. This causes llvm-readelf to generate an unexpected error message, LLVM ERROR: Virtual address is not in any segment

cat > 1.c << \!
int foo = 10;
int main() { return foo; }
!

cat > script.t << \!
PHDRS {
  CODE_RO PT_LOAD;
  DATA PT_LOAD;
  GOT PT_LOAD;
  DYN PT_LOAD;
  DYNAMIC PT_DYNAMIC;
  RELADYN PT_LOAD;
}
SECTIONS {
  .text : { *(.text) } :CODE_RO
  .data : { *(.data) } :DATA
  .dynamic : { *(.dynamic) }: DYN : DYNAMIC
  .rela.dyn : {*(.rel.dyn)} : RELADYN
}
!

gcc -c 1.c -fPIC
ld -shared 1.o -T script.t -Bsymbolic -pie -soname foo -o foo
echo llvm-readelf:
llvm-readelf -d foo
echo readelf:
readelf -d foo

Thank you @sidneym, it's very helpful!

I am considering suppress the failure in llvm-readobj and just give a warning. I think this tool should give us enough information on binaries and should not crash because of an invalid value, right? But I still have no idea by now ... I don't want break the ELF.cpp internal API ...