Fix symbol visibility issues, add test for it

[hebasto]

Closes #1181.

Catches the actual symbol visibility issue.

Replaces #1135.

[hebasto]

Rebased on top of the merged #1356.

[hebasto]

Rebased f816729 -> d7b1cea (pr1359.02 -> pr1359.03) due to the conflict with #1313.

[hebasto]

@maflcko

Is it possible for a Cirrus persistent worker to do not override environment variables set with the ENV command in the Dockerfile?

[maflcko]

Maybe --env-file drops ENV?

[hebasto]

Rebased to resolve conflicts.

[hebasto]

#1595 (comment):

This reminds me that we should move forward with #1359.

Agreed.

Rebased and ready to move forward :)

[hebasto]

Cool that this caught a real (new) symbol leak.

To clarify, the fix is in the first commit. The excerpt from the CI log with an error message from the previous iteration of this branch looks as follows:

+ python3 ./tools/symbol-check.py .libs/libsecp256k1.so
.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
.libs/libsecp256k1.so: failed EXPORTED_SYMBOLS
Error: Process completed with exit code 1.

@fanquake suggested that the test could potentially be simplified for c-i.

Thanks! The tools/symbol-check.py script has been simplified.

[theuni]

Code review ACK 44a9392. No opinion on the tests/ci.

[real-or-random]

I think this is small enough to keep it in util.h. Or is there a reason not to keep it here?

[hebasto]

The reason is to facilitate the further attempts to make all headers self-contained.

[real-or-random]

I guess I'm just missing it. Can you explain how this will help with #1039?

[hebasto]

My intention was to avoid bringing the high-level ../include/secp256k1.h header into lower-level code via util.h. However, at this point, util.h is already being included indirectly through other headers.

[theuni]

No opinion here, I defer to @real-or-random for preference.

[hebasto]

@real-or-random's comments have been addressed.

[fanquake]

What are the calls to .concrete for?

[hebasto]

Dropped.

[fanquake]

I don't think this code can be reached, because if it is an unknown format, .parse() will have errored (printing Unknown format), and then lief.Binary.FORMATS = library.format will fail:

./tools/symbol-check.py not_a_file
Unknown format
Traceback (most recent call last):
  File "./tools/symbol-check.py", line 81, in <module>
    exe_format: lief.Binary.FORMATS = library.format
                                      ^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'format'

[hebasto]

Thanks! Reworked.

[real-or-random]

I don't think this code can be reached,

This will be reachable once LIEF adds support for a new binary format, so I think it was useful to have it.

[hebasto]

@fanquake's feedback has been addressed.

[real-or-random]

This script can still be simplified. Let me suggest a refactored version later.

[real-or-random]

After reading the docs, I believe that .exported_functions is from the abstract Binary class, and [entry.name for entry in library.get_export()] may give the better result for PE because it includes non-function exports? See https://lief.re/doc/latest/formats/pe/python.html#export-entry

[hebasto]

The suggested approach returns the same list of symbols on the master branch. Incorporated as a potentially superior one.

[real-or-random]

After reading the docs, I believe that .exported_functions is from the abstract Binary class, and .exported_symbols may give a better result for Mach-O because it includes non-function exports?

[hebasto]

The suggested approach returns the same list of symbols on the master branch. Incorporated as a potentially superior one.

[real-or-random]

Hm, just checking: You're saying that this change didn't make a difference, right? So it still finds only exported functions and not exported variables?

(And I have the same question for PE.)

[hebasto]

Hm, just checking: You're saying that this change didn't make a difference, right?

Correct.

So it still finds only exported functions and not exported variables?

With the reverted second commit, both variants catch the same local variables:

./tools/symbol-check.py: In .libs/libsecp256k1.dylib: Unexpected exported symbols: {'secp256k1_pre_g', 'secp256k1_pre_g_128', 'secp256k1_ecmult_gen_prec_table'}

(And I have the same question for PE.)

With the reverted second commit, both variants do not catch local variables.

For more details, please refer to:

• https://github.com/hebasto/secp256k1/actions/runs/11640943476
• https://github.com/hebasto/secp256k1/actions/runs/11640955643

