Partially resolves #30520
bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:https://github.com/bitcoin/bitcoin/blob/44057fe38ccc5d05a6249413467e002db2ae023d/src/secp256k1/examples/CMakeLists.txt#L9lief package to 0.16.5contrib/devtools/security-check.py
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32431.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
-static-pie bitcoind by fanquake)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/41764401882
LLM reason (✨ experimental): (empty)
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Concept ACK.
Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:
From what i understand, that’s only used for the example, not for the library itself. The bcrypt-using code (fill_random in examples/examples_util.h) is never included in the library.
It’s unlikely that the LIEF version changes anything there.
157@@ -158,7 +158,7 @@ chain for " target " development."))
158 (define-public python-lief
159 (package
160 (name "python-lief")
161- (version "0.13.2")
162+ (version "0.16.5")
From what i understand, that’s only used for the example, not for the library itself. The bcrypt-using code (
fill_randominexamples/examples_util.h) is never included in the library.It’s unlikely that the LIEF version changes anything there.
Oops, my bad, I must have ran symbol-check.py against a stale build of the bcrypt branch, rebased to drop adding bcrypt to symbol-check.py, marking draft until I test this as working in Guix.
Add usage notes for contrib/devtools/security-check.py
I think you can actually remove these docs. The symbol / security scripts are not for general developer usage, and there’s no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.
Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:
This seems to be a symptom of the above. Dependencies in libsecp256k1 example code, are not relevant for our release builds. Looking at a Guix build of master, I cannot see any dependency on bcrypt.dll in any binary.
283
284 if __name__ == '__main__':
285 retval: int = 0
286 for filename in sys.argv[1:]:
287 binary = lief.parse(filename)
288+ if binary is None:
.parse fails is fine.
lief.Binary without checking
I think you can actually remove these docs.
Thanks, fixed. I did not understand that these were not meant for use outside of Guix builds,
I definitely opened this PR prematurely, thinking I could get away with just bumping the version and fixing the python scripts 😂.
Working on getting guix builds working right, I referenced prior art by @willcl-ark here (https://github.com/willcl-ark/bitcoin/commit/ede9aa93fe6339fe6285c954dc8fbd9ae8623916).
The python build is convinced that a build option is set in config-settings, even when I override configure-flags, which is where config-settings is supposed to get it’s value from (https://github.com/fanquake/guix/blob/ac2d792aae241f5233ee3fdfa29cd3dbaeb9338c/guix/build/pyproject-build-system.scm#L107-L111) :
0starting phase `set-pythonpath'
1phase `set-pythonpath' succeeded after 0.0 seconds
2starting phase `change-directory'
3phase `change-directory' succeeded after 0.0 seconds
4starting phase `build'
5Using 'setup' to build wheels, auto-detected 'setup', override '#f'.
6ERROR: Unrecognized options in config-settings:
7 build -> Did you mean: build-dir?
8error: in phase 'build': uncaught exception:
9%exception #<&invoke-error program: "python" arguments: ("-c" "import sys, importlib, json\nconfig_settings = json.loads (sys.argv[3])\nbuilder = importlib.import_module(sys.argv[1])\nbuilder.build_wheel(sys.argv[2], config_settings=config_settings)" "setup" "dist" "[]") exit-status: 7 term-signal: #f stop-signal: #f>
10phase `build' failed after 0.1 seconds
11command "python" "-c" "import sys, importlib, json\nconfig_settings = json.loads (sys.argv[3])\nbuilder = importlib.import_module(sys.argv[1])\nbuilder.build_wheel(sys.argv[2], config_settings
Some of the primary changes are:
- lief.EXE_FORMATS became lief.Binary.FORMATS IN 0.14.0
- https://github.com/lief-project/LIEF/blob/494f116c6b8c9163fc4b5f6497233f18424981ee/doc/sphinx/changelog.rst?plain=1#L702
- lief.ARCHITECTURES became lief.Header.ARCHITECTURES in 0.16.0
- https://github.com/lief-project/LIEF/blob/494f116c6b8c9163fc4b5f6497233f18424981ee/doc/sphinx/changelog.rst?plain=1#L226C18-L227C18
- lief.ELF.ARCH.x86_64 became lief.ELF.ARCH.X86_64
Co-authored-by: willcl-ark <will8clark@gmail.com>The symbol / security scripts are not for general developer usage, and there’s no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.
i think this makes sense, especially for the symbol check. It doesn’t have any use outside the guix build. The security check may have some value as a more general “binary security check”. But moving both to a guix-specific path would be fine with me.