Fix symbol visibility issues, add test for it

hebasto commented at 12:16 pm on June 26, 2023: member

Closes #1181.

Catches the actual symbol visibility issue.

Replaces #1135.

real-or-random added the label build on Jun 26, 2023

real-or-random added the label assurance on Jun 26, 2023

hebasto force-pushed on Jun 27, 2023

hebasto commented at 7:45 am on June 27, 2023: member

Rebased on top of the merged #1356.

hebasto force-pushed on Jul 13, 2023

hebasto commented at 3:56 pm on July 13, 2023: member

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

hebasto force-pushed on Jul 13, 2023

hebasto marked this as a draft on Jul 13, 2023

hebasto force-pushed on Jul 13, 2023

hebasto marked this as ready for review on Jul 13, 2023

hebasto marked this as a draft on Aug 31, 2023

hebasto force-pushed on Feb 12, 2024

hebasto marked this as ready for review on Feb 12, 2024

hebasto commented at 9:14 pm on February 12, 2024: member

@maflcko

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

maflcko commented at 8:56 am on February 13, 2024: contributor

Maybe --env-file drops ENV?

hebasto force-pushed on Feb 13, 2024

hebasto force-pushed on Apr 23, 2024

hebasto commented at 1:01 pm on April 23, 2024: member

Rebased to resolve conflicts.

hebasto force-pushed on Aug 25, 2024

hebasto commented at 4:22 pm on August 25, 2024: member

#1595#issue-2484933271:

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

Agreed.

Rebased and ready to move forward :)

hebasto force-pushed on Oct 16, 2024

hebasto marked this as a draft on Oct 16, 2024

hebasto force-pushed on Oct 16, 2024

hebasto marked this as ready for review on Oct 16, 2024

hebasto commented at 2:34 pm on October 16, 2024: member

Rebased.

in src/util_defs.h:7 in 3144ba99f3 outdated

0@@ -0,0 +1,12 @@
1+#ifndef SECP256K1_UTIL_DEFS_H
2+#define SECP256K1_UTIL_DEFS_H
3+
4+/* Global variable visibility */
5+/* See: https://github.com/bitcoin-core/secp256k1/issues/1181 */
6+#if !defined(_WIN32) && defined(__GNUC__) && (__GNUC__ >= 4)
7+# define SECP256K1_LOCAL_VAR extern __attribute__ ((visibility ("hidden")))

theuni commented at 2:56 pm on October 16, 2024:

The combo of extern + hidden isn’t completely intuitive, but the gcc visibility guide confirms that it makes sense.

in src/util_defs.h:2 in 3144ba99f3 outdated

0@@ -0,0 +1,12 @@
1+#ifndef SECP256K1_UTIL_DEFS_H
2+#define SECP256K1_UTIL_DEFS_H

theuni commented at 3:01 pm on October 16, 2024:

Nit: util_defs.h is pretty ambiguous. How about util_local_visibility.h or so?

hebasto commented at 8:04 pm on October 16, 2024:

Thanks! Reworked.

in CMakeLists.txt:270 in 4f64b9f4cd outdated

266@@ -267,8 +267,6 @@ else()
267   try_append_c_flags(-Wundef)
268 endif()
269 
270-set(CMAKE_C_VISIBILITY_PRESET hidden)

theuni commented at 3:04 pm on October 16, 2024:

In case it’s not obvious to other reviewers, this is no longer needed because every non-static function should be externally visible.

If we ever need to share private symbols between compilation units this won’t work. But I agree that it makes sense to take advantage of that if we can.

theuni approved

theuni commented at 3:06 pm on October 16, 2024: contributor

Code review and approach ACK.

Cool that this caught a real (new) symbol leak. @fanquake suggested that the test could potentially be simplified for c-i.

hebasto force-pushed on Oct 16, 2024

hebasto commented at 8:31 pm on October 16, 2024: member

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:

0+ python3 ./tools/symbol-check.py .libs/libsecp256k1.so
1.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
2.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
3.libs/libsecp256k1.so: failed EXPORTED_SYMBOLS
4Error: 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.

real-or-random added this to the milestone 0.6.0 on Oct 21, 2024

