Fix symbol visibility issues, add test for it #1359

pull hebasto wants to merge 5 commits into bitcoin-core:master from hebasto:230626-visibility changing 12 files +161 −9
  1. hebasto commented at 12:16 pm on June 26, 2023: member

    Closes #1181.

    Catches the actual symbol visibility issue.

    Replaces #1135.

  2. real-or-random added the label build on Jun 26, 2023
  3. real-or-random added the label assurance on Jun 26, 2023
  4. hebasto force-pushed on Jun 27, 2023
  5. hebasto commented at 7:45 am on June 27, 2023: member
    Rebased on top of the merged #1356.
  6. hebasto force-pushed on Jul 13, 2023
  7. hebasto commented at 3:56 pm on July 13, 2023: member
    Rebased f8167296935958e1c7a65654de8c6606959e9f3f -> d7b1ceaa834d00709dc1206be6dbd65428513ef2 (pr1359.02 -> pr1359.03) due to the conflict with #1313.
  8. hebasto force-pushed on Jul 13, 2023
  9. hebasto marked this as a draft on Jul 13, 2023
  10. hebasto force-pushed on Jul 13, 2023
  11. hebasto marked this as ready for review on Jul 13, 2023
  12. hebasto marked this as a draft on Aug 31, 2023
  13. hebasto force-pushed on Feb 12, 2024
  14. hebasto marked this as ready for review on Feb 12, 2024
  15. 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?

  16. maflcko commented at 8:56 am on February 13, 2024: contributor
    Maybe --env-file drops ENV?
  17. hebasto force-pushed on Feb 13, 2024
  18. hebasto force-pushed on Apr 23, 2024
  19. hebasto commented at 1:01 pm on April 23, 2024: member
    Rebased to resolve conflicts.
  20. hebasto force-pushed on Aug 25, 2024
  21. hebasto force-pushed on Aug 25, 2024
  22. hebasto force-pushed on Aug 25, 2024
  23. hebasto force-pushed on Aug 25, 2024
  24. hebasto force-pushed on Aug 25, 2024
  25. 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 :)

  26. hebasto force-pushed on Oct 16, 2024
  27. hebasto marked this as a draft on Oct 16, 2024
  28. hebasto force-pushed on Oct 16, 2024
  29. Do not export `secp256k1_musig_nonce_gen_internal` e2571385e0
  30. hebasto force-pushed on Oct 16, 2024
  31. hebasto marked this as ready for review on Oct 16, 2024
  32. hebasto commented at 2:34 pm on October 16, 2024: member
    Rebased.
  33. 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.
  34. 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.
  35. 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.

  36. theuni approved
  37. 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.

  38. hebasto force-pushed on Oct 16, 2024
  39. 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.

  40. real-or-random added this to the milestone 0.6.0 on Oct 21, 2024
  41. real-or-random referenced this in commit f0868a9b3d on Oct 21, 2024
  42. theuni approved
  43. theuni commented at 3:51 pm on October 22, 2024: contributor
    Code review ACK 44a9392f8b124aab44bd38c7aab6fbf8c4ac3c11. No opinion on the tests/ci.
  44. 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.
  45. 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.
  46. 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.
  47. 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.
  48. 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.
  49. 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.
  50. Introduce `SECP256K1_LOCAL_VAR` macro
    This change makes the `-fvisibility=hidden` compiler option unnecessary.
    275ec533de
  51. 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.
  52. hebasto force-pushed on Oct 24, 2024
  53. hebasto commented at 10:07 am on October 24, 2024: member
    @real-or-random’s comments have been addressed.
  54. 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:
  55. test: Add `tools/symbol-check.py` baae375df6
  56. ci: Run `tools/symbol-check.py` 759a4818c1
  57. build: Drop no longer needed `-fvisibility=hidden` compiler option b80743a40f
  58. 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.
  59. hebasto force-pushed on Oct 24, 2024
  60. hebasto commented at 11:38 am on October 24, 2024: member
    @fanquake’s feedback has been addressed.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 01:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me