jk-jeon / dragonbox

Reference implementation of Dragonbox in C++
Apache License 2.0
588 stars 37 forks source link

Uninitialized result structure can return incorrect value in may_have_trailing_zeros #27

Closed mrmixer closed 2 years ago

mrmixer commented 2 years ago

It seems that the result structure is never initialized, which means that the may_have_trailing_zeros can return whatever was on the stack if none of on_trailing_zeros or no_trailing_zeros functions are called.

I have this issue in a test program I made to compare my C port of the reference code. It happens with the following values, which returns true (in fact it's not 'true' but '5') for may_have_trailing_zeros in my test code (which means that you might not get the same result with a simple call with those values). That value comes from the compute_nearest_normal function on line 1799 where ret_value is declared but not initialized. The field is never written to after that.

using namespace jkj::dragonbox;

uint32_t i = 1283457033;
float f = *( float* )( &i );

auto v = to_decimal( f, policy::decimal_to_binary_rounding::nearest_to_even, policy::binary_to_decimal_rounding::do_not_care, policy::trailing_zero::report, policy::sign::return_sign, policy::cache::full );
jk-jeon commented 2 years ago

Oh, you are right! I should add no_trailing_zero right before the line https://github.com/jk-jeon/dragonbox/blob/02059bdee06beb9fea28a399e6463d1d1f8edbfa/include/dragonbox/dragonbox.h#L1845

Thanks a lot for catching this! I'll update shortly. Could you confirm with your port that this is the only spot that needs a fix regarding no-initialization of may_have_trailing_zeros?

mrmixer commented 2 years ago

I'm not sure it's the only place. I detected that one because the result of the reference code and my code were different (my code always initialize the result to 0) and I investigated. I suppose we could initialized ret_value to some know values and check that they aren't present in the final result. I'll get back to you if I find something.

jk-jeon commented 2 years ago

Yeah, I think I looked thoroughly at my code and I believe now the issue is fixed. Please let me know if you find anything wrong, thanks a lot again!