Fix possible UB in the sum_string_vector function (TypeTools.hpp) #893
Conversation
include/CLI/TypeTools.hpp
Outdated
| if(val <= static_cast<double>((std::numeric_limits<std::int64_t>::min)()) || | ||
| val >= static_cast<double>((std::numeric_limits<std::int64_t>::max)()) || | ||
| std::ceil(val) == std::floor(val)) { | ||
| if(val > static_cast<double>((std::numeric_limits<std::int64_t>::min)()) && |
There was a problem hiding this comment.
If I am reading this correctly this will not print correctly for (std::numeric_limits<std::int64_t>::min)() or (std::numeric_limits<std::int64_t>::max)() not sure the previous one was completely correct after looking at it again though either. Also not entirely sure how a max value int64 gets translated to a double so it may be moot and this is actually correct for all possible values.
There was a problem hiding this comment.
I think that double's precision is not enough to hold std::numeric_limits<std::int64_t>::max() or std::numeric_limits<std::int64_t>::min(). I've tried using >=/<=, UB happens: https://cpp.godbolt.org/z/eKc4qP9EW
There was a problem hiding this comment.
Thinking about that a bit more, any integer over the mantissa limit of a double is guaranteed to be an integer. We could change it like this https://cpp.godbolt.org/z/Ynb4E6MxG otherwise there is a bit of gap where std::ceil(val) == std::floor(val) is guaranteed to be true and I am not sure the condition does really what is intended.
There was a problem hiding this comment.
I thought about it for a moment, why don't specialize detail::value_string for doubles to be like https://cpp.godbolt.org/z/rT5vE68qK? The reason for casting to int64_t is to eliminate floating-point part, but that [casting] is unnecessary. Then we could remove that if and just pass doubles always, and they will be converted to string with or without floating point part. And the resulting detail::value_string is still correct since the output string will hold the same value, just without fp part.
There was a problem hiding this comment.
the potential issue is that it would be possible to have it convert to an integer that overloads a subsequent conversion operation, which is why I think the checks on integer max/min were in place. Going to have to play with it a bit more to know if this is actually a problem or not. with the last godbolt you have potential to get 300+ character strings with big scientific notation numbers.
There was a problem hiding this comment.
I understand that converting to int64_t was to use a specific overloaded function, I just don't think that the most elegant way to do it.
I agree, that 300-digit number would not look very good, that part I missed. So, ok, from the wiki:
The 53-bit significand precision gives from 15 to 17 significant decimal digits
So we could just limit precision to let's say 16 (or 19 to preserve previous behavior?) (e.g. number is the thing to discuss), and then it works as expected: https://cpp.godbolt.org/z/Tqv76d4qe.
There was a problem hiding this comment.
The reason that was done is because the function in question needed to generate a string that can if at all possible be read by the integer string conversions further down the line, so the strings generated needed to be in a format that could be read into an int64_t type without errors. I think the precision 16 is a viable solution there. I am not sure we want to specialize value_string to the new operation since the purpose of the code in question is slightly different than the value_string methods. But replacing what's in the else block with the stringstream code should meet the purpose and avoid any UB.
There was a problem hiding this comment.
Updated branch to use strinstream as in godbolt example. Also one test was failing with cool output:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
AppTest is a Catch v2.13.10 host application.
Run with -? for options
-------------------------------------------------------------------------------
SumOptFloat
-------------------------------------------------------------------------------
/tech/projects/CLI11/tests/AppTest.cpp:822
...............................................................................
/tech/projects/CLI11/tests/AppTest.cpp:831: FAILED:
CHECK( 0.6 == val )
with expansion:
0.6 == 0.6
===============================================================================
test cases: 161 | 160 passed | 1 failed
assertions: 497 | 496 passed | 1 failed
Using epsilon for comparison fixed it.
// I believe there should be more warnings enabled, at least -Wfloat-equal for those kinds of things (but that is completely different topic).
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #893 +/- ##
==========================================
- Coverage 99.46% 99.46% -0.01%
==========================================
Files 18 18
Lines 4096 4095 -1
==========================================
- Hits 4074 4073 -1
Misses 22 22
☔ View full report in Codecov by Sentry. |
|
@all-contributors please add @trokhymchuk for code |
|
I've put up a pull request to add @trokhymchuk! 🎉 |
|
@trokhymchuk are you good with this PR? anything else you think needs changing? if not I will merge it. |
I think everything is ok, I don't see anything that needs to be changed (tests are passing, and I don't see any mistakes in the code), at least for now. If you have some ideas, feel free to say them. |
|
I will try adding that float flag see what else pops up but that will be a different effort. Thanks! |
Adds @trokhymchuk as a contributor for code. This was requested by phlptp [in this comment](#893 (comment)) [skip ci] --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Fixes #892
(+ uses safe comparison for double values via epsilon)
https://cpp.godbolt.org/z/Yfje6encG