real-or-random referenced this in commit f0868a9b3d on Oct 21, 2024

theuni approved

theuni commented at 3:51 pm on October 22, 2024: contributor

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

in .github/workflows/ci.yml:786 in 8a5c84a8d3 outdated

781@@ -763,6 +782,12 @@ jobs:
782         run: |
783           cd build/bin/RelWithDebInfo && file *tests.exe bench*.exe libsecp256k1-*.dll || true
784 
785+      - name: Symbol check
786+        if: ${{ matrix.configuration.cmake_options != '-A x64 -DBUILD_SHARED_LIBS=OFF' }}

real-or-random commented at 8:47 pm on October 22, 2024:

nit: Checking for string inequality seems a bit fragile if we modify the builds in the future. Perhaps it’s cleaner to introduce a new variable symbol_check in addition to cmake_options? Or alternatively, a variable shared that controls -DBUILD_SHARED_LIBS.

hebasto commented at 10:08 am on October 24, 2024:

Thanks! Reworked.

in tools/symbol-check.py:3 in 44a9392f8b outdated

0@@ -0,0 +1,98 @@
1+#!/usr/bin/env python3
2+'''
3+A script to check that a secp256k1 shared library

real-or-random commented at 8:56 pm on October 22, 2024:

0A script to check that a libsecp256k1 shared library

:P

hebasto commented at 10:08 am on October 24, 2024:

Thanks! Amended.

in ci/ci.sh:116 in 44a9392f8b outdated

106@@ -107,6 +107,19 @@ file *tests* || true
107 file bench* || true
108 file .libs/* || true
109 
110+if [ "$SYMBOL_CHECK" = "yes" ]
111+then
112+    case "$HOST" in
113+        *mingw*)
114+            ls -l .libs
115+            python3 ./tools/symbol-check.py .libs/libsecp256k1-2.dll

real-or-random commented at 8:59 pm on October 22, 2024:

Now that we invoke python3 here, I think it makes sense to add a python3 --version at the top of the file, as we done for the other tools.

hebasto commented at 10:08 am on October 24, 2024:

Thanks! Reworked.

in tools/symbol-check.py:27 in 44a9392f8b outdated

22+    ./tools/symbol-check.py build/lib/libsecp256k1.dylib
23+'''
24+import re
25+import sys
26+import subprocess
27+from typing import List

real-or-random commented at 9:02 pm on October 22, 2024:

hint: List[T] has been deprecated since Python 3.9, which is the oldest supported Python version. So you can safely use list[T], and this import is not needed. See https://docs.python.org/3.9/library/typing.html?highlight=typing#typing.List (Kudos for adding types!)

hebasto commented at 10:09 am on October 24, 2024:

Thanks! Done.

in src/precomputed_ecmult.h:34 in 44a9392f8b outdated

30 #    define WINDOW_G ECMULT_WINDOW_SIZE
31-extern const secp256k1_ge_storage secp256k1_pre_g[ECMULT_TABLE_SIZE(WINDOW_G)];
32-extern const secp256k1_ge_storage secp256k1_pre_g_128[ECMULT_TABLE_SIZE(WINDOW_G)];
33+SECP256K1_LOCAL_VAR const secp256k1_ge_storage secp256k1_pre_g[ECMULT_TABLE_SIZE(WINDOW_G)];
34+SECP256K1_LOCAL_VAR const secp256k1_ge_storage secp256k1_pre_g_128[ECMULT_TABLE_SIZE(WINDOW_G)];
35 #endif /* defined(EXHAUSTIVE_TEST_ORDER) */

real-or-random commented at 10:48 am on October 23, 2024:

Is there a specific reason to keep the #include in a specific branch (instead of putting it at the top of file)?

hebasto commented at 10:09 am on October 24, 2024:

Nope. Reworked.

in tools/symbol-check.py:33 in 44a9392f8b outdated

28+
29+import lief
30+
31+
32+def grep_exported_symbols() -> List[str]:
33+    grep_output = subprocess.check_output(["git", "grep", "^SECP256K1_API", "--", "include"], universal_newlines=True, encoding="utf8")

