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

Uninitialized result structure can return incorrect value in may_have_trailing_zeros #27

Closed
mrmixer opened this issue Feb 26, 2022 · 3 comments

Comments

@mrmixer
Copy link

mrmixer commented Feb 26, 2022

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
Copy link
Owner

jk-jeon commented Feb 26, 2022

Oh, you are right! I should add no_trailing_zero right before the line

return ret_value;

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
Copy link
Author

mrmixer commented Feb 26, 2022

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
Copy link
Owner

jk-jeon commented Feb 26, 2022

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!

yotann added a commit to yotann/bcdb-private that referenced this issue Mar 9, 2022
9c26179a Fix jk-jeon/dragonbox#27
02059bde Fix a typo
57d9b60c Fix a bug
ba65f0ef Add missing copyright notice
fcc6cc0e Improve comment
29aa8a23 Improve comment
fa139ad9 Help MSVC to not generate horrible assembly
478edcc4 Fix ABI overhead issue
d4f164bd Improve comment
a84c11e1 A small optimization turning mul into lea
6bf01402 Add the old paper along with the new one
955d7387 Rerun Milo's benchmark
77aeee25 Refactoring + some improvement on 9-digits case
b75d296c Optimize digit printing algorithm
8864215b Merge branch 'master' of https://github.com/jk-jeon/dragonbox
dd52aa72 Workaround clang-format & correctly attribute jeaiii
71684f08 Update README.md
21400aab Rewrite paper
3da7d9d8 Change <= epsilon to < epsilon, as that's what we need
4b1f309d Update README.md
fefd03b4 Rerun Milo's benchmark
5ca7e3d2 Change the order of condition evaluations
5ddb8bcf Add more const; reformat
b4d5f705 Add test for compressed cache
fb970736 Fix incorrect analysis; now it is correct.
fe2e5c1e Rename beta_minus_1 to beta to reflect the  new paper
2b673c0c Rerun benchmark
3f33d4dc Run MATLAB even when no actual benchmark has been done
4d82dfe4 Fix a bug that shaded regions are not correctly shown in the exported pdf
e52fe78c Use better exportgraphics rather than print
f1e59661 Merge branch 'master' of https://github.com/jk-jeon/dragonbox
2e6eefa5 Update README.md
0377186d Update README.md
38e11bc4 Rerun verify_cache_precision
07a450ed Ignore pdf files generated from plot_required_bits.m
ef4ac121 Required bits plot MATLAB script
d4f0f783 Reflect the new paper & add file output
eff5914f Reflect the new paper
bba1a8b7 Merge branch 'master' of https://github.com/jk-jeon/dragonbox
a37c3c28 Add missing test cases. No problem found.
96dc15c8 Update README.md
d41e062b Fix a bug
83d5df3e Reflect the new paper, simplify things
692f67f4 Implement jeaiii-style printing algorithm
cd533cde Both binary32 and binary64 remove trailing zeros
af08b920 Fix an error (has no effect in fact)
133f0aaf Match new paper
f5ff2c4e Shall not feed 0 to dragonbox
2537b740 Further simplify check_divisibility_and_divide_by_pow10
4cece761 Even more simplified version of check_divisibility_and_divide_by_pow10
274d4634 Use newly implemented find_all_good_rational
990b2a4f Add file
028083c8 Rename
92001c41 Implement find_all_good_rational_approx_from_below_denoms
2ce3f894 Add some utilities to unsigned_rational
15e6a56d Add a little more utility functions
997a4fda Fix typos and improve some sentences
deabce23 Fix typos, improve some sentences, correct some small errors
2ee87104 Improve comments
812646d3 Improve comments
8c484269 Include missed cases
72e22a33 Include possibily missed cases
66de8584 Add template keyword here and there
1c1ca394 More precise test
dc41a327 Refine false positive condition
3e1083a2 Remove generate_compressed_cache_error_table
938a3ed6 Implement compressed cache verification
259787da Simplify compressed cache policy
569980d5 Justify why we don't need to add the error back.
d7e8e307 Fix errors
23c2c20b Carefully re-done
f75e50af Reformat
1f452fa8 Add is_even()
4a3402db Fix error
369cd0bc Improve comments and other conventions
30d9e4eb Simplify check_divisibility_and_divide_by_pow10. Apply the integer check idea from Schubfach.
4150375b Right-shift for signed integers is implementation-defined See fmtlib/fmt#2709
1c7596b1 Addressing jk-jeon/dragonbox#25
2e3f58c2 Adapt the change in dragonbox.h
8a1c13d7 Change header guards convention
ec688b1a Remove now-unused stuffs
b9f01c18 Reformat
1bfe1176 #if block for ease of testing
3c0f1c6f Eliminate integer checks
dc4557df Reformat
ac5788da Adapt new table generation scheme
6251ad9a Reformatting
346f7538 Use new bignum
aa5c699a Regenerate cache
c0776eec Add dragonbox::common as a dependency to sandbox
bb47e99e Rewrite to match the new method
daa0c81e Fix some trivial errors
a29a8efb Modify copyright
9b11d788 Rename a file
a5316aa0 Rename
bc3d0eaf Modify according to the new scheme
74a58cb8 Remove old files
b4a66924 Big int & continued fractions implementation
6281dc3e Add dragonbox::common to dependency
9868af72 Fix typo in comment
0cdb1548 Add dragonbox_to_chars as a dependency to sandbox
9b460165 Merge pull request #24 from jwakely/patch-1
4f3715d6 Fix typos and grammar in README
cf8b12cc Rerun milo's benchmark
1a964604 Rename main.cpp into benchmark.cpp
ab23f7aa Simplify remove_trailing_zeros further
bf8442be Rename main.cpp into benchmark.cpp
ba521e30 It seems the new version of readtable now reads hex integers directly
5ecd1534 Fix copyright notice
106f7b94 Replace stof/d into strtof/d
12310e74 Remove unused param name
11aae947 Improve comments on ROR
99fd14ba Fix the bug introduced by the commit e26c8d7363eb0ad9753b56a9699dea194784eb5e
648591a4 Fix a bug introduced by commit 2a33d79e1ce6f2045edacd79c8f087ffd10e1323
aad99d52 Silence MSVC warning by defining JKJ_DRAGONBOX_HAS_BUILTIN
39d32f1d Fix typo
2db8656d Fix typo
e26c8d73 Optimize/simplify remove_trailing_zeros
2a33d79e Optimize break_rounding_tie Now the policy classes do not actually round, rather, it returns a boolean flag for if they may want to round
d9b2664d Optimize check_divisibility_and_divide_by_pow10
5a699783 Reduced usages of __int128 __int128 seems to generate worse code
2ac7fa6a Add minmax Euclid to the paper
db39c05c Fix typo
f3764d5a Change divtable from SOA-style into AOS-style Because it makes more sense
63d9dc1b Update paper according to jk-jeon/dragonbox#22
8323f379 Make README.md clearer
a54232b9 Merge pull request #22 from yotann/master
fc909b8d fix bugs in compute_right_closed_directed
16473a7d Merge branch 'master' of https://github.com/jk-jeon/dragonbox
538d7d32 Revised some writings
90face27 Merge pull request #21 from Alexhuszagh/master
e9509370 Add CI support for OSes and other architectures.

git-subtree-dir: third_party/dragonbox
git-subtree-split: 9c26179a9c4368db9f7703879e3b2e34aa0e29c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants