[Bug libstdc++/108554] New: Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[Bug libstdc++/108554] New: Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[fcontact at cuveland dot de]

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

            Bug ID: 108554
           Summary: Warning "null pointer dereferece" raised when
                    extracting a unique_ptr from a map and any "-O" flag
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fcontact at cuveland dot de
  Target Milestone: ---

I found what I believe might be a false positive with g++ 12 when using the
"-Wnull-dereference" warning and a map of unique_ptr with any level of
optimisation.

This behavior does not show up in GCC 9, 10, and 11.

Small but complete example:

#include <map>
#include <memory>
#include <string>
#include <utility>

int main()
{
  // create and initialize a map
  std::map<std::string, std::unique_ptr<int>> my_map;
  my_map["my_key"] = std::make_unique<int>(1);

  auto it = my_map.find("my_key");
  if (it != my_map.end()) {
    // extract an item from the map
    auto item = std::move(my_map.extract(it).mapped());
    return *item;
  }
  return 0;
}

Compiled with:
g++-12 -O -Wnull-dereference <filename>

Compiler version:
g++ (Compiler-Explorer-Build-gcc--binutils-2.38) 12.2.0
(see https://godbolt.org/z/E9KhTT4f6)

[Bug libstdc++/108554] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[redi at gcc dot gnu.org]

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

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jan de Cuveland from comment #0)
> This behavior does not show up in GCC 9, 10, and 11.

It does if you use -Wsystem-headers -- the only thing that changed is that the
compiler now ignores whether the location is in a system header for many of
warnings with the most false positives.

[Bug libstdc++/108554] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[fcontact at cuveland dot de]

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

--- Comment #2 from Jan de Cuveland <fcontact at cuveland dot de> ---
Thanks for the insight!

Adding a check on empty() for my_map.extract(it) resolves the issue.

So it is just that the static analyzer does not see that (to my understanding)
"it" has to be a valid iterator and thus will always result in a non-empty node
handle.

[Bug libstdc++/108554] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-01-26
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This patch fixes it:

--- a/libstdc++-v3/include/bits/stl_tree.h
+++ b/libstdc++-v3/include/bits/stl_tree.h
@@ -405,12 +405,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Base_ptr _M_node;
     };

+  __attribute__((__nonnull__))
   void
   _Rb_tree_insert_and_rebalance(const bool __insert_left,
                                _Rb_tree_node_base* __x,
                                _Rb_tree_node_base* __p,
                                _Rb_tree_node_base& __header) throw ();

+  __attribute__((__nonnull__,__returns_nonnull__))
   _Rb_tree_node_base*
   _Rb_tree_rebalance_for_erase(_Rb_tree_node_base* const __z,
                               _Rb_tree_node_base& __header) throw ();

[Bug libstdc++/108554] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jan de Cuveland from comment #2)
> So it is just that the static analyzer does not see that (to my
> understanding) "it" has to be a valid iterator and thus will always result
> in a non-empty node handle.

I thought that was the case, but it's actually a little more complicated. There
are some non-inline functions used by std::map when rebalancing the tree, and
the compiler doesn't know that they never return a null pointer (assuming valid
inputs, i.e. no past-the-end iterators, which is a precondition of those
functions anyway).

The patch above uses the returns_nonnull attribute so the compiler knows that
we always get a valid pointer back from those functions.

[Bug libstdc++/108554] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3376467ce090aa0966d59ca3aea35db4f17a4b47

commit r13-5386-g3376467ce090aa0966d59ca3aea35db4f17a4b47
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jan 26 10:55:28 2023 +0000

    libstdc++: Add returns_nonnull to non-inline std::map detail [PR108554]

    std::map uses a non-inline function to rebalance its tree and the
    compiler can't see that it always returns a valid pointer (assuming
    valid inputs, which is a precondition anyway). This can result in
    -Wnull-derefernce warnings for valid code, because the compiler thinks
    there is a path where the function returns null.

    Adding the returns_nonnull attribute tells the compiler that is can't
    happen. While we're doing that, we might as well also add a nonnull
    attribute to the rebalancing functions too.

    libstdc++-v3/ChangeLog:

            PR libstdc++/108554
            * include/bits/stl_tree.h (_Rb_tree_insert_and_rebalance): Add
            nonnull attribute.
            (_Rb_tree_rebalance_for_erase): Add nonnull and returns_nonnull
            attributes.
            * testsuite/23_containers/map/modifiers/108554.cc: New test.

[Bug libstdc++/108554] [12 Regression] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |11.3.0
      Known to fail|                            |12.2.0
   Target Milestone|---                         |12.3
            Summary|Warning "null pointer       |[12 Regression] Warning
                   |dereferece" raised when     |"null pointer dereferece"
                   |extracting a unique_ptr     |raised when extracting a
                   |from a map and any "-O"     |unique_ptr from a map and
                   |flag                        |any "-O" flag

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk, but this should be backported to gcc-12 too.

The warning is present on older branches but suppressed by default, so it's not
important to backport before to older branches.

[Bug libstdc++/108554] [12 Regression] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3adf6dd508491d26e21840a8a70b016f876edd53

commit r13-5454-g3adf6dd508491d26e21840a8a70b016f876edd53
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jan 27 11:28:37 2023 +0000

    libstdc++: Use dg-bogus in new test [PR108554]

    libstdc++-v3/ChangeLog:

            PR libstdc++/108554
            * testsuite/23_containers/map/modifiers/108554.cc: Use dg-bogus.

[Bug libstdc++/108554] [12 Regression] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:2fdfa3768b25c85df39eaf9b850e130e42a4dd6f

commit r12-9345-g2fdfa3768b25c85df39eaf9b850e130e42a4dd6f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jan 26 10:55:28 2023 +0000

    libstdc++: Add returns_nonnull to non-inline std::map detail [PR108554]

    std::map uses a non-inline function to rebalance its tree and the
    compiler can't see that it always returns a valid pointer (assuming
    valid inputs, which is a precondition anyway). This can result in
    -Wnull-derefernce warnings for valid code, because the compiler thinks
    there is a path where the function returns null.

    Adding the returns_nonnull attribute tells the compiler that is can't
    happen. While we're doing that, we might as well also add a nonnull
    attribute to the rebalancing functions too.

    libstdc++-v3/ChangeLog:

            PR libstdc++/108554
            * include/bits/stl_tree.h (_Rb_tree_insert_and_rebalance): Add
            nonnull attribute.
            (_Rb_tree_rebalance_for_erase): Add nonnull and returns_nonnull
            attributes.
            * testsuite/23_containers/map/modifiers/108554.cc: New test.

    (cherry picked from commit 3376467ce090aa0966d59ca3aea35db4f17a4b47)

[Bug libstdc++/108554] [12 Regression] Warning "null pointer dereferece" raised when extracting a unique_ptr from a map and any "-O" flag

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 12.3