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

Fix int64 min issue #1722

Merged
merged 3 commits into from
Sep 10, 2019
Merged

Fix int64 min issue #1722

merged 3 commits into from
Sep 10, 2019

Conversation

t-b
Copy link
Contributor

@t-b t-b commented Aug 23, 2019

Close #1708.

For some gcc version (Ubuntu 5.5.0-12ubuntu1~16.04) the existing code
crashes when the minimum value of int64_t is outputted.

Rewrite the code to be less complicated to avoid it. This partially
reverts what was done in 546e2cb (rotating_light fixed some warnings,
2019-03-13).

It might be the case that this reintroduces the warning removed earlier. If that is the case, I will rewrite the code using templates to conditionally remove the sign for the negative case only.

Do we need static assertions which ensure that every positive number_integer_t fits into a number_unsigned_t?

@coveralls
Copy link

coveralls commented Aug 24, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 8067c3c on t-b:fix-int64-min-issue into eab68e7 on nlohmann:develop.

@jaredgrubb
Copy link
Contributor

This patch invokes Undefined Behavior. You aren't allowed to take the negative of "min int". Eg, for Int8, the negative of -128 is +128, which is not representable in Int8. This is UB in C++.

@jaredgrubb
Copy link
Contributor

Maybe you could do something like static_cast<number_unsigned_t>(-(x+1)) + 1 ?

@t-b
Copy link
Contributor Author

t-b commented Aug 25, 2019

@jaredgrubb Thanks for the review.

Good point.

Your suggestion is pretty close to the old solution prior to 546e2cb. I've used that one, but also moved the code into functions to avoid the warning which 546e2cb wanted to fix.

Let's see what CI says.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

I have some minor change requests.

include/nlohmann/detail/output/serializer.hpp Outdated Show resolved Hide resolved
test/src/unit-serialization.cpp Outdated Show resolved Hide resolved
include/nlohmann/detail/output/serializer.hpp Show resolved Hide resolved
For some gcc version (Ubuntu 5.5.0-12ubuntu1~16.04) the existing code
crashes when the minimum value of int64_t is outputted.

Resurrect the code from before 546e2cb (:rotating_light: fixed some warnings,
2019-03-13) but delegate the sign removal so that the compilers don't
complain about taking the negative value of an unsigned value.

In addition we also rewrite the expression so that we first increment
and then negate.

The definition of remove_sign(number_unsigned_t) is never called as
unsigned values are never negative.
@t-b
Copy link
Contributor Author

t-b commented Sep 3, 2019

@nlohmann Thanks for the review.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this Sep 10, 2019
@nlohmann nlohmann added this to the Release 3.7.1 milestone Sep 10, 2019
@nlohmann
Copy link
Owner


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description

  • Fix serialization to work with minimal/maximal values.

@nlohmann nlohmann merged commit 06ccd43 into nlohmann:develop Sep 10, 2019
@nlohmann
Copy link
Owner

Thanks a lot!

@t-b t-b deleted the fix-int64-min-issue branch September 10, 2019 09:38
dnsmichi pushed a commit to Icinga/icinga2 that referenced this pull request Dec 13, 2019
This includes the following fixes:

nlohmann/json#1436

> For a deeply-nested JSON object, the recursive implementation of json_value::destroy function causes stack overflow.

nlohmann/json#1708
nlohmann/json#1722

Stack size

nlohmann/json#1693 (comment)

Integer Overflow

nlohmann/json#1447

UTF8, json dump out of bounds

nlohmann/json#1445

Possibly influences #7532
Al2Klimov pushed a commit to Icinga/icinga2 that referenced this pull request Dec 16, 2019
This includes the following fixes:

nlohmann/json#1436

> For a deeply-nested JSON object, the recursive implementation of json_value::destroy function causes stack overflow.

nlohmann/json#1708
nlohmann/json#1722

Stack size

nlohmann/json#1693 (comment)

Integer Overflow

nlohmann/json#1447

UTF8, json dump out of bounds

nlohmann/json#1445

Possibly influences #7532
N-o-X pushed a commit to Icinga/icinga2 that referenced this pull request May 8, 2020
This includes the following fixes:

nlohmann/json#1436

> For a deeply-nested JSON object, the recursive implementation of json_value::destroy function causes stack overflow.

nlohmann/json#1708
nlohmann/json#1722

Stack size

nlohmann/json#1693 (comment)

Integer Overflow

nlohmann/json#1447

UTF8, json dump out of bounds

nlohmann/json#1445

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

Successfully merging this pull request may close these issues.

Error in dump() with int64_t minimum value
4 participants