Skip to content

[Low] Add rule-of-five to GFrameDataCollection to prevent double-free if copied #136

@zhaozhiwen

Description

@zhaozhiwen

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions