Switch number parsing to std::from_chars / extended locale strto*_l if available#5237
Open
jgreitemann wants to merge 4 commits into
Open
Switch number parsing to std::from_chars / extended locale strto*_l if available#5237jgreitemann wants to merge 4 commits into
std::from_chars / extended locale strto*_l if available#5237jgreitemann wants to merge 4 commits into
Conversation
In debug builds, this test fails with `SIGABRT` due to an assertion. In release builds, truncation of the fractional part will be detected. Sometimes, this test may also cause memory violation (`SIGSEGV`) due to it being undefined behavior to set locale and use locale-dependent functions concurrently. On Windows, this does not happen since the global locale is protected by a mutex. This test assumes that a locale called "de_DE.UTF-8" exists on the host. If it does not, the test bails out but does not fail. The locale name is hard-coded because it is impossible to query available locales portably. "de_DE.UTF-8" is an accepted format on Ubuntu, macOS, and Windows. Signed-off-by: Jonas Greitemann <jgreitemann@gmail.com>
When an integer JSON token exceeds the range of the underlying 64-bit unsigned/integer types, the lexer instead identifies the token type as `value_float`. When the value exceeds the range of the underlying floating-point type, the token type remains as `value_float`. This was previously not tested explicitly for the edge case values and subtly breaking this behavior right around the edge case was not caught by any other test. Signed-off-by: Jonas Greitemann <jgreitemann@gmail.com>
…herwise `std::from_chars` is locale-independent and thus avoids the TOCTOU issue nlohmann#5198 when it's available. This commit introduces a `from_chars` utility in the `detail` namespace which merely wraps `std::from_chars` if it is available and emulates its API and behavior using the `strto*` family of functions when it is not. As of this commit, the `strto*`-based fallback is still locale-dependent and only matches `std::from_chars` when the "C" locale is used. Availability of `std::from_chars` for floating point numbers is varied. For instance, as of libc++ 22, `std::from_chars` for `long double` isn't supported and, consequently, the feature-test macro `__cpp_lib_to_chars` is not set, while the overloads for `float` and `double` are available. To avoid complicating matters further, the feature-test macro is taken as the authoritative indicator for (full) `std::from_chars` support and the fallback is used otherwise. If `std::from_chars` is available, our wrapper additionally tweaks its output in the case of floating-point out-of-range results. The behavior in this case is poorly (under-)specified and existing implementations do not agree. The wrapper's tweaks are intended to ensure consistent behavior across all toolchains in this edge case. In practice, this should only take effect when parsing out-of-range floats on libstdc++. The unit tests for the `from_chars` helper are intentionally compiled in both C++11 and C++17 modes. This ensures that the test expectations align with the actual behavior we're trying to model. The tests are not aiming to cover all of floating-point parsing comprehensively, as `std::from_chars` is specified to behave mostly the same as `strto*` and instead focuses on the edge cases where the two differ: - leading whitespace is not tolerated - `+` sign is not allowed (outside of the exponent) - out-parameter is left unchanged in case of `invalid_argument` and integral `result_out_of_range` errors Signed-off-by: Jonas Greitemann <jgreitemann@gmail.com>
On Windows, `_strtof_l` et al. are always available. POSIX on the other hand only covers `newlocale` and `freelocale` but the extended locale functions `strtof_l` provided by glibc or `<xlocale.h>` on BSDs cannot be assumed to be universally available. For this reason, the trait `has_extended_locale_support` uses SFINAE to detect whether `strtof_l` is defined and falls back to the locale-dependent standard functions if it isn't. Signed-off-by: Jonas Greitemann <jgreitemann@gmail.com>
nlohmann
requested changes
Jul 5, 2026
nlohmann
left a comment
Owner
There was a problem hiding this comment.
Some observations (thanks to Claude Code):
-
The
std::from_chars-tierP4168 shim overflows a plainintwhen re-parsing the exponent, causing an assertion abort (debug builds) or a silently wrong result (release builds). E.g.,"1e-99999999999999999999"incorrectly parses toinfinstead of the correct0.0. -
The POSIX extended_locale_traits SFINAE probe only tests
strtof_l, but the selected specialization unconditionally usesstrtoll_l/strtoull_l/isspace_ltoo, which musl doesn't declare.
Owner
|
Hi @jgreitemann, thanks for your patch! I had a look at the CI failures:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5198. The issue describes a TOCTOU bug when changes to the global C locale race against the use of
localeconv/strto*by the lexer. Even discounting the TOCTOU issue, using locale-dependent functions while switching locale concurrently is undefined behavior.Implements a two-tiered approach to avoid locale dependent number parsing:
std::from_charsprovides an inherently locale-independent API.strto*_lon POSIX,_strto*_lon Windows) and pass them an instance of the "C" locale. This isolates them from the effects of the global locale.strto*functions. The bug is not fixed in this case.All approaches are encapsulated in terms of a
nlohmann::detail::from_charsAPI that is more or less identical to that ofstd::from_chars.Caveats / Availability:
std::from_charssupport is detected based on the__cpp_lib_to_charsfeature-test macro. Implementation support forstd::from_charshas not been great. For example, libc++ only added support forfloatanddoublein Clang 20 and to this day does not supportlong double. Hence, libc++ does not set__cpp_lib_to_charsand we won't be usingstd::from_chars, even forfloatanddouble. If a future version of libc++ adds support forlong double, the PR would start using it, though.std::from_charswhich inspects the result in case ofresult_out_of_rangeand ensures behavior consistent with what P4168 proposes (and which libc++ and MS STL already implement), even on libstdc++ (which currently leaves the value unset). If P4168 is accepted as a defect report and implementations converge in the future, this shim should no longer be exercised (though it would have to stay in to still support older toolchains).strtof_l. On Windows,_strtof_land friends have been available since at least Visual Studio 2005, so I think we don't need to bother with SFINAE in that case.A last-resort mitigation (in case extended locale APIs are not available) would potentially see us using
uselocale(POSIX) and_configthreadlocale(Windows) to set (and unset) a thread-local locale. This approach comes with degraded performance, even for users who do not need to mitigate the issue because they don't change locale. In practice, the absence of the two other mitigations should be quite rare, so this approach is not implemented by this PR (see discussion in issue). An open question is whether this is still a sufficiently important issue to add a note to the documentation.make amalgamate.