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

Fixed various UBSan warnings about working with NULL pointers #2802

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Nov 15, 2023

Any pointer arithmetic with NULL pointers is technically UB, even if you don't end up dereferencing the pointer.

@seanm
Copy link
Contributor Author

seanm commented Nov 15, 2023

@DennisHeimbigner @WardF this is the last of #2692 split out.

I reformulated it to not use the ternary operator, as per your style.

Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what do to about this. utf8proc was taken from another site;
Unidata did not create the code. Perhaps we need to see if there is a later
version of utf8prop or find a better replacement.

@dopplershift
Copy link
Member

Looks like utf8proc is currently maintained by the Julia project. Not sure what it would take to update our internal copy, which looks like it's been manually namespaced with an nc_ prefix.

Any pointer arithmetic with NULL pointers is technically UB, even if you don't end up dereferencing the pointer.
@seanm
Copy link
Contributor Author

seanm commented Nov 15, 2023

I am not sure what do to about this. utf8proc was taken from another site;

I'll split the utf8proc change out of this PR in order to not delay this PR.

Maybe the issue is already fixed upstream. But we could still use my change in the short term, I think it's a quite conservative change.

@DennisHeimbigner
Copy link
Collaborator

I don't feel like replacing utf8proc, so fixing it seems like the right way to go.

@seanm
Copy link
Contributor Author

seanm commented Nov 15, 2023

I don't feel like replacing utf8proc, so fixing it seems like the right way to go.

New PR just for utf8: #2803

@ethanrd
Copy link
Member

ethanrd commented Nov 15, 2023

@DennisHeimbigner - It looks like utf8proc was pulled into netCDF-C back in February 2017 which would have been Unicode 9. Unicode is currently at version 15.1. Most of the new major versions added thousands of new characters. So, seems like it might be worth looking at a full update to the latest utf8proc at some point.

@WardF WardF merged commit 0c6fd78 into Unidata:main Nov 20, 2023
99 checks passed
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.

5 participants