script: Fix undefined behavior in Clone() -- std::transform writes past end of empty vector

[cuiweixie]

Motivation

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.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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.

[pinheadmz]

Can you provide a unit test that triggers the UB before the patch is applied? That will help with review.

[cuiweixie]

Can you provide a unit test that triggers the UB before the patch is applied? That will help with review.

@pinheadmz Done

[maflcko]

Was this LLM generated? What are the steps to test this? What is the output before and after the changes here?

[cuiweixie]

build test_bitcoin

cmake --build build -j$(sysctl -n hw.ncpu) --target test_bitcoin

with this patch output ok like:

➜  bitcoin git:(bugfix) ./build/bin/test_bitcoin --run_test=descriptor_clone_tests 
Running 2 test cases...

*** No errors detected

without this patch output like:

➜  bitcoin git:(bugfix) ✗ ./build/bin/test_bitcoin --run_test=descriptor_clone_tests      
Running 2 test cases...
test/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]
Assertion failed: (m_subdescriptor_args.size() == m_depths.size()), function TRDescriptor, file descriptor.cpp, line 1515.
******** errors disabling the alternate stack:
        #error:22
        Invalid argument
unknown location:0: fatal error: in "descriptor_clone_tests/tr_descriptor_clone": signal: SIGABRT (application abort requested)
test/descriptor_tests.cpp:1409: last checkpoint

*** 2 failures are detected in the test module "Bitcoin Core Test Suite"

[cuiweixie]

Mostly, I am writing golang. https://github.com/golang/go/commits?author=cuiweixie most pr is before LLM got popular.
I am just using LLM to give me some tips(like test example) to understand the code base.
And most code of this pr iis finish by myself.
But I do use LLM to assit me, It's something like google that I used to use mostly before LLM exists.

[maflcko]

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.

[cuiweixie]

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.

[maflcko]

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?

[cuiweixie]

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

[rkrux]

ACK 44feab2 because it gets rid of the possible undefined behaviour.

[cuiweixie]

Do we need other change before merge this pr? if not, maybe just merge this pr.

[frankomosh]

Code Review ACK 44feab2. Fix seems minimal and correct.