Skip to content

LGTM.com - false positive Multiplication result may overflow 'int' before it is converted to 'size_t'. #3015

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

Open
sandsmark opened this issue Mar 8, 2020 · 4 comments

Comments

@sandsmark
Copy link

It doesn't seem to understand bounds checking before conversion, or that the value returns with the thrown exception I guess:
https://lgtm.com/projects/g/sandsmark/genieutils/snapshot/83dc9ec17bc36ac8f77bbbf82fae9ffbbd6ceeca/files/src/resource/SmxFrame.cpp?sort=name&dir=ASC&mode=heatmap#L145

y is checked right before, m_normalHeader.width is checked before the loop (line 116).

@sandsmark
Copy link
Author

y is checked at the end of the loop as well (line 179), but I tested checking the size of the vectors it is compared to before the loop and it didn't help (though it would be the best, would be one less conditional in a semi-hot path even if the rest of the code there thrashes stuff).

@intrigus-lgtm
Copy link
Contributor

The C standard only guarantees that an int can take values in the range [-32767,+32767].
Therefore the multiplication could indeed overflow on an exotic platform.

@sandsmark
Copy link
Author

true, and I expect getting a static analyzer to remember something like static_assert(std::numeric_limits<int>::max() >= 2147483647ull && "We assume a sane int size in bounds checking"); or something is a bit unrealistic.

@jbj jbj added the C++ label Mar 9, 2020
@jbj
Copy link
Contributor

jbj commented Mar 9, 2020

Thanks for the report. I think the main problem for the range analysis is that m_normalHeader.width is not tracked as a value because it's a field of an object. Only local variables are tracked, so the guard is simply invisible to the analysis.

We have most of the building blocks we'd need for fixing this, but realistically it's not on the roadmap to put them together in this particular way. I'll keep this bug open in case we get reports of similar issues and want to group them.

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

No branches or pull requests

3 participants