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

Do not export `secp256k1_musig_nonce_gen_internal` e2571385e0

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.

Introduce `SECP256K1_LOCAL_VAR` macro

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

275ec533de

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.

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.

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

ci: Run `tools/symbol-check.py` 759a4818c1

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

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.

hebasto force-pushed on Oct 24, 2024

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

@fanquake’s feedback has been addressed.

Fix symbol visibility issues, add test for it #1359