Use swap hack rather than hidden public API #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In preparation for moving the `ScriptBuf` as a plain old datatype to `primitives`; separate the POD methods into their own impl block. Refactor only, no logic changes.
Before we can move the `ScriptBuf` to `primitives` we need a few ways to mutate the inner vector, currently done by accessing `.0`. Add `push`, `pop`, and `extend_from_slice`. Hide them from the docs and comment that they are not really supposed to be used.
In preparation for adding a private `ScriptBufExtPriv` trait, move the private methods to a separate impl block. Make the `push_slice_no_op` method have the same visibility as the others so that the change in scope does not get hidden in the upcoming patch that introduces the trait. (Note this is basically just a code move and the diff shows move of other methods not the private ones.)
As we did before with `Script`; `rustfmt` does not indent stuff in macros so in preparation for adding extension traits using a macro temporarily wrap the impl blocks in modules so we can run the formatter in a patch on its own.
Run `cargo +nightly fmt`, no other changes.
The `define_extension_trait` cannot be easily extended to handle our current usage of `AsRef<PushBytes>` generic. We can achieve almost the same thing with the `impl` syntax. Note that this is a breaking change because with this change turbo fish syntax is no longer possible.
In preparation for moving the `ScriptBuf` type to the `primitives` crate; introduce two `ScriptBuf` extension traits, one public and one private. Note, the private extension trait has `pub(crate)` where as before this patch was applied we use `pub(in crate::script)`. This is because the macro doesn't handle the latter syntax. Note also, that the build failure when `ScriptBuf::from_hex` is not found may be confusing because of the history of `from_hex` - users may go looking for a `FromHex` trait. We should keep this in mind when documenting how to use all the new extension traits.
A public API even if hidden has potential compatibility risks that we want to avoid. We could come up with better API but we can simply workaround it by temporarily swapping the script with an empty one, then modifying the vec and then swapping it back.
tcharding
reviewed
Aug 17, 2024
Comment on lines
244
to
245
| self.as_byte_vec().pop(); | ||
| self.push_opcode(opcode); |
Owner
There was a problem hiding this comment.
That's some trickery - nice one!
tcharding
force-pushed
the
08-13-script-buf
branch
4 times, most recently
from
836a226 to
2bb90b8
Compare
tcharding
pushed a commit
that referenced
this pull request
Jan 6, 2026
…prefix 5d4f9cf Fix decoder bug when ending before decoding prefix (Shing Him Ng) Pull request description: Before this fix, calling `ByteVecDecoder.end()` on a decoder that hadn't finished reading in the full prefix would result in a valid result of an empty vec. This should instead result in an error, since the decoder shouldn't be able to decode something with an incomplete prefix. Found from rust-bitcoin#5315 when i was testing some of the fuzzing: ``` INFO: Running with entropic power schedule (0xFF, 100). INFO: Seed: 4008033836 INFO: Loaded 1 modules (726 inline 8-bit counters): 726 [0x100aca050, 0x100aca326), INFO: Loaded 1 PC tables (726 PCs): 726 [0x100aca328,0x100acd088), INFO: 10 files found in /Users/shingng/git/rust-bitcoin/fuzz/corpus/consensus_encoding_decode_byte_vec INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes thread '<unnamed>' (5327178) panicked at fuzz/fuzz_targets/consensus_encoding/decode_byte_vec.rs:21:17: decoder should error when insufficient data provided note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ==12615== ERROR: libFuzzer: deadly signal #0 0x0001013453c4 in __sanitizer_print_stack_trace+0x28 (librustc-nightly_rt.asan.dylib:arm64+0x5d3c4) #1 0x000100a518d4 in fuzzer::PrintStackTrace()+0x30 (consensus_encoding_decode_byte_vec:arm64+0x1000158d4) #2 0x000100a45db8 in fuzzer::Fuzzer::CrashCallback()+0x54 (consensus_encoding_decode_byte_vec:arm64+0x100009db8) #3 0x00019914f740 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x3740) #4 0x000199145884 in pthread_kill+0x124 (libsystem_pthread.dylib:arm64+0x6884) #5 0x00019904a84c in abort+0x78 (libsystem_c.dylib:arm64+0x7984c) #6 0x000100aa9e44 in _RNvNtNtNtCsk9AQ7OSayGk_3std3sys3pal4unix14abort_internal+0x8 (consensus_encoding_decode_byte_vec:arm64+0x10006de44) #7 0x000100aa9cd0 in _RNvNtCsk9AQ7OSayGk_3std7process5abort+0x8 (consensus_encoding_decode_byte_vec:arm64+0x10006dcd0) #8 0x000100aa5528 in _RNCNvCsaBYAWE6hvc2_13libfuzzer_sys10initialize0B3_+0xb8 (consensus_encoding_decode_byte_vec:arm64+0x100069528) #9 0x000100a9106c in _RNvNtCsk9AQ7OSayGk_3std9panicking15panic_with_hook+0x264 (consensus_encoding_decode_byte_vec:arm64+0x10005506c) #10 0x000100a85148 in _RNCNvNtCsk9AQ7OSayGk_3std9panicking13panic_handler0B5_+0x6c (consensus_encoding_decode_byte_vec:arm64+0x100049148) #11 0x000100a7cad4 in _RINvNtNtCsk9AQ7OSayGk_3std3sys9backtrace26___rust_end_short_backtraceNCNvNtB6_9panicking13panic_handler0zEB6_+0x8 (consensus_encoding_decode_byte_vec:arm64+0x100040ad4) #12 0x000100a8572c in _RNvCseYE12Li5r0M_7___rustc17rust_begin_unwind+0x1c (consensus_encoding_decode_byte_vec:arm64+0x10004972c) #13 0x000100aaa484 in _RNvNtCsh0x4TIixgmZ_4core9panicking9panic_fmt+0x24 (consensus_encoding_decode_byte_vec:arm64+0x10006e484) #14 0x000100a3d6ac in _RNvNvCsdWVpjOStM1p_34consensus_encoding_decode_byte_vec1__19___libfuzzer_sys_run decode_byte_vec.rs:45 #15 0x000100a3f52c in rust_fuzzer_test_input lib.rs:276 #16 0x000100a4436c in _RINvNvNtCsk9AQ7OSayGk_3std9panicking12catch_unwind7do_callNCNvCsaBYAWE6hvc2_13libfuzzer_sys15test_input_wrap0lEBY_+0xc4 (consensus_encoding_decode_byte_vec:arm64+0x10000836c) #17 0x000100a45034 in __rust_try+0x18 (consensus_encoding_decode_byte_vec:arm64+0x100009034) rust-bitcoin#18 0x000100a43c6c in LLVMFuzzerTestOneInput+0x16c (consensus_encoding_decode_byte_vec:arm64+0x100007c6c) rust-bitcoin#19 0x000100a47670 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)+0x158 (consensus_encoding_decode_byte_vec:arm64+0x10000b670) rust-bitcoin#20 0x000100a488e8 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&)+0x240 (consensus_encoding_decode_byte_vec:arm64+0x10000c8e8) rust-bitcoin#21 0x000100a49058 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&)+0x88 (consensus_encoding_decode_byte_vec:arm64+0x10000d058) rust-bitcoin#22 0x000100a676b8 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))+0x1aa4 (consensus_encoding_decode_byte_vec:arm64+0x10002b6b8) rust-bitcoin#23 0x000100a74158 in main+0x24 (consensus_encoding_decode_byte_vec:arm64+0x100038158) rust-bitcoin#24 0x000198d7dd50 (<unknown module>) NOTE: libFuzzer has rudimentary signal handlers. Combine libFuzzer with AddressSanitizer or similar for better crash reports. SUMMARY: libFuzzer: deadly signal MS: 0 ; base unit: 0000000000000000000000000000000000000000 artifact_prefix='/Users/shingng/git/rust-bitcoin/fuzz/artifacts/consensus_encoding_decode_byte_vec/'; Test unit written to /Users/shingng/git/rust-bitcoin/fuzz/artifacts/consensus_encoding_decode_byte_vec/crash-da39a3ee5e6b4b0d3255bfef95601890afd80709 Base64: ──────────────────────────────────────────────────────────────────────────────── Failing input: fuzz/artifacts/consensus_encoding_decode_byte_vec/crash-da39a3ee5e6b4b0d3255bfef95601890afd80709 Output of `std::fmt::Debug`: [] Reproduce with: cargo fuzz run consensus_encoding_decode_byte_vec fuzz/artifacts/consensus_encoding_decode_byte_vec/crash-da39a3ee5e6b4b0d3255bfef95601890afd80709 Minimize test case with: cargo fuzz tmin consensus_encoding_decode_byte_vec fuzz/artifacts/consensus_encoding_decode_byte_vec/crash-da39a3ee5e6b4b0d3255bfef95601890afd80709 ──────────────────────────────────────────────────────────────────────────────── Error: Fuzz target exited with exit status: 77 ``` ACKs for top commit: apoelstra: ACK 5d4f9cf; successfully ran local tests tcharding: ACK 5d4f9cf Tree-SHA512: 89298839cfc33b2cf3379ca5ddcbf3edb94b81d5dfae01ca23b034e390f01653a1a147642928313ae02a2136a9a9397f6f66d369abb12786016ac2186638f076
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I felt like writing up the change. You probably want to incorporate it into previous history rather than leaving things change back and forth.
A public API even if hidden has potential compatibility risks that we want to avoid. We could come up with better API but we can simply workaround it by temporarily swapping the script with an empty one, then modifying the vec and then swapping it back.