devtools: Correctly extract symbol versions in symbol-check #22244

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2021-06-pixie changing 2 files +15 −12
  1. laanwj commented at 6:35 pm on June 14, 2021: member

    I misunderstood the ELF specification for version symbols (verneed): The vn_aux pointer is relative to the main verneed record, not the start of the section.

    This caused many symbols to not be versioned properly in the return value of elf.dyn_symbols. This was discovered in #21454.

    Fix it by correcting the offset computation.

    • xkb versions symbols (using the prefix V), as this library is used by bitcoin-qt, add it to the valid versions in symbol-check.py

    This unfortunately brings to light some symbols that have been introduced since and weren’t caught (from a gitian compile of master):

     0bitcoin-cli: symbol getrandom from unsupported version GLIBC_2.25
 1bitcoin-cli: failed IMPORTED_SYMBOLS
 2bitcoind: symbol getrandom from unsupported version GLIBC_2.25
 3bitcoind: symbol log from unsupported version GLIBC_2.29
 4bitcoind: symbol fcntl64 from unsupported version GLIBC_2.28
 5bitcoind: symbol pow from unsupported version GLIBC_2.29
 6bitcoind: symbol exp from unsupported version GLIBC_2.29
 7bitcoind: failed IMPORTED_SYMBOLS
 8bitcoin-qt: symbol exp from unsupported version GLIBC_2.29
 9bitcoin-qt: symbol fcntl64 from unsupported version GLIBC_2.28
10bitcoin-qt: symbol log from unsupported version GLIBC_2.29
11bitcoin-qt: symbol pow from unsupported version GLIBC_2.29
12bitcoin-qt: symbol statx from unsupported version GLIBC_2.28
13bitcoin-qt: symbol getrandom from unsupported version GLIBC_2.25
14bitcoin-qt: symbol renameat2 from unsupported version GLIBC_2.28
15bitcoin-qt: symbol getentropy from unsupported version GLIBC_2.25
16bitcoin-qt: failed IMPORTED_SYMBOLS
17bitcoin-wallet: symbol exp from unsupported version GLIBC_2.29
18bitcoin-wallet: symbol log from unsupported version GLIBC_2.29
19bitcoin-wallet: symbol fcntl64 from unsupported version GLIBC_2.28
20bitcoin-wallet: failed IMPORTED_SYMBOLS
21test_bitcoin: symbol getrandom from unsupported version GLIBC_2.25
22test_bitcoin: symbol log from unsupported version GLIBC_2.29
23test_bitcoin: symbol fcntl64 from unsupported version GLIBC_2.28
24test_bitcoin: symbol pow from unsupported version GLIBC_2.29
25test_bitcoin: symbol exp from unsupported version GLIBC_2.29
26test_bitcoin: failed IMPORTED_SYMBOLS
  2. devtools: Fix verneed section parsing in pixie 
    I misunderstood the ELF specification for version symbols (verneed):
The `vn_aux` pointer is relative to the main verneed record, not the
start of the section.

This caused many symbols to not be versioned properly in the return
value of `elf.dyn_symbols`. This was discovered in #21454.

Fix it by correcting the offset computation.
    19e598bab0
  3. devtools: Add xkb version to symbol-check 
    xkb versions symbols (using the prefix `V`), as this library is used by
bitcoin-qt, add it to the valid versions in `symbol-check.py`.
    a33381acf5
  4. laanwj added the label Scripts and tools on Jun 14, 2021
  5. laanwj added this to the milestone 22.0 on Jun 14, 2021
  6. laanwj commented at 7:21 pm on June 14, 2021: member
    • exp is used in CRollingBloomFilter::CRollingBloomFilter (in our code)
    • fcntl64 has a data reference, it’s not ever called in the code (objdump -d bitcoin-qt|grep fcntl64@plt shows nothing)
    0$ objdump -R bitcoin-qt|grep fcntl64                                                            
