💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340081949)
> > Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
>
> Can you explain how that test would work? Not all functions are guaranteed to be fortified.
1. Get the list of fortified functions from all binaries.
2. Create a list of the corresponding unfortified functions based on the list from step 1.
3. Check for the absence of symbols from the list in step 2.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340081949)
> > Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
>
> Can you explain how that test would work? Not all functions are guaranteed to be fortified.
1. Get the list of fortified functions from all binaries.
2. Create a list of the corresponding unfortified functions based on the list from step 1.
3. Check for the absence of symbols from the list in step 2.
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340094083)
> Get the list of fortified functions from all binaries.
This assumes that fortification is already working correctly, otherwise you'll miss any (relevant) function that hasn't be fortified at least one time.
Again, it's not a bug to have unfortified functions. So I'm not sure what you are trying to acheive by turning that into a check failure. Can you explain further.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340094083)
> Get the list of fortified functions from all binaries.
This assumes that fortification is already working correctly, otherwise you'll miss any (relevant) function that hasn't be fortified at least one time.
Again, it's not a bug to have unfortified functions. So I'm not sure what you are trying to acheive by turning that into a check failure. Can you explain further.
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2340096700)
I am thinking it would be less hassle to set up local storage. Ensuring that a given task type only runs on a given machine should be enough to ensure a "single ccache dir". (ccache results can't be shared between tasks anyway)
This would also avoid additional work or downtime when something goes wrong in the remote "storage box" (connection management, maintenance, configuration, ...)
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2340096700)
I am thinking it would be less hassle to set up local storage. Ensuring that a given task type only runs on a given machine should be enough to ensure a "single ccache dir". (ccache results can't be shared between tasks anyway)
This would also avoid additional work or downtime when something goes wrong in the remote "storage box" (connection management, maintenance, configuration, ...)
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340112965)
> Again, it's not a bug to have unfortified functions.
Then how it can be classified?
> So I'm not sure what you are trying to acheive by turning that into a check failure.
Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340112965)
> Again, it's not a bug to have unfortified functions.
Then how it can be classified?
> So I'm not sure what you are trying to acheive by turning that into a check failure.
Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340122396)
> Then how it can be classified?
As expected behaviour, given that whether fortification occurs is dependant on various (compiler & libc) heuristics.
> Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
I don't really understand what you mean. Can you give a specific example of a false po
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340122396)
> Then how it can be classified?
As expected behaviour, given that whether fortification occurs is dependant on various (compiler & libc) heuristics.
> Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
I don't really understand what you mean. Can you give a specific example of a false po
...
💬 maflcko commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1751591719)
style nit for `LogPrintf("Error` here and below:
For new code, it would be good to use a non-deprecated log function (like `LogInfo`, `LogWarning`, or `LogError`). Otherwise, there may be confusion whether the `info` severity was picked on purpose or if something more suitable should be used instead.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1751591719)
style nit for `LogPrintf("Error` here and below:
For new code, it would be good to use a non-deprecated log function (like `LogInfo`, `LogWarning`, or `LogError`). Otherwise, there may be confusion whether the `info` severity was picked on purpose or if something more suitable should be used instead.
💬 maflcko commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2340166976)
Apart from the benchmark, it may also be useful to create a flamegraph/heatmap, so that it is easy to see where the time is spent.
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2340166976)
Apart from the benchmark, it may also be useful to create a flamegraph/heatmap, so that it is easy to see where the time is spent.
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340179006)
> Can you give a specific example of a false positive/issue, in the context of our Guix environment/build.
Sure. This [branch](https://github.com/hebasto/bitcoin/commits/pr27038/0910-DEMO.2/) clearly demonstrates false positive results for `bitcoind` and `bitcoin-qt`:
```
b07d7f1b7e5f5eaf8649685e7f8e031e4ac078f87dbd27e0d732c2a578ef3c4c guix-build-423fc912bca9/output/dist-archive/bitcoin-423fc912bca9.tar.gz
e7cce3c0bdf87e4583067fdf39aea50fb4c12fb0d52cbb78e3c7c0c967a9b215 guix-build-423fc9
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340179006)
> Can you give a specific example of a false positive/issue, in the context of our Guix environment/build.
Sure. This [branch](https://github.com/hebasto/bitcoin/commits/pr27038/0910-DEMO.2/) clearly demonstrates false positive results for `bitcoind` and `bitcoin-qt`:
```
b07d7f1b7e5f5eaf8649685e7f8e031e4ac078f87dbd27e0d732c2a578ef3c4c guix-build-423fc912bca9/output/dist-archive/bitcoin-423fc912bca9.tar.gz
e7cce3c0bdf87e4583067fdf39aea50fb4c12fb0d52cbb78e3c7c0c967a9b215 guix-build-423fc9
...
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340212685)
> Sure. This branch clearly demonstrates false positive results for bitcoind and bitcoin-qt:
Thanks. I built this branch and inspected `bitcoind`, and it contains calls to fortified functions. i.e:
```bash
objdump -D /root/bitcoin/guix-build-423fc912bca9/output/x86_64-linux-gnu/bitcoin-423fc912bca9/bin/bitcoind
<snip>
6f9afe: e8 cd dd 95 ff call 578d0 <__vsnprintf_chk@plt>
6fd0a1: e8 ba b1 95 ff call 58260 <__fprintf_chk@plt>
6fe135: e8 66 a0 95 ff call
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340212685)
> Sure. This branch clearly demonstrates false positive results for bitcoind and bitcoin-qt:
Thanks. I built this branch and inspected `bitcoind`, and it contains calls to fortified functions. i.e:
```bash
objdump -D /root/bitcoin/guix-build-423fc912bca9/output/x86_64-linux-gnu/bitcoin-423fc912bca9/bin/bitcoind
<snip>
6f9afe: e8 cd dd 95 ff call 578d0 <__vsnprintf_chk@plt>
6fd0a1: e8 ba b1 95 ff call 58260 <__fprintf_chk@plt>
6fe135: e8 66 a0 95 ff call
...
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340221792)
> So how is this a false positive?
This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340221792)
> So how is this a false positive?
This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1751658436)
Got the error again, trying to create a godbolt reproducer to understand the apparent pointer arithmetic misunderstanding
<details>
<summary>Error</summary>
```
In file included from /ci_container_base/src/script/script.h:11,
from /ci_container_base/src/primitives/transaction.h:11,
from /ci_container_base/src/primitives/block.h:9,
from /ci_container_base/src/kernel/chainparams.h:11,
from /ci_container_base/src/kernel
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1751658436)
Got the error again, trying to create a godbolt reproducer to understand the apparent pointer arithmetic misunderstanding
<details>
<summary>Error</summary>
```
In file included from /ci_container_base/src/script/script.h:11,
from /ci_container_base/src/primitives/transaction.h:11,
from /ci_container_base/src/primitives/block.h:9,
from /ci_container_base/src/kernel/chainparams.h:11,
from /ci_container_base/src/kernel
...
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340225004)
> This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
Yes, the check as implemented, which checks for (any) usage of foritfied function calls in the binary, passes, because the binary contains calls to fortified functions.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340225004)
> This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
Yes, the check as implemented, which checks for (any) usage of foritfied function calls in the binary, passes, because the binary contains calls to fortified functions.
💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751663364)
Reusing the same data from the fuzz input for two different things (e.g. decode/encode and encode/decode) is usually not a good idea.
How about adding a decode step after the encoding above (i.e. decode -> encode -> decode)? Or alternatively split this second round-trip into its own harness as well?
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751663364)
Reusing the same data from the fuzz input for two different things (e.g. decode/encode and encode/decode) is usually not a good idea.
How about adding a decode step after the encoding above (i.e. decode -> encode -> decode)? Or alternatively split this second round-trip into its own harness as well?
:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30848)
(https://github.com/bitcoin/bitcoin/issues/30848)
👍 dergoegge approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292038673)
Code review ACK 1243d2ffa30ebc8a148623e3cdabd61f70714932
CI timeout is unrelated
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292038673)
Code review ACK 1243d2ffa30ebc8a148623e3cdabd61f70714932
CI timeout is unrelated
👍 fanquake approved a pull request: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791#pullrequestreview-2292069963)
ACK 2d68c3b1c2e4f8fb881efc3569506d426ee5155d
(https://github.com/bitcoin/bitcoin/pull/30791#pullrequestreview-2292069963)
ACK 2d68c3b1c2e4f8fb881efc3569506d426ee5155d
🚀 fanquake merged a pull request: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791)
(https://github.com/bitcoin/bitcoin/pull/30791)
💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2340343047)
Addressed comments by @instagibbs. Thanks for the review!
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2340343047)
Addressed comments by @instagibbs. Thanks for the review!
👍 dergoegge approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292103593)
reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292103593)
reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751765403)
Thanks for the review!
> is usually not a good idea
Can you please expand on that? I don't mind splitting, as you've recommended, but I have to understand what the current drawbacks are.
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751765403)
Thanks for the review!
> is usually not a good idea
Can you please expand on that? I don't mind splitting, as you've recommended, but I have to understand what the current drawbacks are.