ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.04k stars 371 forks source link

Validate allocated DataNode is not null to prevent gcc -Wnull-dereference (issue #830) #873

Closed brawner closed 2 months ago

brawner commented 3 months ago

gcc appears to warn about not checking for null when allocating these pointers. It is probably a false positive because clang does not issue the same warning, and optimizations can change how gcc issues this warning. Nonetheless, this is one approach to resolving the issue.

Addresses https://github.com/ETLCPP/etl/issues/830

semanticdiff-com[bot] commented 3 months ago

Review changes with SemanticDiff.

jwellbelove commented 3 months ago

There are errors in the CI Example

/home/runner/work/etl/etl/test/../include/etl/map.h:1226:32: error: comparison of distinct pointer types ('etl::map_base::Node *' and 'etl::imap<std::basic_string<char>, int, std::greater<std::basic_string<char>>>::Data_Node **')
      inserted = inserted_node == &node;
jwellbelove commented 3 months ago

I've investigated this some more and found a pattern to the warning. Many ETL containers use a function called allocate_data_node() to allocate a node from an etl::ipool. Only in the set/multiset/map/multimap containers does it return a reference; in all the others it returns a pointer. In all of the containers the function that calls it returns a reference (they don't seem to exhibit the warning), so the solution appears to be to just modify the allocate_data_node() in set/multiset/map/multimap. I've already applied this change to my issue branch and run the CI test set with -O2 optimisation.

brawner commented 3 months ago

@jwellbelove Did you want me to fix this PR, or are you handling it on your branch?

jwellbelove commented 3 months ago

I've tested my changes with my CI scripts here, so I'll merge them. Thanks for your PR though. All inputs are always appreciated.

brawner commented 3 months ago

I can confirm that your fixed version appears to work with my code as well.