Skip to content

[Medium] Stop leaking heap ifstream on every ASCII system/material load #132

@zhaozhiwen

Description

@zhaozhiwen

GSystemTextFactory::gSystemTextFileStream returns a heap-allocated std::ifstream* that callers close() but never delete, leaking one stream per ASCII geometry/material load. The not-found / nullptr return paths leak too.

Affected files

  • gemc/gsystem/gsystemFactories/text/systemTextFactory.cc:30 (new std::ifstream(...))
  • gemc/gsystem/gsystemFactories/text/loadGeometry.cc:19,38 (caller: close() only)
  • gemc/gsystem/gsystemFactories/text/loadMaterials.cc:13,32 (caller: close() only)

Details

The factory allocates the stream on the heap and returns the raw pointer:

std::ifstream* GSystemTextFactory::gSystemTextFileStream(GSystem* system, const std::string& SYSTEMTYPE) {
    ...
    auto IN = new std::ifstream(fname.c_str());
    if (IN->good()) { return IN; }
    ... // also returns IN from the trial-location loop

Callers only close, never delete:

// loadGeometry.cc
auto IN = gSystemTextFileStream(system, GTEXTGEOMTYPE);
if (IN != nullptr) {
    while (!IN->eof()) { ... }
    IN->close();   // closes the file handle but the heap object leaks
}

loadMaterials.cc has the identical pattern. There is no delete IN anywhere, so each call leaks a std::ifstream. (The nullptr early-returns at systemTextFactory.cc:57,63 don't leak because they return before/without a live owner mismatch — but they also discard a stream only in the not-found branch where IN was already allocated, so they leak the allocated IN too.)

Impact

A memory leak proportional to the number of systems loaded (geometry + materials files). Small in absolute terms, but it grows with geometry reloads (each Construct() rebuild reloads systems), so long-running interactive sessions accumulate leaks.

Proposed fix (preferred): return a std::unique_ptr

Change the factory to transfer ownership; callers then drop the manual close() (RAII closes on scope exit).

systemTextFactory.h (declaration):

-    std::ifstream* gSystemTextFileStream(GSystem* system, const std::string& SYSTEMTYPE);
+    std::unique_ptr<std::ifstream> gSystemTextFileStream(GSystem* system, const std::string& SYSTEMTYPE);

systemTextFactory.cc:

-    auto IN = new std::ifstream(fname.c_str());
+    auto IN = std::make_unique<std::ifstream>(fname.c_str());

     if (IN->good()) {
         log->info(1, "Trying file ", fname);
-        return IN;
+        return IN;          // unique_ptr is moved out
     }
     ...
         if (IN->good()) {
             log->info(1, "Trying file ", newName);
-            return IN;
+            return IN;
         }
     ...
-    return nullptr;
+    return nullptr;         // implicit unique_ptr(nullptr)

(All three return IN; sites move the same unique_ptr; the return nullptr; sites are fine since unique_ptr is nullptr-constructible.)

loadGeometry.cc / loadMaterials.cc:

-    auto IN = gSystemTextFileStream(system, GTEXTGEOMTYPE);
+    auto IN = gSystemTextFileStream(system, GTEXTGEOMTYPE);   // now unique_ptr<ifstream>
     if (IN != nullptr) {
         ...
-        IN->close();
+        // stream auto-closed / freed when IN goes out of scope
     }

Minimal alternative (if signature change is undesirable): keep the raw pointer but add delete IN; after IN->close(); in both callers, and ensure the not-found branch in the factory deletes before returning nullptr. The unique_ptr version is preferred because it makes the leak impossible by construction.


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

    Status
    Planned

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions