Skip to content

fix(eo): fix null pointer and dangling pointer bugs in eoForge#89

Open
evvaletov wants to merge 1 commit intonojhan:masterfrom
evvaletov:fix/eoforge-bugs
Open

fix(eo): fix null pointer and dangling pointer bugs in eoForge#89
evvaletov wants to merge 1 commit intonojhan:masterfrom
evvaletov:fix/eoforge-bugs

Conversation

@evvaletov
Copy link

Summary

Two bugs in eo/src/eoForge.h:

1. eoForgeOperator no-arg specialization: instantiate_ptr() returns null

The partial specialization for constructors without arguments (line 206)
constructs an empty shared_ptr instead of allocating a new Op:

// Before: default-constructs a null shared_ptr
_instantiated_ptr = std::shared_ptr<Op>();

// After: actually creates an Op instance
_instantiated_ptr = std::make_shared<Op>();

The full-args overload correctly uses std::make_shared<Op>(...) via
op_constructor_ptr, so this is an inconsistency in the no-arg
specialization.

2. eoForgeMap::setup(): dangling pointer and memory leak

After delete this->at(name), the key remains in the map pointing to
freed memory. std::map::emplace does not insert when the key already
exists, so:

  • The map retains the dangling pointer to the deleted object
  • The newly allocated pfo leaks
// Before: emplace silently no-ops on existing key
this->emplace({name, pfo});

// After: overwrites the (now-dangling) pointer with the new allocation
this->at(name) = pfo;

Both setup() overloads (with-args and no-args) had the same bug.
Note that eoForgeVector::setup() already does this correctly via
this->at(index) = pfo.

Changes

File Lines
eo/src/eoForge.h 3 lines changed

Test plan

  • All 191 tests pass, including t-operator-forge

eoForgeOperator no-arg specialization: instantiate_ptr() constructed an
empty shared_ptr instead of a new Op instance, always returning nullptr.
Fix: std::shared_ptr<Op>() → std::make_shared<Op>().

eoForgeMap::setup(): after deleting the old operator, emplace() silently
no-ops when the key already exists, leaving a dangling pointer in the map
and leaking the new allocation. Fix: assign through at() instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant