Skip to content

Commit d0b1258

Browse files
authored
Fix thread-safety in AppSecRequestContext derivatives field (#9923)
The derivatives field in AppSecRequestContext used a volatile Map that didn't guarantee atomicity for read-modify-write operations. This could cause race conditions when multiple threads tried to report derivatives simultaneously. Migrates the derivatives field from: private volatile Map<String, Object> derivatives; to: private final AtomicReference<Map<String, Object>> derivatives = new AtomicReference<>();
1 parent 7aef6ed commit d0b1258

File tree

3 files changed

+332
-57
lines changed

3 files changed

+332
-57
lines changed

dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java

Lines changed: 60 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.concurrent.atomic.AtomicBoolean;
3636
import java.util.concurrent.atomic.AtomicInteger;
3737
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
38+
import java.util.concurrent.atomic.AtomicReference;
3839
import org.slf4j.Logger;
3940
import org.slf4j.LoggerFactory;
4041

@@ -140,7 +141,7 @@ public class AppSecRequestContext implements DataBundle, Closeable {
140141
private boolean responseBodyPublished;
141142
private boolean respDataPublished;
142143
private boolean pathParamsPublished;
143-
private volatile Map<String, Object> derivatives;
144+
private final AtomicReference<Map<String, Object>> derivatives = new AtomicReference<>();
144145

145146
private final AtomicBoolean rateLimited = new AtomicBoolean(false);
146147
private volatile boolean throttled;
@@ -649,9 +650,9 @@ public void close() {
649650
requestHeaders.clear();
650651
responseHeaders.clear();
651652
persistentData.clear();
653+
final Map<String, Object> derivatives = this.derivatives.getAndSet(null);
652654
if (derivatives != null) {
653655
derivatives.clear();
654-
derivatives = null;
655656
}
656657
}
657658
}
@@ -743,54 +744,57 @@ public void reportDerivatives(Map<String, Object> data) {
743744
log.debug("Reporting derivatives: {}", data);
744745
if (data == null || data.isEmpty()) return;
745746

746-
// Store raw derivatives
747-
if (derivatives == null) {
748-
derivatives = new HashMap<>();
749-
}
750-
751-
// Process each attribute according to the specification
752-
for (Map.Entry<String, Object> entry : data.entrySet()) {
753-
String attributeKey = entry.getKey();
754-
Object attributeConfig = entry.getValue();
755-
756-
if (attributeConfig instanceof Map) {
757-
@SuppressWarnings("unchecked")
758-
Map<String, Object> config = (Map<String, Object>) attributeConfig;
759-
760-
// Check if it's a literal value schema
761-
if (config.containsKey("value")) {
762-
Object literalValue = config.get("value");
763-
if (literalValue != null) {
764-
// Preserve the original type - don't convert to string
765-
derivatives.put(attributeKey, literalValue);
766-
log.debug(
767-
"Added literal attribute: {} = {} (type: {})",
768-
attributeKey,
769-
literalValue,
770-
literalValue.getClass().getSimpleName());
747+
// Initialize or update derivatives atomically
748+
derivatives.updateAndGet(
749+
current -> {
750+
Map<String, Object> updated = current != null ? new HashMap<>(current) : new HashMap<>();
751+
752+
// Process each attribute according to the specification
753+
for (Map.Entry<String, Object> entry : data.entrySet()) {
754+
String attributeKey = entry.getKey();
755+
Object attributeConfig = entry.getValue();
756+
757+
if (attributeConfig instanceof Map) {
758+
@SuppressWarnings("unchecked")
759+
Map<String, Object> config = (Map<String, Object>) attributeConfig;
760+
761+
// Check if it's a literal value schema
762+
if (config.containsKey("value")) {
763+
Object literalValue = config.get("value");
764+
if (literalValue != null) {
765+
// Preserve the original type - don't convert to string
766+
updated.put(attributeKey, literalValue);
767+
log.debug(
768+
"Added literal attribute: {} = {} (type: {})",
769+
attributeKey,
770+
literalValue,
771+
literalValue.getClass().getSimpleName());
772+
}
773+
}
774+
// Check if it's a request data schema
775+
else if (config.containsKey("address")) {
776+
String address = (String) config.get("address");
777+
@SuppressWarnings("unchecked")
778+
List<String> keyPath = (List<String>) config.get("key_path");
779+
@SuppressWarnings("unchecked")
780+
List<String> transformers = (List<String>) config.get("transformers");
781+
782+
Object extractedValue = extractValueFromRequestData(address, keyPath, transformers);
783+
if (extractedValue != null) {
784+
// For extracted values, convert to string as they come from request data
785+
updated.put(attributeKey, extractedValue.toString());
786+
log.debug("Added extracted attribute: {} = {}", attributeKey, extractedValue);
787+
}
788+
}
789+
} else {
790+
// Handle plain string/numeric values
791+
updated.put(attributeKey, attributeConfig);
792+
log.debug("Added direct attribute: {} = {}", attributeKey, attributeConfig);
793+
}
771794
}
772-
}
773-
// Check if it's a request data schema
774-
else if (config.containsKey("address")) {
775-
String address = (String) config.get("address");
776-
@SuppressWarnings("unchecked")
777-
List<String> keyPath = (List<String>) config.get("key_path");
778-
@SuppressWarnings("unchecked")
779-
List<String> transformers = (List<String>) config.get("transformers");
780795

781-
Object extractedValue = extractValueFromRequestData(address, keyPath, transformers);
782-
if (extractedValue != null) {
783-
// For extracted values, convert to string as they come from request data
784-
derivatives.put(attributeKey, extractedValue.toString());
785-
log.debug("Added extracted attribute: {} = {}", attributeKey, extractedValue);
786-
}
787-
}
788-
} else {
789-
// Handle plain string/numeric values
790-
derivatives.put(attributeKey, attributeConfig);
791-
log.debug("Added direct attribute: {} = {}", attributeKey, attributeConfig);
792-
}
793-
}
796+
return updated;
797+
});
794798
}
795799

796800
/**
@@ -938,14 +942,17 @@ private Object applyTransformers(Object value, List<String> transformers) {
938942
}
939943

940944
public boolean commitDerivatives(TraceSegment traceSegment) {
941-
log.debug("Committing derivatives: {} for {}", derivatives, traceSegment);
942945
if (traceSegment == null) {
943946
return false;
944947
}
945948

949+
// Get and clear derivatives atomically
950+
Map<String, Object> derivativesToCommit = derivatives.getAndSet(null);
951+
log.debug("Committing derivatives: {} for {}", derivativesToCommit, traceSegment);
952+
946953
// Process and commit derivatives directly
947-
if (derivatives != null && !derivatives.isEmpty()) {
948-
for (Map.Entry<String, Object> entry : derivatives.entrySet()) {
954+
if (derivativesToCommit != null && !derivativesToCommit.isEmpty()) {
955+
for (Map.Entry<String, Object> entry : derivativesToCommit.entrySet()) {
949956
String key = entry.getKey();
950957
Object value = entry.getValue();
951958

@@ -969,14 +976,13 @@ public boolean commitDerivatives(TraceSegment traceSegment) {
969976
}
970977
}
971978

972-
// Clear all attribute maps
973-
derivatives = null;
974979
return true;
975980
}
976981

977982
// Mainly used for testing and logging
978983
Set<String> getDerivativeKeys() {
979-
return derivatives == null ? emptySet() : new HashSet<>(derivatives.keySet());
984+
Map<String, Object> current = derivatives.get();
985+
return current == null ? emptySet() : new HashSet<>(current.keySet());
980986
}
981987

982988
public boolean isThrottled(RateLimiter rateLimiter) {

0 commit comments

Comments
 (0)