Repository navigation
Conversation
…debase Character classes in specifications and protocols are defined over ASCII, yet callers used <cctype>, whose results depend on the global locale and which is undefined for a negative char, and several of them passed bytes from headers, configuration files and environment variables straight through. Others repeated the same range checks and hexadecimal decoding and encoding by hand. common/detail/ascii.h provides constexpr IsAsciiDigit, IsAsciiLower, IsAsciiUpper, IsAsciiAlpha, IsAsciiAlnum, IsAsciiHexDigit, IsAsciiControl, IsAsciiSpace, ToAsciiLower, ToAsciiUpper, HexDigitValue, LowerHexDigit and UpperHexDigit, plus EncodeLowerHex and EncodeUpperHex for byte sequences. TraceState, Baggage, StringUtil::Trim, EnvironmentCarrier, env_variables, DocumentNode, PercentDecode, InstrumentDescriptor comparison and hashing, the OpenTelemetry trace state sampler, the environment entity detector, the OTLP and HTTP client header maps, the curl HTTP time parser and debug log, UrlDecoder, the HTTP test server, the ETW provider GUID, the Prometheus sanitizers and the W3C test server use them. The digit table in trace/propagation/detail/hex.h becomes HexDigitValue's lookup table: propagators decode trace and span IDs on every request, and the lookup is faster there than range checks. The encoders replace the hexadecimal encoding tables and loops in TraceId, SpanId, TraceFlags, Baggage, the service detector's UUID formatter and the OTLP HTTP and file clients, and the digit helpers replace them in the trace state sampler and the ETW UUID. Each keeps its case. ascii_test checks each classifier against <cctype> in the "C" locale for all 256 byte values, and each encoder against printf.
Before GCC 16, libstdc++'s std::regex matcher recurses once per input character (GCC bug 86164), so validating a 255-character instrument name needs a deep stack and overflows small thread stacks, such as the few KB an RTOS task gets. Names and units are now checked with a loop that accepts the same patterns. Unlike the regex, which read the view as a C string, it checks exactly the view: an embedded NUL is rejected, and a view that is not NUL-terminated is no longer read past its end, which accepted an empty view over a valid name and rejected a valid view followed by invalid characters. Unlike the former non-regex fallback, it does not read the first character of an empty name, and it uses the ASCII helpers instead of the locale-dependent std::isalpha and std::isalnum. meter.h no longer includes <regex> through the validator's header. TestUnit checked its valid units with ValidateName; it now calls ValidateUnit. New cases cover an embedded NUL, non-ASCII characters (including as the first character of a name), an empty unit and views that are not NUL-terminated; each fails with the regex. A benchmark covers construction and validation.
cppcheck reports their callers in SpanId, TraceId and TraceFlags as reading the uninitialized buffers that the B3 propagator passes in. The call sites go back to writing two digits per byte with LowerHexDigit and UpperHexDigit.
The opentracing shim propagation test declared an unused common namespace alias and included attribute_value.h only for it. The validator benchmark passes names as nostd::string_view.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4715 +/- ##
==========================================
+ Coverage 86.81% 86.83% +0.02%
==========================================
Files 525 526 +1
Lines 20736 20757 +21
==========================================
+ Hits 18000 18022 +22
+ Misses 2736 2735 -1
🚀 New features to boost your workflow:
|
ltowarek
marked this pull request as ready for review
October 19, 2026 05:09
Member
|
reproduced both validator bugs on main. asan flags a heap over-read in |
mateenali66
approved these changes
Oct 19, 2026
marcalff
approved these changes
Oct 20, 2026
marcalff
left a comment
marcalff
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks for the fix.
Independently of the issue with regexp, this fix removes all the hard coded helpers from the code base and replaces them with a clean implementation.
This is a most welcome change, great work.
This branch has not been deployed
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
4 participants
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.
Follow-up to #4570.
InstrumentMetaDataValidatorchecks every instrument name withstd::regex. Before GCC 16, libstdc++'s regex uses one stack frame per character (GCC bug 86164), so a long name can overflow a small stack. On an ESP32-S3 (GCC 15.2), a 255-character name overflows a 4 KB task.Changes
common/detail/ascii.hwith small ASCII helpers (IsAsciiAlpha,HexDigitValue,LowerHexDigit, ...) and use them instead of<cctype>and hand-written copies.<cctype>depends on the locale and is undefined for negativechar. In the "C" locale nothing changes, and a test checks every byte. Under other locales, bytes above 0x7F are no longer treated as letters or spaces.TraceFlags::ToLowerBase16keeps its uppercase output.string_viewpast its end. FixTestUnit, which calledValidateName.Benchmarks
main (a273994) vs this PR (0e8dda2) , GCC 13.3
-O3, i5-4590, one core, median of 5 runs.Inject is 5-24% faster and
Baggage::ToHeader11-15% faster. Extract changes by -3% to +4%.propagator_benchmark, baggage_benchmark and span_id_benchmark
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes