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

[glibutil] Check for integer overflow. SX#GULBI-122 #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martyone
Copy link
Member

This makes the Coverity tool quiet about this rather false positive issue.

This makes the Coverity tool quiet about this rather false positive
issue.
@martyone martyone requested a review from monich November 15, 2024 08:36
@@ -252,6 +252,7 @@ gutil_strv_remove(
len--;
l = len - pos;
while ((i = gutil_strv_find_last_impl(sv + pos, s, l)) >= 0) {
g_assert(pos + i >= 0);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what's this g_assert supposed to do? If the assertion becomes false, then hitting the assert would crash the program (even the production build). If the assertion is always true then this statement never does anything. In any case this doesn't fix the actual problem (if there is a problem).

I don't like the idea of having g_asserts in the production code.

Copy link
Member Author

Choose a reason for hiding this comment

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

With this assert in place Coverity sees that the code cannot reach the next line, should an integer overflow happen for (pos + i). It is unlikely to happen that so big a vector would be used, but at the same time it would clearly be wrong to state that this is a false positive. Hence the preference of assert over the coverity[overflow_const:FALSE] annotation.

As to the possibility of crashing the program in production build. Would you prefer to let it continue with corrupted memory instead?

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