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

Check value is in range before casting from double to uint32_t #1828

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes: #1827

The static_cast<long> can cause a UBSAN failure if the value of tmp is out of range. It was also incorrect to cast to long, because the type of ur.second is uint32_t.

@kevinbackhouse kevinbackhouse added bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify labels Aug 2, 2021
@kevinbackhouse
Copy link
Collaborator Author

I wonder if we should use nearbyint for the rounding on the main branch. (Can't use it on 0.27-maintenance because it's a C++11 feature.) C++11 also has an lround function, but it doesn't seem to be any better at handling out-of-range values, and it's return type is long which isn't platform independent.

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2021

This pull request introduces 1 alert when merging 1455cc8 into b364b1d - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

src/tags_int.cpp Outdated Show resolved Hide resolved
@hassec
Copy link
Member

hassec commented Aug 2, 2021

👍 for using C++11 features in main.

Any reason to use nearbyint over just std::round? std::round would give use the rounding behavior we have implemented right now, while nearbyint would depend on the rounding mode that is set

@kevinbackhouse kevinbackhouse linked an issue Aug 2, 2021 that may be closed by this pull request
@kevinbackhouse kevinbackhouse added this to the v0.27.5 milestone Aug 2, 2021
Co-authored-by: Christoph Hasse <hassec@users.noreply.github.com>
@kevinbackhouse
Copy link
Collaborator Author

@hassec: you're right, std::round is better than std::nearbyint. I overlooked it when I was reading about std::lround!

@kevinbackhouse kevinbackhouse merged commit ca32016 into Exiv2:0.27-maintenance Aug 3, 2021
kevinbackhouse added a commit that referenced this pull request Aug 3, 2021
Check value is in range before casting from double to uint32_t (backport #1828)
@clanmills clanmills mentioned this pull request Aug 9, 2021
@kevinbackhouse kevinbackhouse deleted the FixIssue1827 branch August 11, 2021 14:45
@clanmills clanmills mentioned this pull request Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined behavior in cast from double to long
2 participants