diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java index f40b47a89d4..6cc0f940f66 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java @@ -46,6 +46,26 @@ public static DD64bTraceId from(String s) throws NumberFormatException { return DD64bTraceId.create(LongStringUtils.parseUnsignedLong(s), s); } + /** + * Like {@link #from(String)}, but returns {@code null} instead of throwing when the given {@code + * String} is not a plain decimal unsigned 64 bit id. Stricter than {@link #from(String)}: a + * leading {@code +} and non-ASCII digits are rejected. + * + *

Internal API for parsing untrusted input, such as propagation headers, on the request path. + * Not intended for use outside the tracer. + * + * @param s String of unsigned 64 bit id + * @return DDTraceId, or {@code null} if the {@code String} is not a valid id + */ + public static DD64bTraceId fromOrNull(String s) { + int len = s == null ? 0 : s.length(); + long id = LongStringUtils.parseUnsignedLongOrSentinel(s, 0, len, 0L); + if (id == 0L && !LongStringUtils.isUnsignedLongZero(s, 0, len)) { + return null; + } + return DD64bTraceId.create(id, s); + } + /** * Create a new {@code DDTraceId} from the given {@code String} hex representation of the unsigned * 64 bit id. diff --git a/dd-trace-api/src/main/java/datadog/trace/api/internal/util/LongStringUtils.java b/dd-trace-api/src/main/java/datadog/trace/api/internal/util/LongStringUtils.java index aa55b8943fb..650f39c63b4 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/internal/util/LongStringUtils.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/internal/util/LongStringUtils.java @@ -73,7 +73,93 @@ private static int firstNonZeroCharacter(CharSequence s, int start) { return firstNonZero; } + /** + * Parse the decimal representation of the unsigned 64 bit long from the {@code String}. + * + * @param s String in decimal of unsigned 64-bits long. + * @return long + * @throws NumberFormatException + */ public static long parseUnsignedLong(String s) throws NumberFormatException { + long result = parseUnsignedLongOrSentinel(s, 0, s == null ? 0 : s.length(), 0L); + if (result != 0L) { + return result; + } + // Zero is ambiguous with the invalid sentinel. Let the strict parser decide: it accepts input + // the non-throwing parser rejects (a leading '+', non-ASCII digits) and builds the exception. + return parseUnsignedLongStrict(s); + } + + /** + * Parse the decimal representation of the unsigned 64 bit long from the {@code CharSequence} + * without throwing, for untrusted input such as propagation headers. + * + *

Accepts 1 to 20 ASCII digits only: no sign, no whitespace, no non-ASCII digits. An unsigned + * 64 bit value uses every bit, so no return value can mean "invalid" on its own. Callers pick an + * {@code ifInvalid} that is impossible or harmless for their use, e.g. {@code 0} for trace and + * span ids. With {@code 0} as {@code ifInvalid}, {@link #isUnsignedLongZero} tells a real zero + * apart from invalid input. + * + * @param s CharSequence containing the decimal digits + * @param start the start index of the decimal value + * @param len the length of the decimal value + * @param ifInvalid the value returned for null, empty, malformed or out of range input + * @return the parsed long, or {@code ifInvalid} + */ + public static long parseUnsignedLongOrSentinel( + CharSequence s, int start, int len, long ifInvalid) { + if (s == null || len <= 0 || len > 20 || start < 0 || start + len > s.length()) { + return ifInvalid; + } + int end = start + len; + // Signed 64 bits holds any 18 digit number, so the first 18 digits cannot overflow + int fastEnd = Math.min(end, start + 18); + long result = 0; + int i = start; + for (; i < fastEnd; i++) { + int d = s.charAt(i) - '0'; + if (d < 0 || d > 9) { + return ifInvalid; + } + result = result * 10 + d; + } + for (; i < end; i++) { + int d = s.charAt(i) - '0'; + if (d < 0 || d > 9 || Long.compareUnsigned(result, MAX_FIRST_PART) > 0) { + return ifInvalid; + } + long scaled = result * 10; + result = scaled + d; + if (Long.compareUnsigned(result, scaled) < 0) { + return ifInvalid; + } + } + return result; + } + + /** + * Whether {@code s[start, start + len)} is a zero that {@link #parseUnsignedLongOrSentinel} + * accepts: 1 to 20 {@code '0'} characters. Callers that use {@code 0} as the invalid sentinel + * check this only when the parse returned {@code 0}. + * + * @param s CharSequence containing the decimal digits + * @param start the start index of the decimal value + * @param len the length of the decimal value + * @return true if the range parses to zero, false if it is zero-valued only as the sentinel + */ + public static boolean isUnsignedLongZero(CharSequence s, int start, int len) { + if (s == null || len <= 0 || len > 20 || start < 0 || start + len > s.length()) { + return false; + } + for (int i = start, end = start + len; i < end; i++) { + if (s.charAt(i) != '0') { + return false; + } + } + return true; + } + + private static long parseUnsignedLongStrict(String s) throws NumberFormatException { if (s == null) { throw new NumberFormatException("s can't be null"); } diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java index 98225bb9698..27ca226355b 100644 --- a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -70,6 +71,46 @@ void failParsingIllegal64BitIdStringRepresentation(String stringId) { assertThrows(NumberFormatException.class, () -> DD64bTraceId.from(stringId)); } + @TableTest({ + "scenario | stringId | expectedId ", + "zero | '0' | DD64bTraceId.ZERO ", + "one | '1' | DD64bTraceId.ONE ", + "max | '18446744073709551615' | DD64bTraceId.MAX ", + "long max | '9223372036854775807' | DD64bTraceId.LONG_MAX", + "long max plus one | '9223372036854775808' | DD64bTraceId.LONG_MIN" + }) + @ParameterizedTest(name = "fromOrNull parses valid 64-bit id String representation [{index}]") + void fromOrNullParsesValid64BitIdStringRepresentation(String stringId, DD64bTraceId expectedId) { + DD64bTraceId ddid = DD64bTraceId.fromOrNull(stringId); + + assertEquals(expectedId, ddid); + assertEquals(stringId, ddid.toString()); + } + + @Test + void fromOrNullKeepsTheParsedStringForToString() { + String stringId = "1234567890"; + + assertSame(stringId, DD64bTraceId.fromOrNull(stringId).toString()); + } + + @ParameterizedTest(name = "fromOrNull returns null for illegal 64-bit id String [{index}]") + @NullSource + @ValueSource( + strings = { + "", + "-1", + "+1", + "18446744073709551616", + "18446744073709551625", + "184467440737095516150", + "18446744073709551a1", + "184467440737095511a" + }) + void fromOrNullReturnsNullForIllegal64BitIdString(String stringId) { + assertNull(DD64bTraceId.fromOrNull(stringId)); + } + @TableTest({ "scenario | hexId | expectedId ", "zero | '0' | DD64bTraceId.ZERO ", diff --git a/dd-trace-api/src/test/java/datadog/trace/api/internal/util/LongStringUtilsTest.java b/dd-trace-api/src/test/java/datadog/trace/api/internal/util/LongStringUtilsTest.java new file mode 100644 index 00000000000..a18f74a3034 --- /dev/null +++ b/dd-trace-api/src/test/java/datadog/trace/api/internal/util/LongStringUtilsTest.java @@ -0,0 +1,154 @@ +package datadog.trace.api.internal.util; + +import static datadog.trace.api.internal.util.LongStringUtils.parseUnsignedLong; +import static datadog.trace.api.internal.util.LongStringUtils.parseUnsignedLongOrSentinel; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import datadog.trace.api.DDTraceApiTableTestConverters; +import java.math.BigInteger; +import java.util.concurrent.ThreadLocalRandom; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.tabletest.junit.TableTest; +import org.tabletest.junit.TypeConverterSources; + +@TypeConverterSources(DDTraceApiTableTestConverters.class) +class LongStringUtilsTest { + private static final long SENTINEL = 42L; + + @TableTest({ + "scenario | input | expected ", + "zero | '0' | 0 ", + "one | '1' | 1 ", + "leading zeros | '00001' | 1 ", + "18 digits | '999999999999999999' | 999999999999999999 ", + "19 digits | '1000000000000000000' | 1000000000000000000", + "long max | '9223372036854775807' | Long.MAX_VALUE ", + "long max plus one | '9223372036854775808' | Long.MIN_VALUE ", + "unsigned max | '18446744073709551615' | -1 ", + "20 digits with leading zero | '00000000000000000001' | 1 " + }) + @ParameterizedTest(name = "parse valid decimal [{index}]") + void parseValidDecimal(String input, long expected) { + assertEquals(expected, parseUnsignedLongOrSentinel(input, 0, input.length(), SENTINEL)); + assertEquals(expected, parseUnsignedLong(input)); + } + + @ParameterizedTest(name = "non-throwing parse returns sentinel for invalid input [{index}]") + @ValueSource( + strings = { + "", + "-1", + "+1", + " 1", + "1 ", + "12a", + "0x1a", + "١", // ARABIC-INDIC DIGIT ONE + "18446744073709551616", // unsigned max + 1 + "18446744073709551620", // first 19 digits already too large + "99999999999999999999", + "73659894405625460838", // JDK 8's parseUnsignedLong wraps this instead of throwing + "184467440737095516150", // 21 digits + "000000000000000000001" // 21 digits, even with leading zeros + }) + void nonThrowingParseReturnsSentinelForInvalidInput(String input) { + assertEquals(SENTINEL, parseUnsignedLongOrSentinel(input, 0, input.length(), SENTINEL)); + } + + @Test + void nonThrowingParseReturnsSentinelForNull() { + assertEquals(SENTINEL, parseUnsignedLongOrSentinel(null, 0, 0, SENTINEL)); + } + + @TableTest({ + "scenario | input | start | len | expected", + "middle | 'x123y' | 1 | 3 | 123 ", + "prefix | '123y' | 0 | 3 | 123 ", + "negative start | '123' | -1 | 2 | 42 ", + "past the end | '123' | 1 | 3 | 42 ", + "zero length | '123' | 0 | 0 | 42 ", + "includes bad char | 'x123y' | 1 | 4 | 42 " + }) + @ParameterizedTest(name = "non-throwing parse of a range [{index}]") + void nonThrowingParseOfARange(String input, int start, int len, long expected) { + assertEquals(expected, parseUnsignedLongOrSentinel(input, start, len, SENTINEL)); + } + + @TableTest({ + "scenario | input | start | len | expected", + "zero | '0' | 0 | 1 | true ", + "20 zeros | '00000000000000000000' | 0 | 20 | true ", + "zeros in a range | 'x00y' | 1 | 2 | true ", + "21 zeros | '000000000000000000000' | 0 | 21 | false ", + "non-zero digit | '01' | 0 | 2 | false ", + "not a digit | 'a' | 0 | 1 | false ", + "empty | '' | 0 | 0 | false ", + "past the end | '0' | 0 | 2 | false " + }) + @ParameterizedTest(name = "isUnsignedLongZero [{index}]") + void isUnsignedLongZero(String input, int start, int len, boolean expected) { + assertEquals(expected, LongStringUtils.isUnsignedLongZero(input, start, len)); + } + + @Test + void isUnsignedLongZeroOfNull() { + assertFalse(LongStringUtils.isUnsignedLongZero(null, 0, 0)); + } + + @TableTest({ + "scenario | input | expected", + "leading plus | '+1' | 1 ", + "non-ASCII digit | '١' | 1 " + }) + @ParameterizedTest(name = "throwing parse keeps accepting what Long.parseLong accepts [{index}]") + void throwingParseKeepsAcceptingWhatLongParseLongAccepts(String input, long expected) { + assertEquals(expected, parseUnsignedLong(input)); + } + + @ParameterizedTest(name = "throwing parse rejects invalid input [{index}]") + @NullSource + @ValueSource(strings = {"", "-1", "12a", "18446744073709551616", "184467440737095516150"}) + void throwingParseRejectsInvalidInput(String input) { + assertThrows(NumberFormatException.class, () -> parseUnsignedLong(input)); + } + + @Test + void matchesJdkForRandomUnsignedLongs() { + ThreadLocalRandom random = ThreadLocalRandom.current(); + for (int i = 0; i < 10_000; i++) { + long value = random.nextLong(); + String s = Long.toUnsignedString(value); + assertEquals(value, parseUnsignedLongOrSentinel(s, 0, s.length(), SENTINEL), s); + } + } + + @Test + void matchesExactValueForRandomDigitStrings() { + // Lengths around the 18 / 19 / 20 digit boundaries, where overflow handling changes. Longer + // input is always rejected, even with leading zeros, matching parseUnsignedLong(String). + // BigInteger is the oracle: JDK 8's parseUnsignedLong misses some 20-digit overflows and + // wraps them instead of throwing. + ThreadLocalRandom random = ThreadLocalRandom.current(); + char[] digits = new char[20]; + for (int i = 0; i < 10_000; i++) { + int len = 17 + random.nextInt(4); + for (int j = 0; j < len; j++) { + digits[j] = (char) ('0' + random.nextInt(10)); + } + String s = new String(digits, 0, len); + BigInteger exact = new BigInteger(s); + boolean valid = exact.bitLength() <= 64; + long expected = exact.longValue(); + if (valid) { + assertEquals(expected, parseUnsignedLongOrSentinel(s, 0, len, SENTINEL), s); + } else { + assertEquals(SENTINEL, parseUnsignedLongOrSentinel(s, 0, len, SENTINEL), s); + } + } + } +} diff --git a/dd-trace-core/src/jmh/java/datadog/trace/core/propagation/DatadogIdParseBenchmark.java b/dd-trace-core/src/jmh/java/datadog/trace/core/propagation/DatadogIdParseBenchmark.java new file mode 100644 index 00000000000..0708ada94d2 --- /dev/null +++ b/dd-trace-core/src/jmh/java/datadog/trace/core/propagation/DatadogIdParseBenchmark.java @@ -0,0 +1,135 @@ +package datadog.trace.core.propagation; + +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import datadog.trace.api.Config; +import datadog.trace.api.DD64bTraceId; +import datadog.trace.api.DDTraceId; +import datadog.trace.api.DynamicConfig; +import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; +import datadog.trace.bootstrap.instrumentation.api.TagContext; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.slf4j.LoggerFactory; + +/** + * Cost of parsing an {@code x-datadog-trace-id} header value, valid and invalid, with the throwing + * {@link DD64bTraceId#from(String)} and the non-throwing {@link DD64bTraceId#fromOrNull(String)}, + * and of a full Datadog-style extraction carrying that value. + * + *

The invalid values are the two shapes behind the highest-volume propagation entries in Error + * Tracking: a value past the unsigned 64 bit range ({@code numberFormatOutOfLongRange}) and a value + * that is not decimal at all. Valid values cover both the 18 digit path and the 19 digit path, + * where most randomly generated 63 bit ids fall. + * + *

Results. Invalid ids cost 20-43 ns per extraction instead of 840-880 ns, because no + * exception is built. The benchmark's stack is shallow; a real server's is much deeper, so the + * saving in production is larger. Overflow (24 ns to parse) is slower than not-decimal (3 ns) + * because all 20 digits are read before the overflow shows. Valid ids parse about 9 ns faster than + * with {@code Long.parseLong}. Before this change, valid-id extraction varied between forks (the 19 + * digit case ran 119-598 ns); after, every fork is within a few ns. Numbers are the median of 5 + * forks, ns/op, with the per-fork range; "before" is the same benchmark on the parent commit, + * without the {@code fromOrNull} arm. + * Apple M1 Max, 10 CPUs - macOS/aarch64 - JDK 25 + * arm trace id header before after + * extract 18 digits 153 (152-153) 107 (100-109) + * extract 19 digits 149 (119-598) 109 (108-110) + * extract unsigned max + 1 840 (821-947) 43 (42-43) + * extract not decimal 882 (760-884) 21 (21-21) + * fromThrowing 18 digits 31 (31-31) 22 (22-23) + * fromThrowing 19 digits 32 (32-33) 24 (23-24) + * fromThrowing unsigned max + 1 875 (771-953) 919 (915-926) + * fromThrowing not decimal 815 (804-836) 804 (792-809) + * fromOrNull 18 digits - 22 (22-23) + * fromOrNull 19 digits - 24 (24-24) + * fromOrNull unsigned max + 1 - 24 (23-24) + * fromOrNull not decimal - 3 (3-3) + * + */ +@State(Scope.Benchmark) +@Warmup(iterations = 3, time = 2, timeUnit = SECONDS) +@Measurement(iterations = 5, time = 2, timeUnit = SECONDS) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(NANOSECONDS) +@Fork(5) +public class DatadogIdParseBenchmark { + @Param({ + "123456789012345678", // valid, 18 digits + "8217536438929873290", // valid, 19 digits + "18446744073709551616", // invalid, unsigned max + 1 + "1a2b3c4d5e6f" // invalid, not decimal + }) + String traceIdHeader; + + HttpCodec.Extractor extractor; + Map headers; + + @Setup + public void setUp() { + // The test logback.xml on the JMH classpath sets the root logger to DEBUG. Production runs at + // INFO by default, and per-request debug output would swamp the invalid-header arms. + ((Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME)).setLevel(Level.INFO); + + DynamicConfig dynamicConfig = + DynamicConfig.create() + .setHeaderTags(Collections.emptyMap()) + .setBaggageMapping(Collections.emptyMap()) + .apply(); + extractor = DatadogHttpCodec.newExtractor(Config.get(), dynamicConfig::captureTraceConfig); + + headers = new LinkedHashMap<>(); + headers.put(DatadogHttpCodec.TRACE_ID_KEY, traceIdHeader); + headers.put(DatadogHttpCodec.SPAN_ID_KEY, "2345678901234567890"); + headers.put(DatadogHttpCodec.SAMPLING_PRIORITY_KEY, "1"); + headers.put("user-agent", "benchmark"); + } + + @Benchmark + public DDTraceId fromThrowing() { + try { + return DD64bTraceId.from(traceIdHeader); + } catch (NumberFormatException e) { + return null; + } + } + + @Benchmark + public DDTraceId fromOrNull() { + return DD64bTraceId.fromOrNull(traceIdHeader); + } + + @Benchmark + public TagContext extract() { + return extractor.extract(headers, MAP_VISITOR); + } + + private static final AgentPropagation.ContextVisitor> MAP_VISITOR = + new MapContextVisitor(); + + private static final class MapContextVisitor + implements AgentPropagation.ContextVisitor> { + @Override + public void forEachKey(Map carrier, AgentPropagation.KeyClassifier classifier) { + for (Map.Entry entry : carrier.entrySet()) { + if (!classifier.accept(entry.getKey(), entry.getValue())) { + return; + } + } + } + } +} diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java index 278e4cfceb4..11242afa81e 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java @@ -1,6 +1,9 @@ package datadog.trace.core.propagation; import static datadog.trace.api.TracePropagationStyle.DATADOG; +import static datadog.trace.api.internal.util.LongStringUtils.isUnsignedLongZero; +import static datadog.trace.api.internal.util.LongStringUtils.parseUnsignedLongOrSentinel; +import static datadog.trace.api.telemetry.LogCollector.EXCLUDE_TELEMETRY; import static datadog.trace.core.propagation.HttpCodec.firstHeaderValue; import static datadog.trace.core.propagation.XRayHttpCodec.XRayContextInterpreter.handleXRayTraceHeader; import static datadog.trace.core.propagation.XRayHttpCodec.X_AMZN_TRACE_ID; @@ -10,6 +13,7 @@ import datadog.context.propagation.CarrierSetter; import datadog.trace.api.Config; import datadog.trace.api.DD128bTraceId; +import datadog.trace.api.DD64bTraceId; import datadog.trace.api.DDSpanId; import datadog.trace.api.DDTags; import datadog.trace.api.DDTraceId; @@ -171,10 +175,26 @@ public boolean accept(String key, String value) { if (null != value) { switch (classification) { case TRACE_ID: - traceId = DDTraceId.from(firstHeaderValue(value)); + { + String traceIdValue = firstHeaderValue(value); + DDTraceId parsedTraceId = DD64bTraceId.fromOrNull(traceIdValue); + if (parsedTraceId == null) { + return rejectInvalidId(TRACE_ID_KEY, traceIdValue); + } + traceId = parsedTraceId; + } break; case SPAN_ID: - spanId = DDSpanId.from(firstHeaderValue(value)); + { + String spanIdValue = firstHeaderValue(value); + int len = spanIdValue.length(); + long parsedSpanId = + parseUnsignedLongOrSentinel(spanIdValue, 0, len, DDSpanId.ZERO); + if (parsedSpanId == DDSpanId.ZERO && !isUnsignedLongZero(spanIdValue, 0, len)) { + return rejectInvalidId(SPAN_ID_KEY, spanIdValue); + } + spanId = parsedSpanId; + } break; case ORIGIN: origin = firstHeaderValue(value); @@ -209,6 +229,14 @@ public boolean accept(String key, String value) { return true; } + // Malformed ids come from the caller, not from a tracer bug, and repeat on every request from + // that caller, so they are rejected without an exception and kept out of telemetry + private boolean rejectInvalidId(String headerName, String headerValue) { + invalidateContext(); + log.debug(EXCLUDE_TELEMETRY, "Ignoring context with invalid {}: {}", headerName, headerValue); + return false; + } + @Override protected TagContext build() { restore128bTraceId(); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/HttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/HttpCodec.java index 61e9a8f105e..b6df5b8455d 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/HttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/HttpCodec.java @@ -2,6 +2,8 @@ import static datadog.trace.api.DDTags.PARENT_ID; import static datadog.trace.api.TracePropagationStyle.TRACECONTEXT; +import static datadog.trace.api.internal.util.LongStringUtils.isUnsignedLongZero; +import static datadog.trace.api.internal.util.LongStringUtils.parseUnsignedLongOrSentinel; import static datadog.trace.core.propagation.DatadogHttpCodec.SPAN_ID_KEY; import datadog.context.propagation.CarrierSetter; @@ -329,11 +331,14 @@ public boolean accept(String key, String value) { } private void cacheDatadogSpanId(String key, String value) { - if (SPAN_ID_KEY.equalsIgnoreCase(key)) { - try { - // Parse numeric header value to format it as 16 hexadecimal character format - this.datadogSpanIdHex = DDSpanId.toHexStringPadded(DDSpanId.from(value)); - } catch (NumberFormatException ignored) { + if (SPAN_ID_KEY.equalsIgnoreCase(key) && value != null) { + // Parse numeric header value to format it as 16 hexadecimal character format. Same + // non-throwing parse as DatadogHttpCodec, so malformed ids cost no exception and both agree + // on which ids are valid. + int len = value.length(); + long spanId = parseUnsignedLongOrSentinel(value, 0, len, DDSpanId.ZERO); + if (spanId != DDSpanId.ZERO || isUnsignedLongZero(value, 0, len)) { + this.datadogSpanIdHex = DDSpanId.toHexStringPadded(spanId); } } } diff --git a/dd-trace-core/src/test/java/datadog/trace/core/propagation/DatadogHttpExtractorTest.java b/dd-trace-core/src/test/java/datadog/trace/core/propagation/DatadogHttpExtractorTest.java index 356f6aeeda6..462848c57a0 100644 --- a/dd-trace-core/src/test/java/datadog/trace/core/propagation/DatadogHttpExtractorTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/core/propagation/DatadogHttpExtractorTest.java @@ -266,7 +266,13 @@ void extractHttpHeadersWithOutOfRangeSpanId() { "uint64 max traceId | 'MAX' | '1' | true ", "out-of-range traceId | 'MAX+1' | '1' | false ", "uint64 max spanId | '1' | 'MAX' | true ", - "out-of-range spanId | '1' | 'MAX+1' | false " + "out-of-range spanId | '1' | 'MAX+1' | false ", + "leading plus traceId | '+1' | '1' | false ", + "leading plus spanId | '1' | '+1' | false ", + "empty traceId | '' | '1' | false ", + "empty spanId | '1' | '' | false ", + "hex traceId | '0x1a' | '1' | false ", + "hex spanId | '1' | '0x1a' | false " }) void moreIdRangeValidation( @ConvertWith(TraceIdConverter.class) String traceId, diff --git a/dd-trace-core/src/test/java/datadog/trace/core/propagation/HttpExtractorTest.java b/dd-trace-core/src/test/java/datadog/trace/core/propagation/HttpExtractorTest.java index a950013ab7a..5dc2d7b7db1 100644 --- a/dd-trace-core/src/test/java/datadog/trace/core/propagation/HttpExtractorTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/core/propagation/HttpExtractorTest.java @@ -136,7 +136,9 @@ void extractHttpHeadersUsingStyles( "TRACECONTEXT only with traceState p | [TRACECONTEXT] | | | | | 'W3C_TRACE_STATE_WITH_P' | '1' | 'W3C_SPAN_ID_LSTR' | ", "B3MULTI,TRACECONTEXT with traceState p | [B3MULTI, TRACECONTEXT] | | | '1' | '2' | 'W3C_TRACE_STATE_WITH_P' | '1' | 'W3C_SPAN_ID_LSTR' | 'W3C_PARENT_ID' ", "B3MULTI,DATADOG,TRACECONTEXT no traceState | [B3MULTI, DATADOG, TRACECONTEXT] | '1' | '2' | '1' | '4' | | '1' | 'W3C_SPAN_ID_LSTR' | '0000000000000002'", - "DATADOG,TRACECONTEXT no p traceState | [DATADOG, TRACECONTEXT] | '1' | '2' | | | 'W3C_TRACE_STATE_NO_P' | '1' | 'W3C_SPAN_ID_LSTR' | '0000000000000002'" + "DATADOG,TRACECONTEXT no p traceState | [DATADOG, TRACECONTEXT] | '1' | '2' | | | 'W3C_TRACE_STATE_NO_P' | '1' | 'W3C_SPAN_ID_LSTR' | '0000000000000002'", + "DATADOG,TRACECONTEXT malformed span id | [DATADOG, TRACECONTEXT] | '1' | 'abc' | | | 'W3C_TRACE_STATE_NO_P' | '1' | 'W3C_SPAN_ID_LSTR' | ", + "DATADOG,TRACECONTEXT signed span id | [DATADOG, TRACECONTEXT] | '1' | '+2' | | | 'W3C_TRACE_STATE_NO_P' | '1' | 'W3C_SPAN_ID_LSTR' | " }) void checkW3CTraceContextOverride( List styles,