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

dump_float: truncation from ptrdiff_t to long #527

Closed
ftillier opened this issue Mar 21, 2017 · 9 comments
Closed

dump_float: truncation from ptrdiff_t to long #527

ftillier opened this issue Mar 21, 2017 · 9 comments
Assignees
Labels
Milestone

Comments

@ftillier
Copy link

Building with VisualStudio 2017, using /W4 and /Wx, I get the following:
1>json.hpp(6946): warning C4244: '=': conversion from 'std::_Array_iterator<_Ty,64>::difference_type' to 'long', possible loss of data

On Windows, long is always 32-bits. Declaring 'len' as std::ptrdiff_t fixes the issue.

@ftillier
Copy link
Author

index a9a5067..e7b23a5 100644
--- a/src/json.hpp.re2c
+++ b/src/json.hpp.re2c
@@ -6927,7 +6927,7 @@ class basic_json
             static constexpr auto d = std::numeric_limits<number_float_t>::digits10;
 
             // the actual conversion
-            long len = snprintf(number_buffer.data(), number_buffer.size(),
+            std::ptrdiff_t len = snprintf(number_buffer.data(), number_buffer.size(),
                                 "%.*g", d, x);
 
             // negative value indicates an error

@gregmarr
Copy link
Contributor

According to the VS 2015 docs, the return type is int. The VS2017 docs say the same thing. I wonder if they changed the return type in VS 2017 without updating the docs.

@ftillier
Copy link
Author

Sorry for the lack of context. The compiler error comes from line 6946:
len = (end - number_buffer.begin());

The return type of snprintf is still int, which gets promoted to long just fine. It's the assignment, which uses std::array::difference_type (in this case std::ptrdiff_t) on the right hand side, which could truncate since long is smaller than ptrdiff_t.

@nlohmann nlohmann added the platform: visual studio related to MSVC label Mar 21, 2017
@gregmarr
Copy link
Contributor

Thanks for the clarification, that makes more sense. I just assumed that the error was on the code line that you posted, and that it was line 6946 in the version of the header that you were using.

@ftillier
Copy link
Author

I don't have a system on which I can run re2c, so I hesitate to issue a PR for this - is the patch above sufficient?

@TurpentineDistillery
Copy link

TurpentineDistillery commented Mar 22, 2017

@ftillier, you don't need re2c. You can do git checkout src/json.hpp.re2c && git diff src/json.hpp | patch src/json.hpp.re2c
@nlohmann, perhaps mention the above hack in contribution guidelines?

WRT the fix: const auto len = snprintf(... ?

@nlohmann
Copy link
Owner

@TurpentineDistillery Good point!

@nlohmann nlohmann self-assigned this Mar 22, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Mar 22, 2017
nlohmann added a commit that referenced this issue Mar 22, 2017
The result of snprintf is later used in situations where a long may
overflow.
nlohmann added a commit that referenced this issue Mar 22, 2017
@nlohmann
Copy link
Owner

@ftillier Should be fixed now. Could you please validate?

@ftillier
Copy link
Author

@nlohmann, validated as fixed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants