Read either DT_HASH or DT_GNU_HASH to determine the size of DT_SYMTAB [crashpad/crashpad : master]
Scott Graham (Gerrit)
(Should have just happily ignored that quiet little "bad offset" error! :)
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
3 comments:
File snapshot/elf/elf_dynamic_array_reader.h:
Patch Set #6, Line 60:
LOG(ERROR)Could be "PLOG_IF(ERROR, log) << ..."
File snapshot/elf/elf_image_reader.h:
Patch Set #6, Line 219:
size_tWhile we're unlikely to overflow a native size_t in this use-case, VMSize makes it more clear to me that we're talking about a size in the target process' address space.
File snapshot/elf/elf_image_reader.cc:
Patch Set #6, Line 621:
(memory_.Is64Bit()GetNumberOfSymbolEntriesFromGnuHash also has access to memory_ and only uses the WordSize once. I'd consider making this check there, rather than templatizing the whole method.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
1 comment:
Patch Set #6, Line 60:
LOG(ERROR)
Could be "PLOG_IF(ERROR, log) << ... […]
*"LOG_IF(..."
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Thanks!
3 comments:
Patch Set #6, Line 60:
LOG(ERROR)
*"LOG_IF(... […]
Done
File snapshot/elf/elf_image_reader.h:
Patch Set #6, Line 219:
size_t
While we're unlikely to overflow a native size_t in this use-case, VMSize makes it more clear to me […]
Done
File snapshot/elf/elf_image_reader.cc:
Patch Set #6, Line 621:
(memory_.Is64Bit()
GetNumberOfSymbolEntriesFromGnuHash also has access to memory_ and only uses the WordSize once. […]
Done.
(When I started writing it, I thought Elf32_Word and Elf64_Word were different sizes. Oh, to be so naive and carefree again...)
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Patch set 7:Code-Review +1
Mark Mentovai (Gerrit)
13 comments:
File snapshot/elf/elf_dynamic_array_reader.h:
Patch Set #7, Line 53:
//! \param[out] value The value, casted to an appropriate type, if found.\param[in] log
Patch Set #7, Line 56:
bool GetValue(uint64_t tag, V* value, bool log) {And reorder to put the “in” parameters together before the “out” one.
File snapshot/elf/elf_image_reader.h:
Patch Set #7, Line 217:
bool GetAddressFromDynamicArray(uint64_t tag, VMAddress* address, bool log);Reorder parameters here too.
Patch Set #7, Line 219:
bool GetNumberOfSymbolEntriesFromHash(VMSize* number_of_symbol_table_entries);Even if you’re moderately familiar with ELF, “from hash” is kinda like “what hash? how can you read into a hash at all?”
GetNumberOfSymbolEntriesFromDtHash? And then the same with the next one, FromDtGnuHash?
Patch Set #7, Line 221:
VMSize* number_of_symbol_table_entries);It’d be nice to have a test that ensures that if both of these are present (as they normally are on Linux, at least), that they produce consistent results.
File snapshot/elf/elf_image_reader.cc:
Patch Set #7, Line 621:
if (!GetNumberOfSymbolEntriesFromGnuHash(&number_of_symbol_table_entries)) {&&-chain these instead of nesting.
Patch Set #7, Line 656:
// nchain is the second word in the DT_HASH table.I’d rather see this done through a struct. The custom arithmetic (and required commentary) isn’t great form.
Patch Set #7, Line 675:
GnuHashHeaderThis struct can be totally anonymous, and you can write sizeof(header) instead of sizeof(GnuHashHeader) below. Generally it’s better to use sizeof on a variable than a type when you have one handy anyway.
Patch Set #7, Line 675:
struct GnuHashHeader {The links that describe the DT_GNU_HASH format from the commit message belong here too.
Patch Set #7, Line 686:
uint32_t buckets[header.nbuckets];This is an alloca() equivalent, no good. You need to dynamically allocate the memory instead.
Patch Set #7, Line 699:
if (buckets[i] > last_symbol) {std::max
File snapshot/elf/elf_symbol_table_reader.cc:
Patch Set #7, Line 77:
elf_reader_->ReadDynamicStringTableAtOffset(entry.st_name, &string)This clause was primarily intended to detect the end of the symbol table before, but that’s wholly unnecessary now.
Now that we know how large the table is supposed to be, I think that this should move inside the “while” to the “if”. Failure to read a symbol’s name from the string table shouldn’t stop the symbol table scan altogether.
Patch Set #7, Line 87:
address += sizeof(entry);Leave a TODO here to implement this using DT_SYMENT if present instead of sizeof(entry). You don’t have to actually implement that in this change.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Much better, thanks!
13 comments:
File snapshot/elf/elf_dynamic_array_reader.h:
Patch Set #7, Line 53:
//! \param[in] log Specifies whether an error should be logged if \a tag is\param[in] log
Done
Patch Set #7, Line 56:
//! \return `true` if the value is found.
And reorder to put the “in” parameters together before the “out” one.
Done
File snapshot/elf/elf_image_reader.h:
Patch Set #7, Line 217:
bool GetAddressFromDynamicArray(uint64_t tag, bool log, VMAddress* address);Reorder parameters here too.
Done
Patch Set #7, Line 219:
bool GetNumberOfSymbolEntriesFromDtHash(Even if you’re moderately familiar with ELF, “from hash” is kinda like “what hash? how can you read […]
Done
Patch Set #7, Line 221:
bool GetNumberOfSymbolEntriesFromDtGnuHash(It’d be nice to have a test that ensures that if both of these are present (as they normally are on […]
Aha, I haven't been able to build on Linux. I'll look into that and then add a test. (I guess we could probably have the bots that I added for Fuchsia be doing Linux builds on the waterfall too.)
File snapshot/elf/elf_image_reader.cc:
Patch Set #7, Line 621:
!GetNumberOfSymbolEntriesFromDtGnuHash(&number_of_symbol_table_entries)) {
&&-chain these instead of nesting.
Done
I’d rather see this done through a struct. […]
Done
Patch Set #7, Line 675:
DtGnuHashHeadThis struct can be totally anonymous, and you can write sizeof(header) instead of sizeof(GnuHashHead […]
Done
Patch Set #7, Line 675:
struct DtGnuHashHeader {
The links that describe the DT_GNU_HASH format from the commit message belong here too.
Done
Patch Set #7, Line 686:
std::vector<uint32_t> buckets(header.nbuckets);
This is an alloca() equivalent, no good. You need to dynamically allocate the memory instead.
Done
Patch Set #7, Line 699:
for (uint32_t i = 0; i < header.nbuckets; ++i) {std::max
Done
File snapshot/elf/elf_symbol_table_reader.cc:
Patch Set #7, Line 77:
lf_reader_->ReadDynamicStringTableAtOffset(entry.st_name, &string)This clause was primarily intended to detect the end of the symbol table before, but that’s wholly u […]
Done
Patch Set #7, Line 87:
// TODO(scottmg): This should respect DT_SYMENT if present.Leave a TODO here to implement this using DT_SYMENT if present instead of sizeof(entry). […]
Done
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
1 comment:
File snapshot/elf/elf_image_reader.h:
Aha, I haven't been able to build on Linux. I'll look into that and then add a test. […]
Got it building on Linux and added a test to the latest patchset, but at least in crashpad_snapshot_test I don't have a .gnu.hash, only .hash.
https://gist.github.com/anonymous/75ef73afdf8fae6f3d8fea9f0c51408c
I guess what I'll need to do is track down how to force .hash, .gnu.hash, or both and then build separate targets for all 3 that the test can inspect.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
1 comment:
Got it building on Linux and added a test to the latest patchset, but at least in crashpad_snapshot_ […]
(I'm off in the weeds sorting this out, but I think the DT_GNU_HASH reading isn't quite right. I'll ping when this is ready for another look.)
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
1 comment:
(I'm off in the weeds sorting this out, but I think the DT_GNU_HASH reading isn't quite right. […]
OK, there's a working Linux test now. I've run into some weirdness that I can't explain, as well as a variety of problems that would be good to fix, but I haven't, yet:
- BuildArtifact() doesn't include a 'lib' prefix on Linux which loadable_module in gyp adds (?) so the test dlopen()s an executable instead, which is fine, but a bit unusual. I'm not sure if it should be added there or removed in the build?
- dlopen() doesn't return a link_map on Android, so the test doesn't work on Android, but I think it could otherwise with another way to find the elf base.
- dlopen() always seems to fail on Fuchsia (in other tests too). I haven't tracked down why that is.
- Fuchsia would probably fail for the link_map reason too.
- I accidentally didn't export any symbols from the test binary at first (forgetting the whole --dynamic-list=test_exported_symbols.sym thing almost right away!) and after some debugging, I *think* `ld` isn't emitting a correct .gnu.hash. Or rather, I guess since there's no spec, it's correct-by-definition, but I don't understand how it would be possible to get the correct count for DT_SYMTAB. With no explicit exported symbols, there's 8 things in the symbol table afaict:
https://gist.github.com/sgraham/2e6dcfb459bb7367a0ce636a7e66198e
.hash says nbuckets is 8 (the second word), but .gnu.hash looks basically empty, so I don't know how I could derive an "8" from that.
When there's 1 (or more) exported symbols it looks like this:
https://gist.github.com/sgraham/b459cfc5b1793c804dac92b8c7739cdf
where .hash reports 9, and .gnu.hash has a single bucket entry, + the 8 non-hashed symbols, so also resulting in 9 as expected.
So, the test binary exports a single symbol. :/
I don't think the no-exports case works with .gnu.hash (i.e. on Fuchsia), but I guess that's probably not a huge problem in practice.
(Alternatively, I'm totally misunderstanding something about the section's format which is very much possible.)
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
2 comments:
OK, there's a working Linux test now. I've run into some weirdness that I can't explain, as well as a variety of problems that would be good to fix, but I haven't, yet:
- BuildArtifact() doesn't include a 'lib' prefix on Linux which loadable_module in gyp adds (?) so the test dlopen()s an executable instead, which is fine, but a bit unusual. I'm not sure if it should be added there or removed in the build?
As opposed to shared_library, I think it’s intended for loadable_module to never include a “lib” prefix. If one meta-build system or another doesn’t do this, we can manually clear out the prefix.
- dlopen() doesn't return a link_map on Android, so the test doesn't work on Android, but I think it could otherwise with another way to find the elf base.
- dlopen() always seems to fail on Fuchsia (in other tests too). I haven't tracked down why that is.
Uh-oh.
- Fuchsia would probably fail for the link_map reason too.
- I accidentally didn't export any symbols from the test binary at first (forgetting the whole --dynamic-list=test_exported_symbols.sym thing almost right away!) and after some debugging, I *think* `ld` isn't emitting a correct .gnu.hash. Or rather, I guess since there's no spec, it's correct-by-definition, but I don't understand how it would be possible to get the correct count for DT_SYMTAB. With no explicit exported symbols, there's 8 things in the symbol table afaict:
https://gist.github.com/sgraham/2e6dcfb459bb7367a0ce636a7e66198e
.hash says nbuckets is 8 (the second word), but .gnu.hash looks basically empty, so I don't know how I could derive an "8" from that.
When there's 1 (or more) exported symbols it looks like this:
https://gist.github.com/sgraham/b459cfc5b1793c804dac92b8c7739cdf
where .hash reports 9, and .gnu.hash has a single bucket entry, + the 8 non-hashed symbols, so also resulting in 9 as expected.
So, the test binary exports a single symbol. :/
I don't think the no-exports case works with .gnu.hash (i.e. on Fuchsia), but I guess that's probably not a huge problem in practice.
Meaning that this is broken for a module that only imports things (has undefined symbols)?
(Alternatively, I'm totally misunderstanding something about the section's format which is very much possible.)
I think the next thing to do is look at how the linker is generating DT_GNU_HASH/.gnu.hash. I can help look. Is it gold or lld? (And, if it’s easily switchable, does that change anything?)
This may even be a linker bug.
File snapshot/elf/elf_image_reader.cc:
Patch Set #10, Line 683:
struct DtGnuHashHeader {Is “struct {” OK here? That’s what I meant by “anonymous” before. Same on line 659.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
2 comments:
OK, there's a working Linux test now. I've run into some weirdness that I can't explain, as well as a variety of problems that would be good to fix, but I haven't, yet:
- BuildArtifact() doesn't include a 'lib' prefix on Linux which loadable_module in gyp adds (?) so the test dlopen()s an executable instead, which is fine, but a bit unusual. I'm not sure if it should be added there or removed in the build?
As opposed to shared_library, I think it’s intended for loadable_module to never include a “lib” prefix. If one meta-build system or another doesn’t do this, we can manually clear out the prefix.
- dlopen() doesn't return a link_map on Android, so the test doesn't work on Android, but I think it could otherwise with another way to find the elf base.
- dlopen() always seems to fail on Fuchsia (in other tests too). I haven't tracked down why that is.
Uh-oh.
- Fuchsia would probably fail for the link_map reason too.
- I accidentally didn't export any symbols from the test binary at first (forgetting the whole --dynamic-list=test_exported_symbols.sym thing almost right away!) and after some debugging, I *think* `ld` isn't emitting a correct .gnu.hash. Or rather, I guess since there's no spec, it's correct-by-definition, but I don't understand how it would be possible to get the correct count for DT_SYMTAB. With no explicit exported symbols, there's 8 things in the symbol table afaict:
https://gist.github.com/sgraham/2e6dcfb459bb7367a0ce636a7e66198e
.hash says nbuckets is 8 (the second word), but .gnu.hash looks basically empty, so I don't know how I could derive an "8" from that.
When there's 1 (or more) exported symbols it looks like this:
https://gist.github.com/sgraham/b459cfc5b1793c804dac92b8c7739cdf
where .hash reports 9, and .gnu.hash has a single bucket entry, + the 8 non-hashed symbols, so also resulting in 9 as expected.
So, the test binary exports a single symbol. :/
I don't think the no-exports case works with .gnu.hash (i.e. on Fuchsia), but I guess that's probably not a huge problem in practice.
Meaning that this is broken for a module that only imports things (has undefined symbols)?
Yeah, I think so.
> (Alternatively, I'm totally misunderstanding something about the section's format which is very much possible.)I think the next thing to do is look at how the linker is generating DT_GNU_HASH/.gnu.hash. I can help look. Is it gold or lld? (And, if it’s easily switchable, does that change anything?)
Good question, I'm not sure! I was just doing a default gyp build, so whatever it did, which appears to just be "c++" operating as a clang fake.
[hash-chains] scottmg@around:/work/crashpad/crashpad$ rm out/Debug/crashpad_snapshot_test
[hash-chains] scottmg@around:/work/crashpad/crashpad$ ninja -C out/Debug crashpad_snapshot_test -j1 -v
ninja: Entering directory `out/Debug'
[0->1/1 ~1] c++ -fPIC -pthread -Wl,--as-needed -Wl,-z,noexecstack -m64 -Wl,--dynamic-list=test_exported_symbols.sym -pie -o crashpad_snapshot_test -Wl,--start-group obj/snapshot/crashpad_snapshot_test.cpu_context_test.o obj/snapshot/crashpad_types/crashpad_snapshot_test.crashpad_info_reader_test.o obj/snapshot/crashpad_types/crashpad_snapshot_test.image_annotation_reader_test.o obj/snapshot/elf/crashpad_snapshot_test.elf_image_reader_test.o obj/snapshot/elf/crashpad_snapshot_test.elf_image_reader_test_note.o obj/snapshot/linux/crashpad_snapshot_test.debug_rendezvous_test.o obj/snapshot/linux/crashpad_snapshot_test.exception_snapshot_linux_test.o obj/snapshot/linux/crashpad_snapshot_test.process_reader_test.o obj/snapshot/linux/crashpad_snapshot_test.system_snapshot_linux_test.o obj/snapshot/minidump/crashpad_snapshot_test.process_snapshot_minidump_test.o obj/snapshot/posix/crashpad_snapshot_test.timezone_test.o obj/snapshot/libcrashpad_snapshot.a obj/client/libcrashpad_client.a obj/test/libcrashpad_gtest_main.a obj/test/libcrashpad_test.a obj/third_party/gtest/libgtest.a obj/util/libcrashpad_util.a obj/third_party/mini_chromium/mini_chromium/base/libbase.a -Wl,--end-group -ldl -lcurl -lz
[hash-chains] scottmg@around:/work/crashpad/crashpad$ which c++
/usr/bin/c++
[hash-chains] scottmg@around:/work/crashpad/crashpad$ ls -l /usr/bin/c++
lrwxrwxrwx 1 root root 21 Sep 25 10:10 /usr/bin/c++ -> /etc/alternatives/c++
[hash-chains] scottmg@around:/work/crashpad/crashpad$ ls -l /etc/alternatives/c++
lrwxrwxrwx 1 root root 20 Jan 22 15:18 /etc/alternatives/c++ -> /usr/bin/clang++-3.9But adding -print-prog-name=ld results in:
/usr/bin/ld[hash-chains] scottmg@around:/work/crashpad/crashpad/out/Debug$ c++ -fPIC -pthread -Wl,--as-needed -Wl,-z,noexecstack -m64 -Wl,--dynamic-list=test_exported_symbols.sym -pie -o crashpad_snapshot_test -Wl,--start-group obj/snapshot/crashpad_snapshot_test.cpu_context_test.o obj/snapshot/crashpad_types/crashpad_snapshot_test.crashpad_info_reader_test.o obj/snapshot/crashpad_types/crashpad_snapshot_test.image_annotation_reader_test.o obj/snapshot/elf/crashpad_snapshot_test.elf_image_reader_test.o obj/snapshot/elf/crashpad_snapshot_test.elf_image_reader_test_note.o obj/snapshot/linux/crashpad_snapshot_test.debug_rendezvous_test.o obj/snapshot/linux/crashpad_snapshot_test.exception_snapshot_linux_test.o obj/snapshot/linux/crashpad_snapshot_test.process_reader_test.o obj/snapshot/linux/crashpad_snapshot_test.system_snapshot_linux_test.o obj/snapshot/minidump/crashpad_snapshot_test.process_snapshot_minidump_test.o obj/snapshot/posix/crashpad_snapshot_test.timezone_test.o obj/snapshot/libcrashpad_snapshot.a obj/client/libcrashpad_client.a obj/test/libcrashpad_gtest_main.a obj/test/libcrashpad_test.a obj/third_party/gtest/libgtest.a obj/util/libcrashpad_util.a obj/third_party/mini_chromium/mini_chromium/base/libbase.a -Wl,--end-group -ldl -lcurl -lz -print-prog-name=ld
/usr/bin/ld
[hash-chains] scottmg@around:/work/crashpad/crashpad/out/Debug$ ls -l /usr/bin/ld
lrwxrwxrwx 1 root root 19 Aug 23 14:51 /usr/bin/ld -> x86_64-linux-gnu-ld
[hash-chains] scottmg@around:/work/crashpad/crashpad/out/Debug$ ls -l /usr/bin/x86_64-linux-gnu-ld
lrwxrwxrwx 1 root root 23 Aug 23 14:51 /usr/bin/x86_64-linux-gnu-ld -> x86_64-linux-gnu-ld.bfd
[hash-chains] scottmg@around:/work/crashpad/crashpad/out/Debug$ ls -l /usr/bin/x86_64-linux-gnu-ld.bfd
-rwxr-xr-x 1 root root 1269128 Aug 23 14:51 /usr/bin/x86_64-linux-gnu-ld.bfd
[hash-chains] scottmg@around:/work/crashpad/crashpad/out/Debug$ /usr/bin/x86_64-linux-gnu-ld.bfd -version
GNU ld (GNU Binutils for Debian) 2.29
Copyright (C) 2017 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.So I guess clang-3.9 as compiler, but plain old GNU ld as linker.
... And the gn build doesn't work yet
... But maybe that's because it defaults to clang.
... CXX=clang LD=lld build/gyp_crashpad.py also doesn't build
... compile fails in some places
... test_exported_symbols.sym isn't a loved linker script then
... My clang is a bit old, 3.9, maybe that's relevant
... I failed at running the link directly without using the compiler as a driver for the linker in order to compare them.
... Um, hrm. What was I doing again? :)
>
> This may even be a linker bug.
Yeah, I'm ready to blame anything at this point.
I'll go away for a while and try to figure out which versions of which linkers (if any) can generate sensible .gnu.hash for the import-only case.
Stepping back a bit, I think on Fuchsia assuming clang/lld is fine, so we can fix any problem if it exists there.Android and Linux it seems like we can't rely on that, or any particular revision of the tools for that matter. So perhaps the code needs to change to only use .hash on Linux/Android, and only use .gnu.hash on Fuchsia. Though that's a bummer for this test.
File snapshot/elf/elf_image_reader.cc:
Patch Set #10, Line 683:
struct DtGnuHashHeader {Is “struct {” OK here? That’s what I meant by “anonymous” before. Same on line 659.
Oops, sorry. Done.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
1 comment:
> > OK, there's a working Linux test now. […]
binutils-2.29/bfd/elflink.c:7016
if (cinfo.nsyms == 0)
{
/* Empty .gnu.hash section is special. */
BFD_ASSERT (cinfo.min_dynindx == -1);
free (cinfo.hashcodes);
s->size = 5 * 4 + bed->s->arch_size / 8;
contents = (unsigned char *) bfd_zalloc (output_bfd, s->size);
if (contents == NULL)
return FALSE;
s->contents = contents;
/* 1 empty bucket. */
bfd_put_32 (output_bfd, 1, contents);
/* SYMIDX above the special symbol 0. */
bfd_put_32 (output_bfd, 1, contents + 4);
/* Just one word for bitmask. */
bfd_put_32 (output_bfd, 1, contents + 8);
/* Only hash fn bloom filter. */
bfd_put_32 (output_bfd, 0, contents + 12);
/* No hashes are valid - empty bitmask. */
bfd_put (bed->s->arch_size, output_bfd, 0, contents + 16);
/* No hashes in the only bucket. */
bfd_put_32 (output_bfd, 0,
contents + 16 + bed->s->arch_size / 8);
}
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
1 comment:
> > OK, there's a working Linux test now. […]
OK, here's a verbose log of a simplified case (trying the no-exports a variety of ways): https://gist.github.com/sgraham/43511c7f2467143398e71514f7fff406
- Fuchsia's clang (which defaults to lld)
- Chrome's clang (which defaults to ld)
- Chrome's clang, with -fuse-ld=lld
- Chrome's clang, with -fuse-ld=gold
The summary is: lld and gold has a .gnu.hash that lgtm, ld generates what I believe is a broken one, or at least it follows some sort of different rules that I don't understand.
So! For the task at hand (assuming it's relatively fruitless to attempt to upstream and downstream an ld.bfd change):
- Only actually read .hash on Linux the runtime code, since we have to expect ld modules (Not sure about Android.)
- Once the test works on Fuchsia, have a no-exports and a one-exports case, that compares sizes from .hash and .gnu.hash.
- If we can decide/rely on the linker being lld or gold on Linux internally for Crashpad, then enable that test on Linux, but that'd be test-only.
Whew! And yuck!
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
1 comment:
Missed your comment, and Gerrit can't quote the non-last comment, awesomely.
binutils-2.29/bfd/elflink.c:7016
Ah, thank you!!! (But whyyyy) It seems like the length dt_symtab isn't derivable from that data though... or am I missing something obvious?
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
1 comment:
Missed your comment, and Gerrit can't quote the non-last comment, awesomely. […]
I think the length of the symbol table isn't really intended to be derived from the .gnu.hash section. It's just meant for lookup, which works because bloom is zero, indicating no exported symbols. Tools like readelf that do list all the symbols do it by dividing the section size by the symbol entry size, which we can't do without section headers.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
1 comment:
I think the length of the symbol table isn't really intended to be derived from the .gnu. […]
That said, the comment
/* SYMIDX above the special symbol 0. */
makes it sound like it's still an error since it assumes there is only one non-exported symbol.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
We can’t rule out the possibility of having to deal with BFD ld output on Linux.
If BFD ld only writes this bogus DT_GNU_HASH when there are no _defined_ symbols, then we’re probably OK. We don’t currently have any need to look up UNdefined symbols, and I don’t envision needing to. So long as we won’t be harmed by an incorrect symbol count of 1, then we can probably live with this.
But I suppose it’s possible, although highly unlikely, for there to be 0 symbols.
Scott Graham (Gerrit)
Alrighty, not the most satisfying situation, but I tracked down the .so loading problem on Fuchsia, filed a bug upstream for that ZX-1619, added a workaround for that here, and then made the .hash vs. .gnu.hash test Fuchsia-only for now.
I think this is reasonable because:
- lld is the only linker that will be in use on Fuchsia, so the sections should be as expected there
- .hash is tried before .gnu.hash, so other platforms should generally be fine, and this problem only happens in the no-exports case as far as we know, so even if we encounter a ld.bfd .gnu.hash it should generally be fine.
Mark Mentovai (Gerrit)
LGTM based on all of the stuff we talked about above. This was a rough one, thanks for digging in!
Patch set 16:Code-Review +1
1 comment:
File snapshot/elf/elf_image_reader.cc:
Patch Set #16, Line 618:
// circuitously offer a way to find the number of entries in the symbol table.Comment that you’re trying DT_HASH first because DT_GNU_HASH can be incorrect, but waffle by saying that it’s hopefully not incorrect in any situation that counts.
In fact, you may want to list this limitation in the //! doxygen documentation for the DT_GNU_HASH variant.
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Thanks
(It was more involved than I thought it was going to be when I quickly peeked at what DT_HASH looked like!)
Patch set 17:Commit-Queue +2
1 comment:
Patch Set #16, Line 618:
// circuitously offer a way to find the number of entries in the symbol table.
Comment that you’re trying DT_HASH first because DT_GNU_HASH can be incorrect, but waffle by saying […]
Done
To view, visit change 876879. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Read either DT_HASH or DT_GNU_HASH to determine the size of DT_SYMTAB
Without the section headers for the symbol table, there's no direct way
to calculate the number of entries in the table.
DT_HASH and DT_GNU_HASH are auxiliary tables that are designed to make
symbol lookup faster. DT_HASH is the original and is theoretically
mandatory. DT_GNU_HASH is the new-and-improved, but is more complex.
In practice, however, an Android build (at least vs. API 16) has only
DT_HASH, and not DT_GNU_HASH, and a Fuchsia build has only DT_GNU_HASH
but not DT_HASH. So, both are tried.
This change does not actually use the data in these tables to improve
the speed of symbol lookup, but instead only uses them to correctly
terminate the linear search.
DT_HASH contains the total number of symbols in the symbol table fairly
directly because there is an entry for each symbol table entry in the
hash table, so the number is the same.
DT_GNU_HASH regrettably does not. Instead, it's necessary to walk the
buckets and chain structure to find the largest entry.
DT_GNU_HASH doesn't appear in any "real" documentation that I'm aware
of, other than the binutils code (at least as far as I know). Some
more-and-less-useful references:
- https://flapenguin.me/2017/04/24/elf-lookup-dt-hash/
- https://flapenguin.me/2017/05/10/elf-lookup-dt-gnu-hash/
- http://deroko.phearless.org/dt_gnu_hash.txt
- https://sourceware.org/ml/binutils/2006-10/msg00377.html
Change-Id: I7cfc4372f29efc37446f0931d22a1f790e44076f
Bug: crashpad:213, crashpad:196
Reviewed-on: https://chromium-review.googlesource.com/876879
Commit-Queue: Scott Graham <sco...@chromium.org>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Reviewed-by: Joshua Peraza <jpe...@chromium.org>
---
M build/run_tests.py
M snapshot/BUILD.gn
M snapshot/elf/elf_dynamic_array_reader.h
M snapshot/elf/elf_image_reader.cc
M snapshot/elf/elf_image_reader.h
M snapshot/elf/elf_image_reader_test.cc
M snapshot/elf/elf_symbol_table_reader.cc
M snapshot/elf/elf_symbol_table_reader.h
A snapshot/hash_types_test.cc
M snapshot/snapshot_test.gyp
10 files changed, 279 insertions(+), 23 deletions(-)