real-or-random commented at 10:51 am on October 23, 2024:

0    grep_output = subprocess.check_output(["git", "grep", "^[\s*]SECP256K1_API", "--", "include"], universal_newlines=True, encoding="utf8")

That’s perhaps a bit more robust

hebasto commented at 10:09 am on October 24, 2024:

Thanks! Reworked.

in src/util_local_visibility.h:1 in 44a9392f8b

real-or-random commented at 10:53 am on October 23, 2024:

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

hebasto commented at 10:01 am on October 24, 2024:

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

real-or-random commented at 10:19 pm on November 1, 2024:

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

hebasto commented at 8:00 am on November 2, 2024:

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 commented at 4:46 pm on November 12, 2024:

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

hebasto force-pushed on Oct 24, 2024

hebasto commented at 10:07 am on October 24, 2024: member

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

in tools/symbol-check.py:71 in 8ee12da84c outdated

66+    return ok
67+
68+
69+def check_MACHO_exported_functions(library, expected_exports) -> bool:
70+    ok: bool = True
71+    macho_lib: lief.MACHO.Binary = library.concrete

fanquake commented at 10:18 am on October 24, 2024:

What are the calls to .concrete for?

hebasto commented at 11:39 am on October 24, 2024:

Dropped.

in tools/symbol-check.py:93 in 8ee12da84c outdated

88+    elif exe_format == lief.Binary.FORMATS.PE:
89+        success = check_PE_exported_functions(library, grep_exported_symbols())
90+    elif exe_format == lief.Binary.FORMATS.MACHO:
91+        success = check_MACHO_exported_functions(library, grep_exported_symbols())
92+    else:
93+        print(f'{filename}: unknown executable format')

fanquake commented at 10:29 am on October 24, 2024:

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:

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

hebasto commented at 11:38 am on October 24, 2024:

Thanks! Reworked.

real-or-random commented at 5:27 pm on November 1, 2024:

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 force-pushed on Oct 24, 2024

hebasto commented at 11:38 am on October 24, 2024: member

@fanquake’s feedback has been addressed.

in tools/symbol-check.py:56 in b80743a40f outdated

51+    return ok
52+
53+
54+def check_PE_exported_functions(library, expected_exports) -> bool:
55+    ok: bool = True
56+    for function in library.exported_functions:

real-or-random commented at 4:54 pm on November 1, 2024:

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 commented at 8:32 pm on November 1, 2024:

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

in tools/symbol-check.py:67 in b80743a40f outdated

62+    return ok
63+
64+
65+def check_MACHO_exported_functions(library, expected_exports) -> bool:
66+    ok: bool = True
67+    for function in library.exported_functions:

real-or-random commented at 4:55 pm on November 1, 2024:

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 commented at 8:32 pm on November 1, 2024:

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

real-or-random commented at 10:14 pm on November 1, 2024:

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 commented at 7:40 am on November 2, 2024:

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:

0./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:

real-or-random commented at 10:52 am on November 4, 2024:

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 commented at 11:00 am on November 4, 2024:

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

real-or-random commented at 5:26 pm on November 1, 2024: contributor

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

