tools/symbol-check.py
#1135
The new tools/symbol-check.py aims to ensure that only expected symbols are exported in the secp256k1 shared library.
The script itself stems from Bitcoin Core’s contrib/devtools/symbol-check.py and bitcoin/bitcoin#25020. It uses the LIEF library.
Useful to ensure that build system changes (including bitcoin-core/secp256k1#1113) will not introduce any unexpected regressions.
I think checking that not too many symbols are exported could be interesting for us.
Checking libraries not so much, we anyway don’t have dependencies, so all linkage should be introduced by the compiler. Or what do you think?
Checking libraries not so much, we anyway don’t have dependencies
Cleaned up, improved, undrafted.
Now that I think about this, I wonder why that would be necessary. I’ve never seen any libraries doing this. I think in our case it could be helpful to prevent mistakes where we forget to make an internal function static and then it would be exported?
But on the other hand, it’s yet another file that needs to be maintained. In particular the list of function must be kept in sync with the include files. This violates the DRY principle. Do you think the list could be generated on-the-fly, maybe just using some clever grep SECP256K1_API magic? A more elaborate version would be to use clang-query or similar, see #833, but I think this would be overkill for now.
Weak Concept ACK because it has potential to prevent mistakes where we forget static
I wonder why that would be necessary.
Because quality matters :) For example:
However, generally it is not recommended to export all the symbols in modules. Programmers can just export symbols as needed. This does not only benefit library security, but also benefits dynamic linking time.
I’ve never seen any libraries doing this.
https://github.com/bitcoin/bitcoin/pull/25020#issuecomment-1142151675
I think in our case it could be helpful to prevent mistakes where we forget to make an internal function
staticand then it would be exported?
The secp256k1_pre_g is not static (in most cases). Nevertheless, its export must be avoided.
Other use cases:
maybe just using some clever
grep SECP256K1_APImagic?
:+1:
maybe just using some clever grep SECP256K1_API magic?
That would be neat.
This fails for me with
0> ./contrib/symbol-check.py ./.libs/libsecp256k1.so
1./.libs/libsecp256k1.so: symbol memcpy from unsupported version GLIBC_2.14(4)
2./.libs/libsecp256k1.so: failed IMPORTED_SYMBOLS
This fails for me with
What system/compiler on?
UPD. I assume, it is a clang…
llibsecp256k1, unlike Bitcoin Core, has no affordances to make sure the produced builds are compatible with certain, specific, version of glibc on the user’s system.
I think the max version checks should just be dropped here.
llibsecp256k1, unlike Bitcoin Core, has no affordances to make sure the produced builds are compatible with certain, specific, version of glibc on the user’s system.
Ok yes, I was wondering what the purpose of the max version is… :)
Bikeshedding: I believe contrib is the wrong directory for this, see for example this attempt at a definition: https://softwareengineering.stackexchange.com/questions/252053/whats-in-the-contrib-folder#339925.
I know, Bitcoin Core uses a different definition. My point is that the current contents of the folder fit the definition on stackexchange, so we shouldn’t mix up the two.
I think util or tools is good a name. I wouldn’t mind putting it in the root either.
It’s not clear to me that adding this script is worth it (especially as it continue to shrink). We seem to be adding a new language dependency, and third party library dependency, to run grep + objdump? I might have thought a bash script would be a better fit for libsecp256k1, especially since this is basically just a sanity check that will live in the CI (as the API rarely changes, and I don’t think anyone will run this locally)?
Is there a reason this doesn’t support macOS shared libs? LIEF supports them as far as I’m aware, and it seems odd to add a platform agnostic binary parser as a dependency, and then not support all libraries that are being built.
It’s not clear to me that adding this script is worth it (especially as it continue to shrink). We seem to be adding a new language dependency, and third party library dependency, to run grep + objdump? I might have thought a bash script would be a better fit for libsecp256k1, especially since this is basically just a sanity check that will live in the CI
I think @hebasto convinced me above that this tool has >0 benefit, but indeed, it introduces a maintenance burden and doing the same with objdump instead of Python+LIEF might reduce this burden (but I don’t know – for example I don’t know if objdump exists on macOS).
Note that we already have python as a CI dependency for our sage scripts.
(but I don’t know – for example I don’t know if objdump exists on macOS).
It does, but at the moment, I guess that doesn’t matter, given the script doesn’t check macOS libs.
Would you expect an API issue to be present in one shared library, but not all others? We might be able to get away with just sanity checking an ELF lib, as I assume if there’s an issue that you’d want to catch, present there, it’ll be present everywhere.
Is there a reason this doesn’t support macOS shared libs?
Updated to support macOS libs. LIEF v0.13.0 (not released yet) is required.
LIEF v0.13.0 (not released yet) is required.
Looks like you could do something like this to check that the expected exports are present? Works with LIEF 0.12.3 (latest release):
0diff --git a/tools/symbol-check.py b/tools/symbol-check.py
1index 8206b95..7237158 100755
2--- a/tools/symbol-check.py
3+++ b/tools/symbol-check.py
4@@ -59,12 +59,10 @@ def check_MACHO_exported_functions(library, expected_exports) -> bool:
5 ok: bool = True
6 macho_lib: lief.MACHO.Binary = library.concrete
7
8- for function in macho_lib.exported_functions:
9- name: str = function.name[1:]
10- if name in expected_exports:
11- continue
12- print(f'{filename}: export of function {name} not expected')
13- ok = False
14+ for name in expected_exports:
15+ if not macho_lib.has_symbol(name):
16+ print(f'{filename}: export of function {name} missing')
17+ ok = False
25+ grep_output = subprocess.check_output(["git", "grep", "^SECP256K1_API", "--", "include"], universal_newlines=True, encoding="utf8")
26+ lines = grep_output.split("\n")
27+ exports: List[str] = []
28+ for line in lines:
29+ if line.strip():
30+ function_name = re.sub(r'\W', '', line.split(" ")[-1])
include/secp256k1.h:SECP256K1_API void secp256k1_selftest(void); for which the script assumes the symbol is secp256k1_selftestvoid
Updated 6e6aa4915eb5e984928bbb8b675cf55fd5e67879 -> cc7933d47d70e93ef578b01f7a3336cf0aeaaaa7 (pr1135.06 -> pr1135.07):
We’ve discussed this at length in the meeting today, see https://gnusha.org/secp256k1/2022-12-19.log for details.
Conclusion:
LIEF v0.13.0 (not released yet) is required.
Looks like you could do something like this to check that the expected exports are present? Works with LIEF 0.12.3 (latest release):
0diff --git a/tools/symbol-check.py b/tools/symbol-check.py 1index 8206b95..7237158 100755 2--- a/tools/symbol-check.py 3+++ b/tools/symbol-check.py 4@@ -59,12 +59,10 @@ def check_MACHO_exported_functions(library, expected_exports) -> bool: 5 ok: bool = True 6 macho_lib: lief.MACHO.Binary = library.concrete 7 8- for function in macho_lib.exported_functions: 9- name: str = function.name[1:] 10- if name in expected_exports: 11- continue 12- print(f'{filename}: export of function {name} not expected') 13- ok = False 14+ for name in expected_exports: 15+ if not macho_lib.has_symbol(name): 16+ print(f'{filename}: export of function {name} missing') 17+ ok = False
As the suggested approach changes the check logic, the script fails if any of modules are disabled. For example, the “recovery” module is disabled by default, and script ends with:
0.libs/libsecp256k1.1.dylib: export of function secp256k1_ecdsa_recoverable_signature_parse_compact missing
1.libs/libsecp256k1.1.dylib: export of function secp256k1_ecdsa_recoverable_signature_convert missing
2.libs/libsecp256k1.1.dylib: export of function secp256k1_ecdsa_recoverable_signature_serialize_compact missing
3.libs/libsecp256k1.1.dylib: export of function secp256k1_ecdsa_sign_recoverable missing
4.libs/libsecp256k1.1.dylib: export of function secp256k1_ecdsa_recover missing
5.libs/libsecp256k1.1.dylib: failed EXPORTED_FUNCTIONS
Making this PR a draft for now.