Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

brawner
Copy link

@brawner brawner commented Mar 29, 2024

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 #830

Copy link

Review changes with SemanticDiff.

@jwellbelove
Copy link
Contributor

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
Copy link
Contributor

jwellbelove commented Apr 1, 2024

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
Copy link
Author

brawner commented Apr 1, 2024

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

@jwellbelove
Copy link
Contributor

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
Copy link
Author

brawner commented Apr 1, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants