Returns invalid traceflags for a bad string parse instead of throwing…

… an exception.
open-telemetry · Feb 26, 2021 · 70fa3a2 · 70fa3a2
1 parent f1c38ef
commit 70fa3a2
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 22 deletions.
diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/ImmutableSpanContext.java b/api/all/src/main/java/io/opentelemetry/api/trace/ImmutableSpanContext.java
@@ -38,7 +38,7 @@ static SpanContext create(
       TraceFlags traceFlags,
       TraceState traceState,
       boolean remote) {
-    if (SpanId.isValid(spanIdHex) && TraceId.isValid(traceIdHex)) {
+    if (SpanId.isValid(spanIdHex) && TraceId.isValid(traceIdHex) && traceFlags.isValid()) {
       return createInternal(
           traceIdHex, spanIdHex, traceFlags, traceState, remote, /* valid= */ true);
     }

diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/ImmutableTraceFlags.java b/api/all/src/main/java/io/opentelemetry/api/trace/ImmutableTraceFlags.java
@@ -6,7 +6,6 @@
 package io.opentelemetry.api.trace;
 
 import io.opentelemetry.api.internal.OtelEncodingUtils;
-import java.util.Objects;
 import javax.annotation.concurrent.Immutable;
 
 @Immutable
@@ -15,6 +14,7 @@ final class ImmutableTraceFlags implements TraceFlags {
   // Bit to represent whether trace is sampled or not.
   private static final byte SAMPLED_BIT = 0x01;
 
+  private static final ImmutableTraceFlags INVALID = new ImmutableTraceFlags((byte) 0);
   static final ImmutableTraceFlags DEFAULT = fromByte((byte) 0x00);
   static final ImmutableTraceFlags SAMPLED = fromByte(SAMPLED_BIT);
   static final int HEX_LENGTH = 2;
@@ -24,7 +24,12 @@ final class ImmutableTraceFlags implements TraceFlags {
 
   // Implementation of the TraceFlags.fromHex().
   static ImmutableTraceFlags fromHex(CharSequence src, int srcOffset) {
-    Objects.requireNonNull(src, "src");
+    // TODO: Avoid calling `subSequence`.
+    if (src == null
+        || src.length() < srcOffset + 2
+        || !OtelEncodingUtils.isValidBase16String(src.subSequence(srcOffset, srcOffset + 2))) {
+      return INVALID;
+    }
     return fromByte(
         OtelEncodingUtils.byteFromBase16(src.charAt(srcOffset), src.charAt(srcOffset + 1)));
   }
@@ -65,6 +70,11 @@ public byte asByte() {
     return this.byteRep;
   }
 
+  @Override
+  public boolean isValid() {
+    return this != INVALID;
+  }
+
   @Override
   public String toString() {
     return asHex();

diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/SpanContext.java b/api/all/src/main/java/io/opentelemetry/api/trace/SpanContext.java
@@ -135,7 +135,9 @@ default boolean isSampled() {
    * @return {@code true} if this {@code SpanContext} is valid.
    */
   default boolean isValid() {
-    return TraceId.isValid(getTraceId()) && SpanId.isValid(getSpanId());
+    return TraceId.isValid(getTraceId())
+        && SpanId.isValid(getSpanId())
+        && getTraceFlags().isValid();
   }
 
   /**

diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/TraceFlags.java b/api/all/src/main/java/io/opentelemetry/api/trace/TraceFlags.java
@@ -90,4 +90,10 @@ static TraceFlags fromByte(byte traceFlagsByte) {
    * @return the byte representation of the {@link TraceFlags}.
    */
   byte asByte();
+
+  /**
+   * Returns whether this {@link TraceFlags} is valid. An invalid {@link TraceFlags} should
+   * generally be discarded.
+   */
+  boolean isValid();
 }
diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/propagation/W3CTraceContextPropagator.java b/api/all/src/main/java/io/opentelemetry/api/trace/propagation/W3CTraceContextPropagator.java
@@ -207,26 +207,21 @@ private static SpanContext extractContextFromTraceParent(String traceparent) {
       return SpanContext.getInvalid();
     }
 
-    try {
-      String version = traceparent.substring(0, 2);
-      if (!VALID_VERSIONS.contains(version)) {
-        return SpanContext.getInvalid();
-      }
-      if (version.equals(VERSION_00) && traceparent.length() > TRACEPARENT_HEADER_SIZE) {
-        return SpanContext.getInvalid();
-      }
-
-      String traceId =
-          traceparent.substring(TRACE_ID_OFFSET, TRACE_ID_OFFSET + TraceId.getLength());
-      String spanId = traceparent.substring(SPAN_ID_OFFSET, SPAN_ID_OFFSET + SpanId.getLength());
-
-      TraceFlags traceFlags = TraceFlags.fromHex(traceparent, TRACE_OPTION_OFFSET);
-      return SpanContext.createFromRemoteParent(
-          traceId, spanId, traceFlags, TraceState.getDefault());
-    } catch (IllegalArgumentException e) {
-      logger.fine("Unparseable traceparent header. Returning INVALID span context.");
+    String version = traceparent.substring(0, 2);
+    if (!VALID_VERSIONS.contains(version)) {
+      return SpanContext.getInvalid();
+    }
+    if (version.equals(VERSION_00) && traceparent.length() > TRACEPARENT_HEADER_SIZE) {
       return SpanContext.getInvalid();
     }
+
+    String traceId =
+        traceparent.substring(TRACE_ID_OFFSET, TRACE_ID_OFFSET + TraceId.getLength());
+    String spanId = traceparent.substring(SPAN_ID_OFFSET, SPAN_ID_OFFSET + SpanId.getLength());
+
+    TraceFlags traceFlags = TraceFlags.fromHex(traceparent, TRACE_OPTION_OFFSET);
+    return SpanContext.createFromRemoteParent(
+        traceId, spanId, traceFlags, TraceState.getDefault());
   }
 
   private static TraceState extractTraceState(String traceStateHeader) {

diff --git a/api/all/src/test/java/io/opentelemetry/api/trace/SpanContextTest.java b/api/all/src/test/java/io/opentelemetry/api/trace/SpanContextTest.java
@@ -56,6 +56,14 @@ void isValid() {
         .isFalse();
     assertThat(first.isValid()).isTrue();
     assertThat(second.isValid()).isTrue();
+    assertThat(
+            SpanContext.create(
+                    FIRST_TRACE_ID,
+                    FIRST_SPAN_ID,
+                    TraceFlags.fromHex("abc", 3),
+                    TraceState.getDefault())
+                .isValid())
+        .isFalse();
   }
 
   @Test

diff --git a/api/all/src/test/java/io/opentelemetry/api/trace/TraceFlagsTest.java b/api/all/src/test/java/io/opentelemetry/api/trace/TraceFlagsTest.java
@@ -37,6 +37,13 @@ void toFromHex() {
     }
   }
 
+  @Test
+  void toFromHex_Invalid() {
+    assertThat(TraceFlags.fromHex(null, 0).isValid()).isFalse();
+    assertThat(TraceFlags.fromHex("hex", 0).isValid()).isFalse();
+    assertThat(TraceFlags.fromHex("aa", 1).isValid()).isFalse();
+  }
+
   @Test
   void toFromByte() {
     for (int i = 0; i < 256; i++) {