This patch fixes undefined behavior in Clone() in src/script/descriptor.cpp. When std::transform is used with providers.begin() or subdescs.begin() as the output iterator, the vectors have been reserve()d but have size 0. Writing through begin() in that case writes past the logical end of the vector, which is undefined behavior.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, rkrux, frankomosh |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Can you provide a unit test that triggers the UB before the patch is applied? That will help with review. @pinheadmz Done
build test_bitcoin
0cmake --build build -j$(sysctl -n hw.ncpu) --target test_bitcoin
with this patch output ok like:
0➜ bitcoin git:(bugfix) ./build/bin/test_bitcoin --run_test=descriptor_clone_tests
1Running 2 test cases...
2
3*** No errors detected
without this patch output like:
0➜ bitcoin git:(bugfix) ✗ ./build/bin/test_bitcoin --run_test=descriptor_clone_tests
1Running 2 test cases...
2test/descriptor_tests.cpp:1392: error: in "descriptor_clone_tests/multisig_descriptor_clone": check clone->ToString(false) == descs[0]->ToString() has failed [wsh(multi(2))#ma2fnlvs != wsh(multi(2,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3))#flt5crve]
3Assertion failed: (m_subdescriptor_args.size() == m_depths.size()), function TRDescriptor, file descriptor.cpp, line 1515.
4******** errors disabling the alternate stack:
5 #error:22
6 Invalid argument
7unknown location:0: fatal error: in "descriptor_clone_tests/tr_descriptor_clone": signal: SIGABRT (application abort requested)
8test/descriptor_tests.cpp:1409: last checkpoint
9
10*** 2 failures are detected in the test module "Bitcoin Core Test Suite"
Ok, I see.
This is actually dead/unreachable code, so the fix isn’t urgent.
About the test: I think it is a bit too spicy to include the c++ file in such a way to test this. I’d say it is fine to drop the commit, but no strong opinion.
Ok, I see.
This is actually dead/unreachable code, so the fix isn’t urgent.
About the test: I think it is a bit too spicy to include the c++ file in such a way to test this. I’d say it is fine to drop the commit, but no strong opinion.
Drop the unittest case commit.
lgtm ACK 44feab23a7c6060a3b432c04e3f952c5a7104325
Seems fine to fix UB in dead and unreachable code as a small style cleanup.
However, for the pull description, I’d recommend to remove the sections
lgtm ACK 44feab2
Seems fine to fix UB in dead and unreachable code as a small style cleanup.
However, for the pull description, I’d recommend to remove the sections
- Changes: If anyone wanted to look at the changes, they could just look at them directly
- Testing: This should say that it is dead/unreachable code. Maybe mention the test commit from earlier?
Done
