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.
GSystemTextFactory::gSystemTextFileStreamreturns a heap-allocatedstd::ifstream*that callersclose()but neverdelete, leaking one stream per ASCII geometry/material load. The not-found /nullptrreturn 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:
Callers only close, never delete:
loadMaterials.cchas the identical pattern. There is nodelete INanywhere, so each call leaks astd::ifstream. (Thenullptrearly-returns atsystemTextFactory.cc:57,63don't leak because they return before/without a live owner mismatch — but they also discard a stream only in the not-found branch whereINwas already allocated, so they leak the allocatedINtoo.)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_ptrChange the factory to transfer ownership; callers then drop the manual
close()(RAII closes on scope exit).systemTextFactory.h(declaration):systemTextFactory.cc:(All three
return IN;sites move the sameunique_ptr; thereturn nullptr;sites are fine sinceunique_ptris nullptr-constructible.)loadGeometry.cc/loadMaterials.cc:Minimal alternative (if signature change is undesirable): keep the raw pointer but add
delete IN;afterIN->close();in both callers, and ensure the not-found branch in the factory deletes before returningnullptr. Theunique_ptrversion 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.