Skip to content

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Jun 14, 2021

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):

bitcoin-cli: symbol getrandom from unsupported version GLIBC_2.25
bitcoin-cli: failed IMPORTED_SYMBOLS
bitcoind: symbol getrandom from unsupported version GLIBC_2.25
bitcoind: symbol log from unsupported version GLIBC_2.29
bitcoind: symbol fcntl64 from unsupported version GLIBC_2.28
bitcoind: symbol pow from unsupported version GLIBC_2.29
bitcoind: symbol exp from unsupported version GLIBC_2.29
bitcoind: failed IMPORTED_SYMBOLS
bitcoin-qt: symbol exp from unsupported version GLIBC_2.29
bitcoin-qt: symbol fcntl64 from unsupported version GLIBC_2.28
bitcoin-qt: symbol log from unsupported version GLIBC_2.29
bitcoin-qt: symbol pow from unsupported version GLIBC_2.29
bitcoin-qt: symbol statx from unsupported version GLIBC_2.28
bitcoin-qt: symbol getrandom from unsupported version GLIBC_2.25
bitcoin-qt: symbol renameat2 from unsupported version GLIBC_2.28
bitcoin-qt: symbol getentropy from unsupported version GLIBC_2.25
bitcoin-qt: failed IMPORTED_SYMBOLS
bitcoin-wallet: symbol exp from unsupported version GLIBC_2.29
bitcoin-wallet: symbol log from unsupported version GLIBC_2.29
bitcoin-wallet: symbol fcntl64 from unsupported version GLIBC_2.28
bitcoin-wallet: failed IMPORTED_SYMBOLS
test_bitcoin: symbol getrandom from unsupported version GLIBC_2.25
test_bitcoin: symbol log from unsupported version GLIBC_2.29
test_bitcoin: symbol fcntl64 from unsupported version GLIBC_2.28
test_bitcoin: symbol pow from unsupported version GLIBC_2.29
test_bitcoin: symbol exp from unsupported version GLIBC_2.29
test_bitcoin: failed IMPORTED_SYMBOLS

laanwj added 2 commits June 14, 2021 20:31
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 bitcoin#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`.
@laanwj
Copy link
Member Author

laanwj commented Jun 14, 2021

  • 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)
$ objdump -R bitcoin-qt|grep fcntl64                                                            
00000000022b25d0 R_X86_64_64       fcntl64@GLIBC_2.28
$ gdb bitcoin-qt.dbg
(gdb) info symbol 0x00000000022b25d0
aSyscall + 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)

@achow101
Copy link
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.

@laanwj
Copy link
Member Author

laanwj commented Jun 15, 2021

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.

@laanwj
Copy link
Member Author

laanwj commented Jun 17, 2021

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…

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK c9b19ed, tested including comparison with master + reverted 634f6ec 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.

@laanwj
Copy link
Member Author

laanwj commented Jun 18, 2021

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?

…bol-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.
@laanwj
Copy link
Member Author

laanwj commented Jun 18, 2021

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.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e8cd370

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!

@hebasto
Copy link
Member

hebasto commented Jun 20, 2021

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:

Checking glibc back compat...
bitcoind: symbol getrandom from unsupported version GLIBC_2.25
bitcoind: failed IMPORTED_SYMBOLS
bitcoin-cli: symbol getrandom from unsupported version GLIBC_2.25
bitcoin-cli: failed IMPORTED_SYMBOLS
test/test_bitcoin: symbol getrandom from unsupported version GLIBC_2.25
test/test_bitcoin: failed IMPORTED_SYMBOLS
qt/bitcoin-qt: symbol statx from unsupported version GLIBC_2.28
qt/bitcoin-qt: symbol getrandom from unsupported version GLIBC_2.25
qt/bitcoin-qt: symbol renameat2 from unsupported version GLIBC_2.28
qt/bitcoin-qt: symbol getentropy from unsupported version GLIBC_2.25
qt/bitcoin-qt: failed IMPORTED_SYMBOLS

For statx and renameat2 I opened a dedicated #22280.

@hebasto
Copy link
Member

hebasto commented Jun 23, 2021

@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.

@hebasto
Copy link
Member

hebasto commented Jun 23, 2021

@laanwj

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

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

Removed in #22321.

@hebasto
Copy link
Member

hebasto commented Jun 23, 2021

With #22281, #22287/#22305, #22318 and #22321 combined the symbol-check.py passes successfully.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 24, 2021
…ymbol-check

e8cd370 devtools: Integrate ARCH_MIN_GLIBC_VER table into MAX_VERSIONS in symbol-check.py (W. J. van der Laan)
a33381a devtools: Add xkb version to symbol-check (W. J. van der Laan)
19e598b devtools: Fix verneed section parsing in pixie (W. J. van der Laan)

Pull request description:

  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 bitcoin#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):

  ```
  bitcoin-cli: symbol getrandom from unsupported version GLIBC_2.25
  bitcoin-cli: failed IMPORTED_SYMBOLS
  bitcoind: symbol getrandom from unsupported version GLIBC_2.25
  bitcoind: symbol log from unsupported version GLIBC_2.29
  bitcoind: symbol fcntl64 from unsupported version GLIBC_2.28
  bitcoind: symbol pow from unsupported version GLIBC_2.29
  bitcoind: symbol exp from unsupported version GLIBC_2.29
  bitcoind: failed IMPORTED_SYMBOLS
  bitcoin-qt: symbol exp from unsupported version GLIBC_2.29
  bitcoin-qt: symbol fcntl64 from unsupported version GLIBC_2.28
  bitcoin-qt: symbol log from unsupported version GLIBC_2.29
  bitcoin-qt: symbol pow from unsupported version GLIBC_2.29
  bitcoin-qt: symbol statx from unsupported version GLIBC_2.28
  bitcoin-qt: symbol getrandom from unsupported version GLIBC_2.25
  bitcoin-qt: symbol renameat2 from unsupported version GLIBC_2.28
  bitcoin-qt: symbol getentropy from unsupported version GLIBC_2.25
  bitcoin-qt: failed IMPORTED_SYMBOLS
  bitcoin-wallet: symbol exp from unsupported version GLIBC_2.29
  bitcoin-wallet: symbol log from unsupported version GLIBC_2.29
  bitcoin-wallet: symbol fcntl64 from unsupported version GLIBC_2.28
  bitcoin-wallet: failed IMPORTED_SYMBOLS
  test_bitcoin: symbol getrandom from unsupported version GLIBC_2.25
  test_bitcoin: symbol log from unsupported version GLIBC_2.29
  test_bitcoin: symbol fcntl64 from unsupported version GLIBC_2.28
  test_bitcoin: symbol pow from unsupported version GLIBC_2.29
  test_bitcoin: symbol exp from unsupported version GLIBC_2.29
  test_bitcoin: failed IMPORTED_SYMBOLS
  ```

ACKs for top commit:
  hebasto:
    ACK e8cd370

Tree-SHA512: 8c15e3478eb642f01a1ddaadef03f80583f088f9fa8e3bf171ce16b0ec05ffb4675ec147d7ffc6a4360637ed47fca517c6ca2bac7bb30d794c03783cfb964b79
kwvg pushed a commit to kwvg/dash that referenced this pull request Apr 25, 2022
kwvg pushed a commit to kwvg/dash that referenced this pull request Apr 25, 2022
kwvg pushed a commit to kwvg/dash that referenced this pull request Apr 26, 2022
kwvg pushed a commit to kwvg/dash that referenced this pull request Apr 26, 2022
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants