public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug libstdc++/114401] New: libstdc++ allocator destructor omitted when reinserting node_handle into tree- and hashtable-based containers
@ 2024-03-20  8:35 jblair149 at gmail dot com
  2024-03-20 11:23 ` [Bug libstdc++/114401] " redi at gcc dot gnu.org
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: jblair149 at gmail dot com @ 2024-03-20  8:35 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114401

            Bug ID: 114401
           Summary: libstdc++ allocator destructor omitted when
                    reinserting node_handle into tree- and hashtable-based
                    containers
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jblair149 at gmail dot com
  Target Milestone: ---

Created attachment 57742
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57742&action=edit
Source code for test program demonstrating the issue with allocator destruction
on node handle insertion.

Summary:
Implementation in the following functions that reinsert nodes into `_Rb_tree`
and `_Hashtable` may set the node handle data pointer to `nullptr`. This
prevents stateful allocators from being properly destructed when extracting and
reinserting nodes.


Functions in stl_tree.h impacted:
`insert_return_type _Rb_tree_impl::_M_reinsert_node_unique(node_type&& __nh)`

`iterator _Rb_tree_impl::_M_reinsert_node_equal(node_type&& __nh)`

`iterator _Rb_tree_impl::_M_reinsert_node_hint_unique(const_iterator __hint,
node_type&& __nh)`

`iterator _Rb_tree_impl::_M_reinsert_node_hint_equal(const_iterator __hint,
node_type&& __nh)`


Functions in hashtable.h impacted:
`insert_return_type _Hashtable::_M_reinsert_node_unique(node_type&& __nh)`

`insert_return_type _Hashtable::_M_reinsert_node_multi(const_iterator __hint,
node_type&& __nh)`


Details:
The allocator of the node is asserted to be equal to the allocator of the
underlying tree or hashtable in each function, but otherwise the allocator is
unused, and on successful insertion `__nh._M_ptr` is set to `nullptr`.
Following that, in the destructor of `_Node_handle_common`, `_M_reset()` is
never called since the node is detected as `empty()`. Further, the union
`_Optional_alloc` defined within `_Node_handle_common` has a no-op destructor,
so only a manual invocation of `_Optional_alloc::release()` can trigger the
destructor of the allocator contained, which may only be invoked if active. As
a result, the allocator instance within the node is never destructed once the
node has been reinserted into the tree or hashtable. This doesn't matter for
stateless allocators, but for stateful ones this can result in a memory leak.

One potential solution is to:
1) Add a new method to `_Node_handle_common` that will be called after a node
is reinserted rather than simply setting `_M_ptr` to `nullptr`. That method
would invoke `_M_alloc.release()` like `_M_reset()` does, and set the pointer
`_M_ptr` to `nullptr`, but omit the actions to destroy the object and
deallocate the memory since the node has been returned to a compatible
container.

2) Within each of the functions mentioned within the files above, replace
`__nh._M_ptr = nullptr;` with an invocation of the function in (1).

I do not know for certain this is the maximal extent of the issue when
reinserting extracted nodes, but I've expanded from my initial issue using
`std::set` to the associative containers I'm aware of which expose
`node_handle`. I also am not familiar with the naming conventions within the
library. To that end, I'm omitting a patch since this may require discussion.


I believe the current master branch of GCC exhibits the issue; I've confirmed
it is present at least as of commit 994d8f922b9d88f45775f57a490409ab1c3baf59.
This issue is likely present in past releases as well, but I don't know how
that should be handled.

Within the repository, the files I know are impacted are:
gcc.git/libstdc++-v3/include/bits/node_handle.h
gcc.git/libstdc++-v3/include/bits/stl_tree.h
gcc.git/libstdc++-v3/include/bits/hashtable.h


I'm attaching a relatively minimal program which demonstrates the issue. The
program uses a logging, stateful allocator within a `std::set`, inserts
elements, extracts a node, modifies the value of the element within the node,
reinserts the node, and triggers the destruction of the set by the end of
scope. Compilation of the program is as follows:

g++ test_node_reinsert.cpp -o test_node_reinsert

The test program may be run without arguments.


The output of this program prior to the fix:

Begin scope for allocator and set.
initial construction
allocate memory pools
allocator copied; active count = 2
allocator copied; active count = 3
allocator copied; active count = 4
allocator destructed; active count = 3
allocator destructed; active count = 2
allocator copied; active count = 3
End scope for allocator and set.
allocator destructed; active count = 2
allocator destructed; active count = 1
Allocator should be completely desctructed.


The output of this program after my (hacky?) fix, following the instructions
above:

Begin scope for allocator and set.
initial construction
allocate memory pools
allocator copied; active count = 2
allocator copied; active count = 3
allocator copied; active count = 4
allocator destructed; active count = 3
allocator destructed; active count = 2
allocator copied; active count = 3
allocator copied; active count = 4
allocator destructed; active count = 3
allocator destructed; active count = 2
End scope for allocator and set.
allocator destructed; active count = 1
allocator destructed; active count = 0
deallocate memory pools
fully destructed
Allocator should be completely desctructed.

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-03 10:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-20  8:35 [Bug libstdc++/114401] New: libstdc++ allocator destructor omitted when reinserting node_handle into tree- and hashtable-based containers jblair149 at gmail dot com
2024-03-20 11:23 ` [Bug libstdc++/114401] " redi at gcc dot gnu.org
2024-03-20 14:46 ` redi at gcc dot gnu.org
2024-03-20 14:49 ` redi at gcc dot gnu.org
2024-03-21 12:06 ` redi at gcc dot gnu.org
2024-03-22 22:51 ` cvs-commit at gcc dot gnu.org
2024-03-22 22:55 ` redi at gcc dot gnu.org
2024-04-03 10:46 ` cvs-commit at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).