Mark resource-owning classes non-copyable#2667
Open
pr0toboy wants to merge 1 commit into
Open
Conversation
Wifi, DSi_I2CHost and DynamicFIFO all manage heap-allocated state and have a user-defined destructor that frees it, but neither declares a copy constructor nor a copy assignment operator. The implicitly generated versions copy the owned pointer/buffer by value, which would lead to a double-free as soon as one of the copies is destroyed. C++17 guaranteed copy elision currently hides this on the existing call sites (notably the DynamicFIFO Mailbox[9] aggregate-init in DSi_NWifi), so the bug is latent, not active — but the safety net is one accidental `auto x = y;` away from biting. Declare both as = delete on each class. If a future change ever needs copy semantics, the compiler will now refuse it and force the author to write the copy correctly, instead of silently producing a UAF. Build was clean before/after, confirming no existing call site relies on the implicit copies. Found by cppcheck (`noCopyConstructor` + `noOperatorEq` on Wifi, DSi_I2CHost, and DynamicFIFO<>).
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Wifi,DSi_I2CHostand theDynamicFIFO<T>template all manage heap-allocated state and have a user-defined destructor that frees it (delete[] Entries, ownedDSi_BPTWL/DSi_Camera*, etc.), but none of them declares a copy constructor or a copy assignment operator. The implicit ones copy the owned pointers by value, which would lead to a double-free or a UAF as soon as one of the copies goes out of scope.C++17's guaranteed copy elision currently hides this on the existing call sites — most notably the aggregate-init of
DynamicFIFO<u8> Mailbox[9]inDSi_NWifi— so the issue is latent rather than active. The safety net is one accidentalauto x = y;or pass-by-value away from biting, though.This patch declares the copy ctor and copy assignment as
= deleteon each class. If a future change ever needs copy semantics, the compiler will refuse it and force the author to think about it, instead of silently producing a UAF.Build was clean before/after, confirming no existing call site relies on the implicit copies.
Found by cppcheck (
noCopyConstructor+noOperatorEqonWifi,DSi_I2CHost,DynamicFIFO<>).