real-or-random commented at 5:49 pm on November 1, 2024: contributor

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

 0#!/usr/bin/env python3
 1"""Check that a libsecp256k1 shared library exports only expected symbols.
 2
 3Usage examples:
 4  - When building with Autotools:
 5    ./tools/symbol-check.py .libs/libsecp256k1.so
 6    ./tools/symbol-check.py .libs/libsecp256k1-<V>.dll
 7    ./tools/symbol-check.py .libs/libsecp256k1.dylib
 8
 9  - When building with CMake:
10    ./tools/symbol-check.py build/lib/libsecp256k1.so
11    ./tools/symbol-check.py build/bin/libsecp256k1-<V>.dll
12    ./tools/symbol-check.py build/lib/libsecp256k1.dylib"""
13
14import re
15import sys
16import subprocess
17
18import lief
19
20
21class UnexpectedExport(RuntimeError):
22    pass
23
24
25def get_exported_exports(library) -> list[str]:
26    """Adapter function to get exported symbols based on the library format."""
27    if library.format == lief.Binary.FORMATS.ELF:
28        return [symbol.name for symbol in library.exported_symbols]
29    elif library.format == lief.Binary.FORMATS.PE:
30        return [function.name for function in library.exported_functions]
31    elif library.format == lief.Binary.FORMATS.MACHO:
32        return [function.name[1:] for function in library.exported_functions]
33    raise NotImplementedError(f"Unsupported format: {library.format}")
34
35
36def grep_expected_symbols() -> list[str]:
37    """Guess the list of expected exported symbols from the source code."""
38    grep_output = subprocess.check_output(
39        ["git", "grep", "^SECP256K1_API", "--", "include"], # TODO WHITESPACE
40        universal_newlines=True,
41        encoding="utf-8"
42    )
43    lines = grep_output.split("\n")
44    pattern = re.compile(r'\bsecp256k1_\w+')
45    exported: list[str] = [pattern.findall(line)[-1] for line in lines if line.strip()]
46    return exported
47
48
49def check_symbols(library, expected_exports) -> None:
50    """Check that the library exports only the expected symbols."""
51    actual_exports = list(get_exported_exports(library))
52    unexpected_exports = set(actual_exports) - set(expected_exports)
53    if unexpected_exports != set():
54        raise UnexpectedExport(f"Unexpected exported symbols: {unexpected_exports}")
55
56def main():
57    if len(sys.argv) != 2:
58        print(__doc__)
59        return 1
60    library = lief.parse(sys.argv[1])
61    expected_exports = grep_expected_symbols()
62    try:
63        check_symbols(library, expected_exports)
64    except UnexpectedExport as e:
65        print(f"{sys.argv[0]}: In {sys.argv[1]}: {e}")
66        return 1
67    return 0
68
69
70if __name__ == "__main__":
71    sys.exit(main())

hebasto force-pushed on Nov 1, 2024

hebasto commented at 8:33 pm on November 1, 2024: member

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!

in tools/symbol-check.py:40 in 0c9a046203 outdated

35+
36+
37+def grep_expected_symbols() -> list[str]:
38+    """Guess the list of expected exported symbols from the source code."""
39+    grep_output = subprocess.check_output(
40+        ["git", "grep", "^SECP256K1_API", "--", "include"], # TODO WHITESPACE

real-or-random commented at 10:17 pm on November 1, 2024:

0        ["git", "grep", "^\s*SECP256K1_API", "--", "include"],

Oops, I think I had this in mind here, to make the matching a bit more flexible.

hebasto commented at 8:07 am on November 2, 2024:

Thanks! Done.

hebasto force-pushed on Nov 2, 2024

in tools/symbol-check.py:40 in fb5d25dd6a outdated

35+
36+
37+def grep_expected_symbols() -> list[str]:
38+    """Guess the list of expected exported symbols from the source code."""
39+    grep_output = subprocess.check_output(
40+        ["git", "grep", "^\s*SECP256K1_API", "--", "include"],

jonasnick commented at 1:36 pm on November 4, 2024:

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

0r"^\s*SECP256K1_API"

or

0"^\\s*SECP256K1_API"

real-or-random removed this from the milestone 0.6.0 on Nov 4, 2024

real-or-random added this to the milestone 0.6.1 on Nov 4, 2024

Introduce `SECP256K1_LOCAL_VAR` macro

This change makes the `-fvisibility=hidden` compiler option unnecessary.

9ef06c8b86

test: Add `tools/symbol-check.py`

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

aad2b79d79

hebasto force-pushed on Nov 4, 2024

hebasto commented at 5:49 pm on November 4, 2024: member

Rebased and addressed @jonasnick’s #1359 (review).

ci: Run `tools/symbol-check.py` 0b1a0df7e9

build: Drop no longer needed `-fvisibility=hidden` compiler option 7ac46cf93c

hebasto force-pushed on Nov 4, 2024

theuni approved

theuni commented at 4:49 pm on November 12, 2024: contributor

Code-review reACK 7ac46cf93c322193e428effeac3f024f4ab6a05b.

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

Fix symbol visibility issues, add test for it #1359