Add copy constructor and copy operator= to CScript to remove ubsan suppression#17510
Closed
achow101 wants to merge 1 commit intobitcoin:masterfrom
Closed
Add copy constructor and copy operator= to CScript to remove ubsan suppression#17510achow101 wants to merge 1 commit intobitcoin:masterfrom
achow101 wants to merge 1 commit intobitcoin:masterfrom
Conversation
sipa
reviewed
Nov 18, 2019
| CScript(std::vector<unsigned char>::const_iterator pbegin, std::vector<unsigned char>::const_iterator pend) : CScriptBase(pbegin, pend) { } | ||
| CScript(const unsigned char* pbegin, const unsigned char* pend) : CScriptBase(pbegin, pend) { } | ||
|
|
||
| // Define a copy constructor so that operator= is a copy instead of a move to avoid undefined behavior |
Member
There was a problem hiding this comment.
If this is needed, there is another problem. Why does a move cause UB? And why is a move being invoked in the first place with operator=?
Member
Author
There was a problem hiding this comment.
No idea. Maybe @practicalswift can shed some light here?
Member
|
For reference, one tb is this: |
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Member
Agree. This is kind of scary, and working around it like this seems a bad idea if we don't understand why it happens. |
Member
Member
|
Could this pragma in prevector.h be causing alignment issues? (doesn't it mean something like "ignore all padding requirements within the struct") |
Member
|
Smaller code to reproduce: struct Test {
uint8_t a : 8;
CScript script;
};
{
const std::array<unsigned char, 5> D{1, 2, 3, 4, 5};
Test t{0x0f, {}};
t.script = CScript{D.begin(), D.end()};
}Result: |
Member
|
Does it go away if you remove the pragma? |
Member
|
Yes, with this diff it goes away: diff --git a/src/prevector.h b/src/prevector.h
index d307495fbe..a55bcb50fe 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -14,7 +14,6 @@
#include <cstddef>
#include <type_traits>
-#pragma pack(push, 1)
/** Implements a drop-in replacement for std::vector<T> which stores up to N
* elements directly (without heap allocation). The types Size and Diff are
* used to store element counts, and can be any unsigned + signed type.
@@ -522,6 +521,5 @@ public:
return item_ptr(0);
}
};
-#pragma pack(pop)
#endif // BITCOIN_PREVECTOR_H |
Member
|
I would prefer that solution, then. I'm not sure in how far unaligned integers are really UB in C++ (versus architecture/implementation defined), but it'd be better to avoid that for architectures that don't support (or have slow fallback paths for) unaligned reads/writes. (E.g. SiFive U540 traps into the kernel/bootloader to handle unaligned reads and writes, with a function, in software. Wouldn't be surprised if it's 1000 times slower) |
Contributor
|
@laanwj John Regehr and Pascal Cuoq's excellent post "Undefined Behavior in 2017" has a nice section on Alignment Violations. Highly recommended reading! |
laanwj
added a commit
to laanwj/bitcoin
that referenced
this pull request
Nov 20, 2019
`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin#17156 (comment) and bitcoin#17510.
ajtowns
added a commit
to ajtowns/bitcoin
that referenced
this pull request
Dec 10, 2019
`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin#17156 (comment) and bitcoin#17510.
HashUnlimited
pushed a commit
to HashUnlimited/chaincoin
that referenced
this pull request
Apr 17, 2020
`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin#17156 (comment) and bitcoin#17510.
backpacker69
referenced
this pull request
in peercoin/peercoin
Mar 28, 2021
`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin/bitcoin#17156 (comment) and #17510.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 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.
As noted in #17156 (comment), the PSBT fuzzer is triggering a UBSan condition which is fixed by this PR. Additionally, by fixing this, some UBSan suppressions can be removed.
Split from #17156