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

Safer casting from double to long in value.hpp #1831

Merged
merged 2 commits into from
Aug 3, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes: #1830

I think it's easiest to require the value to be in the INT_MIN to INT_MAX range. It means that we're not using the full range of long on platforms with 64-bit long, but it means that the cast won't produce different results on platforms where long is 32 bits.

I have been trying to avoid changing the header files on 0.27-maintenance, but I can't see another solution for this bug. Is this change going to cause any problems?

@kevinbackhouse kevinbackhouse added bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify labels Aug 2, 2021
@kevinbackhouse kevinbackhouse added this to the v0.27.5 milestone Aug 2, 2021
Copy link
Member

@hassec hassec left a comment

Choose a reason for hiding this comment

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

Approving this, since I also don't know a better way to fix this without introducing these specializations in the header.

👍

@kevinbackhouse kevinbackhouse merged commit 2cf0159 into Exiv2:0.27-maintenance Aug 3, 2021
kevinbackhouse added a commit that referenced this pull request Aug 3, 2021
Safer casting from double to long in value.hpp (backport #1831)
@clanmills clanmills mentioned this pull request Aug 9, 2021
@Floessie
Copy link

@kevinbackhouse Hi Kev, is there a reason (except for brevity) to use INT_(MIN|MAX) instead of the more C++ish std::numeric_limits<int>::(min|max)()?

@kevinbackhouse
Copy link
Collaborator Author

@Floessie: I think brevity was my only reason. To be honest though, I was also just trying to fix the UBSAN issue in the simplest way possible, without changing existing behavior. Limiting the range to INT_(MIN|MAX) is a quick fix, not a proper solution. On the main branch, I would like to investigate why we are casting from double to long, and whether we can restrict it to much tighter bounds. For example, are we using this to calculate a percentage, where the expected range is 0..100?

@kevinbackhouse kevinbackhouse deleted the FixIssue1830 branch August 11, 2021 14:45
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 float to long in ValueType<float>::toLong()
3 participants