Merge bitcoin-core/secp256k1#1359: Fix symbol visibility issues, add test for it

d1478763a5 build: Drop no longer needed  `-fvisibility=hidden` compiler option (Hennadii Stepanov)
8ed1d83d92 ci: Run `tools/symbol-check.py` (Hennadii Stepanov)
41d32ab2de test: Add `tools/symbol-check.py` (Hennadii Stepanov)
88548058b3 Introduce `SECP256K1_LOCAL_VAR` macro (Hennadii Stepanov)

Pull request description:

  Closes https://github.com/bitcoin-core/secp256k1/issues/1181.

  [Catches](https://github.com/bitcoin-core/secp256k1/pull/1359#issuecomment-2417892235) the actual symbol visibility issue.

  Replaces https://github.com/bitcoin-core/secp256k1/pull/1135.

ACKs for top commit:
  real-or-random:
    reACK d1478763a5

Tree-SHA512: 4d39f3c4cd32afa2b4418ca79331c72827c76a49a5422afa7c85e60d00a750b91b1b1ab10d91ba578f5817dd938016751168758461fb89de8da56f7d005ae2da

.cirrus.yml

@@ -29,6 +29,8 @@ env:
   BENCH: yes
   SECP256K1_BENCH_ITERS: 2
   CTIMETESTS: yes
+  SYMBOL_CHECK: yes
+  VIRTUAL_ENV: /root/venv
   # Compile and run the tests
   EXAMPLES: yes
 
@@ -53,6 +55,7 @@ cat_logs_snippet: &CAT_LOGS
 
 linux_arm64_container_snippet: &LINUX_ARM64_CONTAINER
   env_script:
+    - export PATH="$VIRTUAL_ENV/bin:$PATH"
     - env | tee /tmp/env
   build_script:
     - DOCKER_BUILDKIT=1 docker build --file "ci/linux-debian.Dockerfile" --tag="ci_secp256k1_arm"

.github/workflows/ci.yml

@@ -40,6 +40,7 @@ env:
   BENCH: 'yes'
   SECP256K1_BENCH_ITERS: 2
   CTIMETESTS: 'yes'
+  SYMBOL_CHECK: 'yes'
   # Compile and run the examples.
   EXAMPLES: 'yes'
 
@@ -365,6 +366,7 @@ jobs:
       ASAN_OPTIONS: 'strict_string_checks=1:detect_stack_use_after_return=1:detect_leaks=1'
       LSAN_OPTIONS: 'use_unaligned=1'
       SECP256K1_TEST_ITERS: 32
+      SYMBOL_CHECK: 'no'
 
     steps:
       - name: Checkout
@@ -415,6 +417,7 @@ jobs:
       SECP256K1_TEST_ITERS: 32
       ASM: 'no'
       WITH_VALGRIND: 'no'
+      SYMBOL_CHECK: 'no'
 
     steps:
       - name: Checkout
@@ -483,6 +486,7 @@ jobs:
       CC: 'clang'
       HOMEBREW_NO_AUTO_UPDATE: 1
       HOMEBREW_NO_INSTALL_CLEANUP: 1
+      SYMBOL_CHECK: 'no'
 
     strategy:
       fail-fast: false
@@ -515,6 +519,12 @@ jobs:
         env: ${{ matrix.env_vars }}
         run: ./ci/ci.sh
 
+      - name: Symbol check
+        run: |
+          python3 --version
+          python3 -m pip install lief
+          python3 ./tools/symbol-check.py .libs/libsecp256k1.dylib
+
       - name: Print logs
         uses: ./.github/actions/print-logs
         if: ${{ !cancelled() }}
@@ -530,6 +540,7 @@ jobs:
       HOMEBREW_NO_INSTALL_CLEANUP: 1
       WITH_VALGRIND: 'no'
       CTIMETESTS: 'no'
+      SYMBOL_CHECK: 'no'
 
     strategy:
       fail-fast: false
@@ -557,6 +568,16 @@ jobs:
         env: ${{ matrix.env_vars }}
         run: ./ci/ci.sh
 
+      - name: Symbol check
+        env:
+          VIRTUAL_ENV: '${{ github.workspace }}/venv'
+        run: |
+          python3 --version
+          python3 -m venv $VIRTUAL_ENV
+          export PATH="$VIRTUAL_ENV/bin:$PATH"
+          python3 -m pip install lief
+          python3 ./tools/symbol-check.py .libs/libsecp256k1.dylib
+
       - name: Print logs
         uses: ./.github/actions/print-logs
         if: ${{ !cancelled() }}
@@ -573,6 +594,7 @@ jobs:
         configuration:
           - job_name: 'x64 (MSVC): Windows (VS 2022, shared)'
             cmake_options: '-A x64 -DBUILD_SHARED_LIBS=ON'
+            symbol_check: 'true'
           - job_name: 'x64 (MSVC): Windows (VS 2022, static)'
             cmake_options: '-A x64 -DBUILD_SHARED_LIBS=OFF'
           - job_name: 'x64 (MSVC): Windows (VS 2022, int128_struct)'
@@ -601,6 +623,13 @@ jobs:
         run: |
           cd build/bin/RelWithDebInfo && file *tests.exe bench*.exe libsecp256k1-*.dll || true
 
+      - name: Symbol check
+        if: ${{ matrix.configuration.symbol_check }}
+        run: |
+          py -3 --version
+          py -3 -m pip install lief
+          py -3 .\tools\symbol-check.py build\bin\RelWithDebInfo\libsecp256k1-5.dll
+
       - name: Check
         run: |
           ctest -C RelWithDebInfo --test-dir build -j ([int]$env:NUMBER_OF_PROCESSORS + 1)

CMakeLists.txt

@@ -271,8 +271,6 @@ else()
   try_append_c_flags(-Wundef)
 endif()
 
-set(CMAKE_C_VISIBILITY_PRESET hidden)
-
 set(print_msan_notice)
 if(SECP256K1_BUILD_CTIME_TESTS)
   include(CheckMemorySanitizer)

Makefile.am

@@ -47,6 +47,7 @@ noinst_HEADERS += src/assumptions.h
 noinst_HEADERS += src/checkmem.h
 noinst_HEADERS += src/testutil.h
 noinst_HEADERS += src/util.h
+noinst_HEADERS += src/util_local_visibility.h
 noinst_HEADERS += src/int128.h
 noinst_HEADERS += src/int128_impl.h
 noinst_HEADERS += src/int128_native.h

ci/ci.sh

@@ -14,7 +14,7 @@ print_environment() {
     for var in WERROR_CFLAGS MAKEFLAGS BUILD \
             ECMULTWINDOW ECMULTGENKB ASM WIDEMUL WITH_VALGRIND EXTRAFLAGS \
             EXPERIMENTAL ECDH RECOVERY EXTRAKEYS MUSIG SCHNORRSIG ELLSWIFT \
-            SECP256K1_TEST_ITERS BENCH SECP256K1_BENCH_ITERS CTIMETESTS\
+            SECP256K1_TEST_ITERS BENCH SECP256K1_BENCH_ITERS CTIMETESTS SYMBOL_CHECK \
             EXAMPLES \
             HOST WRAPPER_CMD \
             CC CFLAGS CPPFLAGS AR NM \
@@ -107,6 +107,20 @@ file *tests* || true
 file bench* || true
 file .libs/* || true
 
+if [ "$SYMBOL_CHECK" = "yes" ]
+then
+    python3 --version
+    case "$HOST" in
+        *mingw*)
+            ls -l .libs
+            python3 ./tools/symbol-check.py .libs/libsecp256k1-5.dll
+            ;;
+        *)
+            python3 ./tools/symbol-check.py .libs/libsecp256k1.so
+            ;;
+    esac
+fi
+
 # This tells `make check` to wrap test invocations.
 export LOG_COMPILER="$WRAPPER_CMD"
 

ci/linux-debian.Dockerfile

@@ -32,7 +32,7 @@ RUN apt-get update && apt-get install --no-install-recommends -y \
         gcc-powerpc64le-linux-gnu libc6-dev-ppc64el-cross libc6-dbg:ppc64el \
         gcc-mingw-w64-x86-64-win32 wine64 wine \
         gcc-mingw-w64-i686-win32 wine32 \
-        python3 && \
+        python3-full && \
         if ! ( dpkg --print-architecture | grep --quiet "arm64" ) ; then \
          apt-get install --no-install-recommends -y \
          gcc-aarch64-linux-gnu libc6-dev-arm64-cross libc6-dbg:arm64 ;\
@@ -77,3 +77,7 @@ RUN \
     apt-get autoremove -y wget && \
     apt-get clean && rm -rf /var/lib/apt/lists/*
 
+ENV VIRTUAL_ENV=/root/venv
+RUN python3 -m venv $VIRTUAL_ENV
+ENV PATH="$VIRTUAL_ENV/bin:$PATH"
+RUN pip install lief

configure.ac

@@ -111,7 +111,6 @@ AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [
       SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0
       SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only
       SECP_TRY_APPEND_CFLAGS([-Wreserved-identifier], $1) # Clang >= 13.0 only
-      SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0
 
       CFLAGS="$SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS"
     fi

src/precomputed_ecmult.h

@@ -13,6 +13,8 @@ extern "C" {
 
 #include "ecmult.h"
 #include "group.h"
+#include "util_local_visibility.h"
+
 #if defined(EXHAUSTIVE_TEST_ORDER)
 #    if EXHAUSTIVE_TEST_ORDER == 7
 #        define WINDOW_G 3
@@ -27,8 +29,8 @@ static secp256k1_ge_storage secp256k1_pre_g[ECMULT_TABLE_SIZE(WINDOW_G)];
 static secp256k1_ge_storage secp256k1_pre_g_128[ECMULT_TABLE_SIZE(WINDOW_G)];
 #else /* !defined(EXHAUSTIVE_TEST_ORDER) */
 #    define WINDOW_G ECMULT_WINDOW_SIZE
-extern const secp256k1_ge_storage secp256k1_pre_g[ECMULT_TABLE_SIZE(WINDOW_G)];
-extern const secp256k1_ge_storage secp256k1_pre_g_128[ECMULT_TABLE_SIZE(WINDOW_G)];
+SECP256K1_LOCAL_VAR const secp256k1_ge_storage secp256k1_pre_g[ECMULT_TABLE_SIZE(WINDOW_G)];
+SECP256K1_LOCAL_VAR const secp256k1_ge_storage secp256k1_pre_g_128[ECMULT_TABLE_SIZE(WINDOW_G)];
 #endif /* defined(EXHAUSTIVE_TEST_ORDER) */
 
 #ifdef __cplusplus

src/precomputed_ecmult_gen.h

@@ -13,10 +13,12 @@ extern "C" {
 
 #include "group.h"
 #include "ecmult_gen.h"
+#include "util_local_visibility.h"
+
 #ifdef EXHAUSTIVE_TEST_ORDER
 static secp256k1_ge_storage secp256k1_ecmult_gen_prec_table[COMB_BLOCKS][COMB_POINTS];
 #else
-extern const secp256k1_ge_storage secp256k1_ecmult_gen_prec_table[COMB_BLOCKS][COMB_POINTS];
+SECP256K1_LOCAL_VAR const secp256k1_ge_storage secp256k1_ecmult_gen_prec_table[COMB_BLOCKS][COMB_POINTS];
 #endif /* defined(EXHAUSTIVE_TEST_ORDER) */
 
 #ifdef __cplusplus

src/util_local_visibility.h

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

tools/symbol-check.py

@@ -0,0 +1,72 @@
+#!/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 [entry.name for entry in library.get_export().entries]
+    elif library.format == lief.Binary.FORMATS.MACHO:
+        return [symbol.name[1:] for symbol in library.exported_symbols]
+    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", r"^\s*SECP256K1_API", "--", "include"],
+        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 = 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())