Skip to content

avoid signed negation overflow in integer_times_pow10 - #421

Open
sahvx655-wq wants to merge 1 commit into
fastfloat:mainfrom
sahvx655-wq:times-pow10-int64-min
Open

sahvx655-wq wants to merge 1 commit into
fastfloat:mainfrom
sahvx655-wq:times-pow10-int64-min

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

The int64_t overload of integer_times_pow10 takes the magnitude with -mantissa, which is signed overflow when the mantissa is the most negative int64_t. Reading the overload I noticed the tests sweep numeric_limits<int64_t>::max() but never min(), so I ran that value under the FASTFLOAT_SANITIZE flags and it aborts with parse_number.h:506: runtime error: negation of -9223372036854775808 cannot be represented in type 'int64_t'; in a C++20 constant expression the call does not compile at all. It is not only a sanitiser complaint: GCC 16.1 at -O2 (aarch64) assumes the negated value is non-negative and emits a signed compare (bgt) for the fast-path mantissa bound, so the wrapped value passes the Clinger gate and integer_times_pow10(INT64_MIN, 0) returns 0x1p+63 where from_chars on the same digits gives -0x1p+63.

Taking the magnitude in uint64_t is well defined for every input, and clang emits identical assembly for the overload at -O2, so nothing changes for values that already worked. The negation happens inside the overload, so that is the only place it can be put right: the function is documented to accept any int64_t, and left alone the sign of the result depends on which compiler and optimisation level built the caller. I added min() cases beside the existing max() ones in tests/basictest.cpp; against the old header they fail the value check under GCC -O2 and abort with SIGABRT in the sanitised build, and they pass with the change.

Negating the most negative int64_t is undefined behaviour. GCC at -O2 uses that to turn the fast-path mantissa bound into a signed compare and returns the wrong sign, so take the magnitude in uint64_t instead.

This branch has not been deployed

No deployments
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

Successfully merging this pull request may close these issues.

1 participant