gcc warning: potential null pointer dereference [-Wnull-dereference] using etl::map::insert

[Forlautboy]

Hi,

I have a question regaring an occuring compiler warning.

Compiling with -O2 the gcc warns about potential null pointer dereference [-Wnull-dereference] using the function etl::map::insert.

As far is I understand the warning is raised from Line 1500 of map.h:
::new (&node.value) value_type(value);
Disabeling the warning arount the insert statement has no effect since the warning arises from an instantiated template.

Similar behaviour occures using flat_map but in lien 1032 of flat_map.h:
::new (pvalue) value_type(etl::forward(value));

I just wanted to ask if sombody faced the same effect and maybe has some tips for coming around.
Finally I am just not sure if its really a problem of if gccs warning is just a false positive.

We are using gcc 12.2.0 and etl v20.38.10 with ETL_THROW_EXCEPTIONS disabled.

Kind Regards,

Tobias

[jwellbelove]

When exceptions are not used GCC is expecting that the error handler could return, thereby propagating a nullptr. As insert can return a indication that the value was not inserted the code should probably be refactored to allow a failed allocation (i.e. pool empty) to result in valid code with the appropriate return value rather than expecting a 'halt' from the error handler.

[Forlautboy]

Ok, I understood. So you suggest that if the error_handler returns, instead of producing undefined behaviour by accessing nullpointer, map::insert to return a nulliterator and false for inserted? That sounds promising. The Standard function std::map::insert returns inserted = false for the case that the key is already present in the map. So the combination of nulliterator and inserted = false for a insert on a full map would still distinguish from the case that the key is already prensent.
I think that could fix our problem. I will try this on a locally and provide further feedback.

[Forlautboy]

I tested your suggestion. Although it eliminates the undefined behavior the warnings sill arise. I am still not certain which "pointer" the compiler thinks of dereferencing. I am even a bit suspicious that the warning considers some internal magic of the new-statement.

/usr/include/etl/map.h:1520:7: warning: potential null pointer dereference [-Wnull-dereference]
1520 | ::new (&node.value) value_type(value);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

=> Line 1500 in unmodified code

/usr/include/etl/map.h:1545:7: warning: potential null pointer dereference [-Wnull-dereference]
1545 | ::new (&node.value) value_type(etl::move(value));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

=> Line 1525 in unmodified code

[jwellbelove]

The warning is there because Data_Node& allocate_data_node() would create an invalid reference if the map were full.

I need to look again at the library and rethink how errors are handled. The containers especially should be left in a known state if, say, an assign tries to over fill a container.
i.e. Does assign call the error handler and then partially fill the container, leave the original data untouched or clear it?

[jwellbelove]

The STL containers will just throw a std::bad_alloc exception if the allocator fails.

[jwellbelove]

I have raised my own issue #831 where I want to change over to using the error handler in the unit tests, as apposed to exceptions.

[Forlautboy]

Thank you very much. I think this improved my understanding of the problem.
Also changing the unit test to use the error_handler sounds clearly beneficial to me.

[brawner]

Here is a simple example to replicate the issue: https://godbolt.org/z/roYMb7GEb . It also occurs in etl::unordered_map and etl::flat_map and the resolutions are all similar: add a check that the allocated pointer is not null before using it.

I think with optimizations turned on the compiler has trouble connecting the logic for ensuring the container is not full and that the allocated pointer won't be null.

Changing create_data_node and allocate_data_node to return pointers instead of references and checking against a nullptr further up in the call stack helped me solve the issue.

[jwellbelove]

I've so far been unable to replicate the error in the unit tests with 20.38.10, using the test below, but if I paste the body of the test into Godbolt (with the same compiler and options) I do get the error!

    TEST(test_issue_830_potential_null_pointer_dereference)
    {
      // Should compile without 'potential null pointer dereference'
      char data[10U] = { 'a', 'b',  'c',  'd',  'e',  'f',  'g',  'h',  'i',  'j' };
      char* p = data;

      auto m = etl::map<int, char*, 10U>{};

      for (int i = 0; i < 10; i++)
      {
        m[i] = p + i;
      }

      // Use 'm'
      (void) m;
    }

[brawner]

One possible solution: #873

[jwellbelove]

Fixed 20.38.11