Skip to content

Fix igraph_t ownership issues in NetlistGraph - #631

Open
julianspeith wants to merge 1 commit into
masterfrom
fix/netlist-graph-igraph-ownership
Open

Fix igraph_t ownership issues in NetlistGraph#631
julianspeith wants to merge 1 commit into
masterfrom
fix/netlist-graph-igraph-ownership

Conversation

@julianspeith

Copy link
Copy Markdown
Contributor

Two related memory-safety defects around NetlistGraph's igraph_t member.

  1. Destroying an uninitialized igraph_t.

    NetlistGraph(Netlist*) initializes only m_nl, deliberately leaving m_graph to be filled in afterwards by the factory that called it (igraph_create() in from_netlist(), igraph_empty() in from_netlist_no_edges(), igraph_copy() in copy()). The destructor, however, called igraph_destroy(&m_graph) unconditionally.

    When a factory bails out between construction and that initialization, the std::unique_ptr holding the half-constructed graph unwinds and igraph_destroy() runs over uninitialized memory. Reachable paths:

    • from_netlist(): constructed at the top, returns early if igraph_vector_int_init() for the edge vector fails, well before igraph_create() is reached.
    • copy(): constructed at the top, returns early if igraph_copy() itself fails.

    Both require an igraph allocation to fail, so this does not show up in
    normal runs, but it is a genuine crash path rather than a leak.

    Fixed by tracking whether m_graph was actually initialized and having
    the destructor honour that. The flag is set only after the respective
    igraph call has reported success. m_graph_ptr is now default-initialized
    to nullptr as well, since the same constructor left it dangling.

  2. NetlistGraph was implicitly copyable.

    igraph_t is a plain C struct holding heap pointers, so the implicitly generated copy constructor and copy assignment operator bitwise-copied m_graph and left two NetlistGraph objects aliasing the same graph internals; the destructor would then igraph_destroy() them twice. The copy's m_graph_ptr would additionally point into the source object.

    Nothing copies a NetlistGraph today -- every factory hands out a std::unique_ptr, the python bindings hold it via RawPtrWrapper and pass it by const reference, and callers wanting a duplicate use the explicit copy() method -- so this was latent. Deleting the copy operations enforces that invariant at compile time instead of leaving it to convention.

Verified by syntax-checking all 14 translation units under plugins/graph_algorithm and plugins/hawkeye (including both python binding files) against the modified header; all compile unchanged. Both files are clang-format clean.

Two related memory-safety defects around NetlistGraph's igraph_t member.

1. Destroying an uninitialized igraph_t.

   NetlistGraph(Netlist*) initializes only m_nl, deliberately leaving
   m_graph to be filled in afterwards by the factory that called it
   (igraph_create() in from_netlist(), igraph_empty() in
   from_netlist_no_edges(), igraph_copy() in copy()). The destructor,
   however, called igraph_destroy(&m_graph) unconditionally.

   When a factory bails out between construction and that initialization,
   the std::unique_ptr holding the half-constructed graph unwinds and
   igraph_destroy() runs over uninitialized memory. Reachable paths:

     - from_netlist(): constructed at the top, returns early if
       igraph_vector_int_init() for the edge vector fails, well before
       igraph_create() is reached.
     - copy(): constructed at the top, returns early if igraph_copy()
       itself fails.

   Both require an igraph allocation to fail, so this does not show up in
   normal runs, but it is a genuine crash path rather than a leak.

   Fixed by tracking whether m_graph was actually initialized and having
   the destructor honour that. The flag is set only after the respective
   igraph call has reported success. m_graph_ptr is now default-initialized
   to nullptr as well, since the same constructor left it dangling.

2. NetlistGraph was implicitly copyable.

   igraph_t is a plain C struct holding heap pointers, so the implicitly
   generated copy constructor and copy assignment operator bitwise-copied
   m_graph and left two NetlistGraph objects aliasing the same graph
   internals; the destructor would then igraph_destroy() them twice. The
   copy's m_graph_ptr would additionally point into the source object.

   Nothing copies a NetlistGraph today -- every factory hands out a
   std::unique_ptr<NetlistGraph>, the python bindings hold it via
   RawPtrWrapper and pass it by const reference, and callers wanting a
   duplicate use the explicit copy() method -- so this was latent. Deleting
   the copy operations enforces that invariant at compile time instead of
   leaving it to convention.

Verified by syntax-checking all 14 translation units under
plugins/graph_algorithm and plugins/hawkeye (including both python
binding files) against the modified header; all compile unchanged. Both
files are clang-format clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@julianspeith
julianspeith requested a review from nils1603 as a code owner July 31, 2026 09:24
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