[real-or-random]

Ok, that means that the LIEF API is a bit unexpected. Or we don't understand it.

• For Mach-O, exported_functions is the same as exported_symbols. Both include variables, but I'd expect the former to include only functions.
• For PE, there doesn't seem to be a way to find exported variables. By the way, importing variables is rare, but the ellswift example and benchmark import the variable secp256k1_ellswift_xdh_hash_function_bip324, so at least we test this in CI. And thus we can be sure that the variables are actually exported in PE, and it's just LIEF that doesn't show them.

Does this match your understanding? Do you think it's worth asking LIEF about the variables?

Relatedly, on platforms where LIEF gives us both variables and functions, would it make sense also check that all expected symbols are actually exported?

[real-or-random]

Nevermind, my analysis of variables is wrong. LIEF shows them. I'll revisit this later.

[real-or-random]

Here's a refactored version, which is more readable IMHO (haven't tested on Mac or Windows):

#!/usr/bin/env python3
"""Check that a libsecp256k1 shared library exports only expected symbols.

Usage examples:
  - When building with Autotools:
    ./tools/symbol-check.py .libs/libsecp256k1.so
    ./tools/symbol-check.py .libs/libsecp256k1-<V>.dll
    ./tools/symbol-check.py .libs/libsecp256k1.dylib

  - When building with CMake:
    ./tools/symbol-check.py build/lib/libsecp256k1.so
    ./tools/symbol-check.py build/bin/libsecp256k1-<V>.dll
    ./tools/symbol-check.py build/lib/libsecp256k1.dylib"""

import re
import sys
import subprocess

import lief


class UnexpectedExport(RuntimeError):
    pass


def get_exported_exports(library) -> list[str]:
    """Adapter function to get exported symbols based on the library format."""
    if library.format == lief.Binary.FORMATS.ELF:
        return [symbol.name for symbol in library.exported_symbols]
    elif library.format == lief.Binary.FORMATS.PE:
        return [function.name for function in library.exported_functions]
    elif library.format == lief.Binary.FORMATS.MACHO:
        return [function.name[1:] for function in library.exported_functions]
    raise NotImplementedError(f"Unsupported format: {library.format}")


def grep_expected_symbols() -> list[str]:
    """Guess the list of expected exported symbols from the source code."""
    grep_output = subprocess.check_output(
        ["git", "grep", "^SECP256K1_API", "--", "include"], # TODO WHITESPACE
        universal_newlines=True,
        encoding="utf-8"
    )
    lines = grep_output.split("\n")
    pattern = re.compile(r'\bsecp256k1_\w+')
    exported: list[str] = [pattern.findall(line)[-1] for line in lines if line.strip()]
    return exported


def check_symbols(library, expected_exports) -> None:
    """Check that the library exports only the expected symbols."""
    actual_exports = list(get_exported_exports(library))
    unexpected_exports = set(actual_exports) - set(expected_exports)
    if unexpected_exports != set():
        raise UnexpectedExport(f"Unexpected exported symbols: {unexpected_exports}")

def main():
    if len(sys.argv) != 2:
        print(__doc__)
        return 1
    library = lief.parse(sys.argv[1])
    expected_exports = grep_expected_symbols()
    try:
        check_symbols(library, expected_exports)
    except UnexpectedExport as e:
        print(f"{sys.argv[0]}: In {sys.argv[1]}: {e}")
        return 1
    return 0


if __name__ == "__main__":
    sys.exit(main())

[hebasto]

Here's a refactored version, which is more readable IMHO (haven't tested on Mac or Windows):

Tested on macOS and on Windows. Incorporated. Thank you!

[jonasnick]

This gives me error symbol-check.backup.py:40: SyntaxWarning: invalid escape sequence '\s'. I think it should be

r"^\s*SECP256K1_API"

or

"^\\s*SECP256K1_API"

[hebasto]

Rebased and addressed @jonasnick's #1359 (comment).

[theuni]

Code-review reACK 7ac46cf.

Still no opinion on the .py/ci.

Being able to drop -fvisibility=hidden is a nice improvement (and also a testament to the code structure!)