100000000022b25d0 R_X86_64_64       fcntl64@GLIBC_2.28
2$ gdb bitcoin-qt.dbg
3(gdb) info symbol 0x00000000022b25d0
4aSyscall + 176 in section .data

    aSyscall appears to come from the sqlite dependency

    • getentropy is used in QRandomGenerator::SystemGenerator::generate (inside qt)
    • getrandom is used in one place: arc4_stir at evutil_rand.c:? (libevent dependency)
    • log is used in various places throughout the code (mainly for log2_work computation for debug)
    • pow is used in many places in our code, also in qt
    • renameat is used in QFileSystemEngine::renameFile (inside qt)
    • statx is used in three places (QFileSystemMetaData inside qt)
  7. laanwj force-pushed on Jun 14, 2021
  8. achow101 commented at 8:58 pm on June 14, 2021: member
    Is there a reference document that explains what the offset is? I found a doc that mentioned it, but my reading of that also came to the original conclusion that you made.
  9. laanwj commented at 6:58 am on June 15, 2021: member

    Is there a reference document that explains what the offset is?

    I couldn’t find anything. But this matches the objdump output, and what the linker itself seems to do, which seems kind of important for a symbol check.

    All the other offsets (vn_next, vna_next) are relative to the referring record, so it is at least consistent.

  10. laanwj commented at 5:05 pm on June 17, 2021: member

    Found it:

    vn_aux

    The byte offset, from the start of this Elf32_Verneed entry, to the Elf32_Vernaux array of version definitions that are required from the associated file dependency. At least one version dependency must exist. Additional version dependencies can be present, the number being indicated by the vn_cnt value.

    Source: Oracle, of all places…

  11. hebasto commented at 5:37 am on June 18, 2021: member

    Approach ACK c9b19edf9990d5ebf34945ce8e9a3220459d70f3, tested including comparison with master + reverted 634f6ec4eb9997d7bd0f8209fad49a4171d42384 from #20434 (non-pixie).

    * xkb versions symbols (using the prefix `V`), as this library is used by bitcoin-qt...

    The libxkbcommon package was added in #21376.

    This PR works flawlessly. Need more time to reason about the last commit to final ACK.

  12. laanwj commented at 11:16 am on June 18, 2021: member

    Need more time to reason about the last commit to final ACK.

    FWW, the last commit is a continuation of #17538 from 2019, which bumped the minimum glibc to 2.17. See also your comment here you were rightly confused: #17538 (comment)

    Maybe it would be better to rewrite the code so that the minimum GLIBC version isn’t split over two places?

  13. devtools: Integrate ARCH_MIN_GLIBC_VER table into MAX_VERSIONS in symbol-check.py 
    The (ancient) versions specified here were deceptive. Entries older than
MAX_VERSIONS['GLIBC'], which is 2.17, are ignored here. So reorganize
the code to avoid confusion for other people reading this code.
    e8cd3700ee
  14. laanwj force-pushed on Jun 18, 2021
  15. laanwj commented at 11:27 am on June 18, 2021: member

    Maybe it would be better to rewrite the code so that the minimum GLIBC version isn’t split over two places?

    Okay, pushed a new last commit that does this. Instead of hardcoding a separate table for GLIBC, it optionally allows entries in MAX_VERSIONS to be a per-architecture dictionary. I hope this is clearer.

  16. hebasto approved
  17. hebasto commented at 11:46 pm on June 19, 2021: member

    ACK e8cd3700eeb27437f5ea435869c9d61214285fdd

    Okay, pushed a new last commit that does this. Instead of hardcoding a separate table for GLIBC, it optionally allows entries in MAX_VERSIONS to be a per-architecture dictionary. I hope this is clearer.

    Many thanks for that!

  18. hebasto commented at 0:46 am on June 20, 2021: member

    This unfortunately brings to light some symbols that have been introduced since and weren’t caught (from a gitian compile of master):

    On master, with applied #22281, #22287 and this PR:

     0Checking glibc back compat...
 1bitcoind: symbol getrandom from unsupported version GLIBC_2.25
 2bitcoind: failed IMPORTED_SYMBOLS
 3bitcoin-cli: symbol getrandom from unsupported version GLIBC_2.25
 4bitcoin-cli: failed IMPORTED_SYMBOLS
 5test/test_bitcoin: symbol getrandom from unsupported version GLIBC_2.25
 6test/test_bitcoin: failed IMPORTED_SYMBOLS
 7qt/bitcoin-qt: symbol statx from unsupported version GLIBC_2.28
 8qt/bitcoin-qt: symbol getrandom from unsupported version GLIBC_2.25
 9qt/bitcoin-qt: symbol renameat2 from unsupported version GLIBC_2.28
10qt/bitcoin-qt: symbol getentropy from unsupported version GLIBC_2.25
11qt/bitcoin-qt: failed IMPORTED_SYMBOLS

    For statx and renameat2 I opened a dedicated #22280.

  19. laanwj merged this on Jun 21, 2021
  20. laanwj closed this on Jun 21, 2021
  21. hebasto commented at 2:11 am on June 23, 2021: member

    @laanwj

    * `getentropy` is used in `QRandomGenerator::SystemGenerator::generate` (inside qt)

* `getrandom` is used in one place: `arc4_stir at evutil_rand.c:?`  (libevent dependency)

    Removed in #22318.

  22. hebasto commented at 8:04 am on June 23, 2021: member

    @laanwj

    • renameat is used in QFileSystemEngine::renameFile (inside qt)

    • statx is used in three places (QFileSystemMetaData inside qt)

    Removed in #22321.

  23. hebasto commented at 8:09 am on June 23, 2021: member
    With #22281, #22287/#22305, #22318 and #22321 combined the symbol-check.py passes successfully.
  24. sidhujag referenced this in commit 5c4ff037a8 on Jun 24, 2021
  25. kittywhiskers referenced this in commit 78bf59c78d on Apr 25, 2022
  26. kittywhiskers referenced this in commit 0b1c3bfe7b on Apr 25, 2022
  27. kittywhiskers referenced this in commit 31b0a7c1a5 on Apr 26, 2022
  28. kittywhiskers referenced this in commit c6e724bf21 on Apr 26, 2022
  29. gwillen referenced this in commit 95858637f6 on Jun 1, 2022
  30. DrahtBot locked this on Aug 16, 2022


laanwj achow101 hebasto

Labels
Scripts and tools

Milestone
22.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me