GFrameDataCollection owns three raw pointers and deletes them in its destructor, but declares no copy/move operations. A copy would share the same pointers and double-free on destruction.
Affected files
gemc/gdata/frame/gFrameDataCollection.h:48-89 (class definition, ctor, dtor)
- members at
gFrameDataCollection.h:197-198
Details
The class owns gevent_header, the payload objects, and the heap-allocated payload vector, and frees all three:
~GFrameDataCollection() {
log->debug(DESTRUCTOR, "GFrameDataCollection");
delete gevent_header;
for (auto* payload : *integralPayloads) { delete payload; }
delete integralPayloads;
}
Members:
GFrameHeader* gevent_header = nullptr;
std::vector<GIntegralPayload*>* integralPayloads;
No copy ctor, copy assignment, move ctor, or move assignment is declared, so the compiler implicitly generates copy/move that perform a shallow pointer copy. If a GFrameDataCollection is ever copied (e.g. stored in a container by value, returned by value pre-C++17 elision, or passed by value), two objects own the same gevent_header / integralPayloads, and the second destructor double-frees them.
Impact
Latent. No active copy site was found in the current code, so the bug is dormant — but it is a footgun: any future code that copies the object (or accidentally passes it by value) gets undefined behavior. The owning raw pointers with a custom destructor and no rule-of-five is exactly the classic violation.
Proposed fix
Make the ownership intent explicit: delete copy, default move. (std::vector<GIntegralPayload*>* and GFrameHeader* are trivially movable as raw pointers, but defaulted move would leave the source's pointers dangling unless nulled — so define move to transfer and null the source, or simply delete both copy and move to forbid relocation entirely. Given there is no current need to move it, deleting all four is the safest minimal fix.)
GFrameDataCollection(GFrameHeader* header, std::shared_ptr<GLogger> logger)
: log(logger), gevent_header(header) {
log->debug(CONSTRUCTOR, "GFrameDataCollection");
integralPayloads = new std::vector<GIntegralPayload*>();
}
+
+ // Owns raw pointers freed in the destructor: forbid copying to avoid double-free.
+ GFrameDataCollection(const GFrameDataCollection&) = delete;
+ GFrameDataCollection& operator=(const GFrameDataCollection&) = delete;
+ // No current need to relocate; forbid moves too (re-enable with explicit
+ // pointer-nulling move ops if a move is later required).
+ GFrameDataCollection(GFrameDataCollection&&) = delete;
+ GFrameDataCollection& operator=(GFrameDataCollection&&) = delete;
The cleaner long-term fix (noted in the file's own header comment) is to convert the members to std::unique_ptr<GFrameHeader> and std::vector<std::unique_ptr<GIntegralPayload>>, which makes the destructor trivial and the move operations correct by default. The = delete set above is the minimal, surgical change to close the double-free hole now.
Split out from #102 (one issue per bug). Found via an AI-assisted code review with manual verification against commit 5f8ce875.
GFrameDataCollectionowns three raw pointers and deletes them in its destructor, but declares no copy/move operations. A copy would share the same pointers and double-free on destruction.Affected files
gemc/gdata/frame/gFrameDataCollection.h:48-89(class definition, ctor, dtor)gFrameDataCollection.h:197-198Details
The class owns
gevent_header, the payload objects, and the heap-allocated payload vector, and frees all three:Members:
GFrameHeader* gevent_header = nullptr; std::vector<GIntegralPayload*>* integralPayloads;No copy ctor, copy assignment, move ctor, or move assignment is declared, so the compiler implicitly generates copy/move that perform a shallow pointer copy. If a
GFrameDataCollectionis ever copied (e.g. stored in a container by value, returned by value pre-C++17 elision, or passed by value), two objects own the samegevent_header/integralPayloads, and the second destructor double-frees them.Impact
Latent. No active copy site was found in the current code, so the bug is dormant — but it is a footgun: any future code that copies the object (or accidentally passes it by value) gets undefined behavior. The owning raw pointers with a custom destructor and no rule-of-five is exactly the classic violation.
Proposed fix
Make the ownership intent explicit: delete copy, default move. (
std::vector<GIntegralPayload*>*andGFrameHeader*are trivially movable as raw pointers, but defaulted move would leave the source's pointers dangling unless nulled — so define move to transfer and null the source, or simply delete both copy and move to forbid relocation entirely. Given there is no current need to move it, deleting all four is the safest minimal fix.)GFrameDataCollection(GFrameHeader* header, std::shared_ptr<GLogger> logger) : log(logger), gevent_header(header) { log->debug(CONSTRUCTOR, "GFrameDataCollection"); integralPayloads = new std::vector<GIntegralPayload*>(); } + + // Owns raw pointers freed in the destructor: forbid copying to avoid double-free. + GFrameDataCollection(const GFrameDataCollection&) = delete; + GFrameDataCollection& operator=(const GFrameDataCollection&) = delete; + // No current need to relocate; forbid moves too (re-enable with explicit + // pointer-nulling move ops if a move is later required). + GFrameDataCollection(GFrameDataCollection&&) = delete; + GFrameDataCollection& operator=(GFrameDataCollection&&) = delete;The cleaner long-term fix (noted in the file's own header comment) is to convert the members to
std::unique_ptr<GFrameHeader>andstd::vector<std::unique_ptr<GIntegralPayload>>, which makes the destructor trivial and the move operations correct by default. The= deleteset above is the minimal, surgical change to close the double-free hole now.Split out from #102 (one issue per bug). Found via an AI-assisted code review with manual verification against commit
5f8ce875.