etl
etl copied to clipboard
ETLCPP
gcc warning: potential null pointer dereference [-Wnull-dereference] using etl::map::insert
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<TValueType>(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
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.
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.
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
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?
The STL containers will just throw a std::bad_alloc exception if the allocator fails.
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.
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.
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.
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;
}