[ASM] Fix access violation exception when reading WAF addresses #6510

NachoEchevarria · 2025-01-08T13:33:37Z

Summary of changes

There are some errors detected in real customers that seem to be related to the method Waf.GetKnownAddresses() All the following errors seem to be caused by the same issue: memory corruption.

The errors detected include:

exception:Type: System.AccessViolationException
Message: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

Unhandled Exception: System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'UnmanagedMemoryPool'.

System.NullReferenceException
   at System.SpanHelpers.IndexOfNullByte(Byte* searchSpace)
   at System.String.Ctor(SByte* value)
   at System.Runtime.InteropServices.Marshal.PtrToStringAnsi(IntPtr ptr)
   at Datadog.Trace.AppSec.Waf.NativeBindings.WafLibraryInvoker.GetKnownAddresses(IntPtr wafHandle)

The stack traces suggest that the problem would be originated in the WAF dll:

System.AccessViolationException
        C:\\Windows\\SYSTEM32\\ntdll.dll!<unknown>+a38ee
        C:\\Program Files\\Datadog\\.NET Tracer\\win-x64\\ddwaf.dll!<unknown>+4f34e
        Datadog.Trace.dll!.IL_STUB_PInvoke
        Datadog.Trace.dll!Datadog.Trace.AppSec.Waf.NativeBindings.WafLibraryInvoker.GetKnownAddresses
        Datadog.Trace.dll!Datadog.Trace.AppSec.Security.UpdateActiveAddresses

A new unit test was created. This unit test launches 100 parallel calls to the WAF GetKnownAddresses method. The test was able to reproduce the error, crashing sometimes the testhost.exe process.

It seems that the WAF builds a cache when receiving the first GetKnownAddresses request. Parallel requests seem to be causing a concurrency issue. A lock would prevent this issue. It would seem that we would only need to protect the GetKnownAddresses calls during the first call, but, when doing that, the unit test still failed. When locking all the calls to the GetKnownAddresses method, tests passed consistently. That's why a WriteLock has been used.

Anyway, this method is only called when there are RC changes, so the performance impact of the lock should be minimal.

Reason for change

We were getting some of these exceptions logs from real customers.

Implementation details

Test coverage

Other details

datadog-ddstaging · 2025-01-08T14:05:57Z

Datadog Report

Branch report: nacho/FixAccessViolationException
Commit report: 3eb9985
Test service: dd-trace-dotnet

✅ 0 Failed, 235923 Passed, 2092 Skipped, 19h 29m 8.57s Total Time

andrewlock · 2025-01-08T14:41:41Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6510) - mean (69ms)  : 66, 71
     .   : milestone, 69,
    master - mean (68ms)  : 66, 71
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (6510) - mean (979ms)  : 958, 1000
     .   : milestone, 979,
    master - mean (979ms)  : 957, 1000
     .   : milestone, 979,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6510) - mean (108ms)  : 104, 112
     .   : milestone, 108,
    master - mean (108ms)  : 105, 110
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (6510) - mean (679ms)  : 664, 693
     .   : milestone, 679,
    master - mean (682ms)  : 668, 696
     .   : milestone, 682,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6510) - mean (92ms)  : 90, 94
     .   : milestone, 92,
    master - mean (91ms)  : 89, 93
     .   : milestone, 91,

    section CallTarget+Inlining+NGEN
    This PR (6510) - mean (632ms)  : 616, 648
     .   : milestone, 632,
    master - mean (639ms)  : 625, 654
     .   : milestone, 639,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6510) - mean (195ms)  : 191, 200
     .   : milestone, 195,
    master - mean (195ms)  : 190, 200
     .   : milestone, 195,

    section CallTarget+Inlining+NGEN
    This PR (6510) - mean (1,108ms)  : 1084, 1131
     .   : milestone, 1108,
    master - mean (1,099ms)  : 1062, 1136
     .   : milestone, 1099,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6510) - mean (280ms)  : 275, 285
     .   : milestone, 280,
    master - mean (278ms)  : 274, 283
     .   : milestone, 278,

    section CallTarget+Inlining+NGEN
    This PR (6510) - mean (870ms)  : 850, 890
     .   : milestone, 870,
    master - mean (869ms)  : 838, 899
     .   : milestone, 869,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6510) - mean (269ms)  : 263, 274
     .   : milestone, 269,
    master - mean (266ms)  : 262, 270
     .   : milestone, 266,

    section CallTarget+Inlining+NGEN
    This PR (6510) - mean (849ms)  : 822, 877
     .   : milestone, 849,
    master - mean (845ms)  : 814, 877
     .   : milestone, 845,

andrewlock · 2025-01-08T15:26:29Z

Benchmarks Report for appsec 🐌

Benchmarks for #6510 compared to master:

1 benchmarks are faster, with geometric mean 1.264
2 benchmarks are slower, with geometric mean 1.137
1 benchmarks have fewer allocations
1 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6510

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody‑net6.0	1.115	185,216.08	206,536.14

Faster 🎉 in #6510

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net6.0	1.264	178.26	140.98

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	185μs	51.2ns	185ns	2.6	0	188.9 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	287μs	153ns	591ns	2.59	0	196.23 KB
master	`AllCycleSimpleBody`	net472	250μs	92.8ns	335ns	35.8	2	225.95 KB
master	`AllCycleMoreComplexBody`	net6.0	191μs	97.2ns	376ns	2.68	0	192.41 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	290μs	142ns	532ns	2.61	0	199.65 KB
master	`AllCycleMoreComplexBody`	net472	254μs	79.5ns	287ns	36.5	2.03	229.46 KB
master	`ObjectExtractorSimpleBody`	net6.0	178ns	0.0979ns	0.379ns	0.00394	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	195ns	0.125ns	0.469ns	0.00364	0	272 B
master	`ObjectExtractorSimpleBody`	net472	163ns	0.219ns	0.849ns	0.0446	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	3.02μs	9.11ns	35.3ns	0.0538	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.85μs	2.81ns	10.5ns	0.0502	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	3.6μs	2.76ns	10.7ns	0.603	0.0054	3.8 KB
#6510	`AllCycleSimpleBody`	net6.0	206μs	104ns	389ns	2.69	0	188.76 KB
#6510	`AllCycleSimpleBody`	netcoreapp3.1	300μs	151ns	565ns	2.7	0	196.09 KB
#6510	`AllCycleSimpleBody`	net472	266μs	134ns	520ns	35.8	2	225.78 KB
#6510	`AllCycleMoreComplexBody`	net6.0	211μs	116ns	449ns	2.64	0	192.26 KB
#6510	`AllCycleMoreComplexBody`	netcoreapp3.1	313μs	123ns	477ns	2.66	0	199.5 KB
#6510	`AllCycleMoreComplexBody`	net472	276μs	163ns	632ns	36.4	2.04	229.3 KB
#6510	`ObjectExtractorSimpleBody`	net6.0	141ns	0.156ns	0.605ns	0.00393	0	280 B
#6510	`ObjectExtractorSimpleBody`	netcoreapp3.1	187ns	0.154ns	0.576ns	0.00366	0	272 B
#6510	`ObjectExtractorSimpleBody`	net472	166ns	0.125ns	0.467ns	0.0446	0	281 B
#6510	`ObjectExtractorMoreComplexBody`	net6.0	3.01μs	2.61ns	9.39ns	0.0525	0	3.78 KB
#6510	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.88μs	3.39ns	13.1ns	0.0503	0	3.69 KB
#6510	`ObjectExtractorMoreComplexBody`	net472	3.6μs	3.77ns	14.6ns	0.603	0.00538	3.8 KB

Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EncodeArgs`	net6.0	35.9μs	16.1ns	60.4ns	0.453	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54.1μs	28.1ns	109ns	0.43	0	32.4 KB
master	`EncodeArgs`	net472	67.7μs	84.2ns	326ns	5.15	0.0674	32.5 KB
master	`EncodeLegacyArgs`	net6.0	78.3μs	56ns	217ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	107μs	282ns	1.09μs	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	155μs	56ns	209ns	0.31	0	2.15 KB
#6510	`EncodeArgs`	net6.0	37.2μs	19.6ns	75.7ns	0.463	0	32.4 KB
#6510	`EncodeArgs`	netcoreapp3.1	54.8μs	41.6ns	156ns	0.434	0	32.4 KB
#6510	`EncodeArgs`	net472	66.9μs	49.5ns	192ns	5.14	0.0664	32.5 KB
#6510	`EncodeLegacyArgs`	net6.0	75.1μs	43.4ns	162ns	0	0	2.14 KB
#6510	`EncodeLegacyArgs`	netcoreapp3.1	109μs	133ns	516ns	0	0	2.14 KB
#6510	`EncodeLegacyArgs`	net472	156μs	85.8ns	332ns	0.314	0	2.15 KB

Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunWafRealisticBenchmark`	net6.0	179μs	245ns	950ns	0	2.44 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	186μs	286ns	1.11μs	0	2.39 KB
master	`RunWafRealisticBenchmark`	net472	200μs	66ns	247ns	0.3	2.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	115μs	80.4ns	311ns	0	1.47 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	124μs	114ns	428ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	133μs	106ns	412ns	0.2	1.49 KB
#6510	`RunWafRealisticBenchmark`	net6.0	175μs	418ns	1.62μs	0	2.44 KB
#6510	`RunWafRealisticBenchmark`	netcoreapp3.1	187μs	172ns	643ns	0	2.39 KB
#6510	`RunWafRealisticBenchmark`	net472	201μs	121ns	470ns	0.3	2.46 KB
#6510	`RunWafRealisticBenchmarkWithAttack`	net6.0	115μs	92.6ns	359ns	0	1.47 KB
#6510	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	123μs	241ns	934ns	0	1.46 KB
#6510	`RunWafRealisticBenchmarkWithAttack`	net472	133μs	56.9ns	220ns	0.201	1.49 KB

Benchmarks.Trace.Iast.StringAspectsBenchmark - Slower ⚠️ More allocations ⚠️

Slower ⚠️ in #6510

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net6.0	1.159	53,100.00	61,550.00	bimodal

More allocations ⚠️ in #6510

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	59.07 KB	59.46 KB	384 B	0.65%

Fewer allocations 🎉 in #6510

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0	263.9 KB	255.26 KB	-8.64 KB	-3.27%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	53.2μs	259ns	1.4μs	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	53μs	237ns	853ns	42.64 KB
master	`StringConcatBenchmark`	net472	37.7μs	103ns	371ns	59.07 KB
master	`StringConcatAspectBenchmark`	net6.0	316μs	1.79μs	13μs	263.9 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	349μs	1.98μs	14.4μs	252.85 KB
master	`StringConcatAspectBenchmark`	net472	279μs	6μs	57.5μs	278.53 KB
#6510	`StringConcatBenchmark`	net6.0	61.1μs	695ns	6.88μs	43.44 KB
#6510	`StringConcatBenchmark`	netcoreapp3.1	53.6μs	236ns	817ns	42.64 KB
#6510	`StringConcatBenchmark`	net472	37.9μs	68.4ns	256ns	59.46 KB
#6510	`StringConcatAspectBenchmark`	net6.0	310μs	1.18μs	4.07μs	255.26 KB
#6510	`StringConcatAspectBenchmark`	netcoreapp3.1	336μs	1.96μs	16.9μs	253.86 KB
#6510	`StringConcatAspectBenchmark`	net472	276μs	5.49μs	52.9μs	278.53 KB

andrewlock · 2025-01-08T15:48:32Z

Benchmarks Report for tracer 🐌

Benchmarks for #6510 compared to master:

1 benchmarks are faster, with geometric mean 1.152
1 benchmarks have fewer allocations
1 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`StartStopWithChild`	net6.0	8.04μs	45.4ns	318ns	0.0158	0.0079	0	5.6 KB
master	`StartStopWithChild`	netcoreapp3.1	10.1μs	54.7ns	314ns	0.0193	0.00964	0	5.8 KB
master	`StartStopWithChild`	net472	16.4μs	58.2ns	225ns	1.04	0.303	0.0957	6.21 KB
#6510	`StartStopWithChild`	net6.0	7.85μs	42.6ns	259ns	0.0198	0.00791	0	5.61 KB
#6510	`StartStopWithChild`	netcoreapp3.1	10.2μs	55.6ns	347ns	0.0202	0.0101	0	5.81 KB
#6510	`StartStopWithChild`	net472	16.4μs	32ns	120ns	1.05	0.317	0.106	6.2 KB

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	497μs	267ns	1.04μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	655μs	404ns	1.51μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	858μs	944ns	3.66μs	0.425	3.3 KB
#6510	`WriteAndFlushEnrichedTraces`	net6.0	485μs	346ns	1.34μs	0	2.7 KB
#6510	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	679μs	560ns	2.17μs	0	2.7 KB
#6510	`WriteAndFlushEnrichedTraces`	net472	858μs	572ns	2.22μs	0.425	3.3 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net6.0	158μs	1.06μs	10.4μs	0.145	14.47 KB
master	`SendRequest`	netcoreapp3.1	173μs	1.09μs	10.7μs	0.19	17.27 KB
master	`SendRequest`	net472	0.00296ns	0.00105ns	0.00395ns	0	0 b
#6510	`SendRequest`	net6.0	158μs	1.16μs	11.5μs	0.148	14.47 KB
#6510	`SendRequest`	netcoreapp3.1	174μs	1.25μs	12.3μs	0.181	17.27 KB
#6510	`SendRequest`	net472	0.0127ns	0.00364ns	0.0141ns	0	0 b

Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6510

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1	41.29 KB	41.69 KB	398 B	0.96%

Fewer allocations 🎉 in #6510

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑net6.0	41.76 KB	41.51 KB	-248 B	-0.59%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	570μs	2.81μs	12.9μs	0.539	0	0	41.76 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	682μs	3.89μs	27.5μs	0.319	0	0	41.29 KB
master	`WriteAndFlushEnrichedTraces`	net472	813μs	3.11μs	12μs	8.41	2.4	0.401	53.31 KB
#6510	`WriteAndFlushEnrichedTraces`	net6.0	552μs	2.93μs	17.6μs	0.558	0	0	41.51 KB
#6510	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	656μs	3.3μs	14.8μs	0.336	0	0	41.69 KB
#6510	`WriteAndFlushEnrichedTraces`	net472	853μs	4.25μs	18.5μs	8.13	2.57	0.428	53.31 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net6.0	1.37μs	1.23ns	4.75ns	0.0143	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.75μs	1.37ns	5.12ns	0.014	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.14μs	4.09ns	15.9ns	0.156	0.00106	987 B
#6510	`ExecuteNonQuery`	net6.0	1.36μs	1.29ns	4.99ns	0.0142	0	1.02 KB
#6510	`ExecuteNonQuery`	netcoreapp3.1	1.77μs	1.12ns	4.19ns	0.0132	0	1.02 KB
#6510	`ExecuteNonQuery`	net472	2.07μs	3.07ns	11.9ns	0.156	0.00103	987 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net6.0	1.22μs	0.649ns	2.43ns	0.0135	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.54μs	0.413ns	1.49ns	0.013	976 B
master	`CallElasticsearch`	net472	2.65μs	2.16ns	8.08ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.36μs	0.875ns	3.39ns	0.0129	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	1.09ns	4.09ns	0.0132	1.02 KB
master	`CallElasticsearchAsync`	net472	2.59μs	1.53ns	5.72ns	0.166	1.05 KB
#6510	`CallElasticsearch`	net6.0	1.3μs	0.381ns	1.48ns	0.0137	976 B
#6510	`CallElasticsearch`	netcoreapp3.1	1.5μs	0.766ns	2.97ns	0.0127	976 B
#6510	`CallElasticsearch`	net472	2.65μs	1.88ns	7.28ns	0.158	995 B
#6510	`CallElasticsearchAsync`	net6.0	1.29μs	0.892ns	3.46ns	0.0129	952 B
#6510	`CallElasticsearchAsync`	netcoreapp3.1	1.71μs	1.66ns	6.42ns	0.0137	1.02 KB
#6510	`CallElasticsearchAsync`	net472	2.73μs	1.18ns	4.42ns	0.166	1.05 KB

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.29μs	0.716ns	2.68ns	0.0134	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.62μs	0.453ns	1.63ns	0.0129	952 B
master	`ExecuteAsync`	net472	1.78μs	0.388ns	1.45ns	0.145	915 B
#6510	`ExecuteAsync`	net6.0	1.2μs	0.656ns	2.45ns	0.0132	952 B
#6510	`ExecuteAsync`	netcoreapp3.1	1.63μs	1.09ns	4.06ns	0.0129	952 B
#6510	`ExecuteAsync`	net472	1.78μs	0.786ns	3.04ns	0.145	915 B

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net6.0	4.49μs	2.74ns	10.6ns	0.0315	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.31μs	2.41ns	9.02ns	0.0386	2.85 KB
master	`SendAsync`	net472	7.32μs	2.26ns	8.75ns	0.495	3.12 KB
#6510	`SendAsync`	net6.0	4.41μs	2.56ns	9.9ns	0.0311	2.31 KB
#6510	`SendAsync`	netcoreapp3.1	5.37μs	2ns	7.74ns	0.0376	2.85 KB
#6510	`SendAsync`	net472	7.28μs	2.13ns	8.25ns	0.494	3.12 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.58μs	0.604ns	2.18ns	0.023	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.28μs	0.753ns	2.82ns	0.0216	1.64 KB
master	`EnrichedLog`	net472	2.71μs	1.07ns	4.16ns	0.25	1.57 KB
#6510	`EnrichedLog`	net6.0	1.47μs	0.68ns	2.54ns	0.0229	1.64 KB
#6510	`EnrichedLog`	netcoreapp3.1	2.14μs	1.29ns	4.83ns	0.0225	1.64 KB
#6510	`EnrichedLog`	net472	2.83μs	2.32ns	8.97ns	0.249	1.57 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net6.0	118μs	185ns	716ns	0.0579	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	121μs	234ns	906ns	0	0	4.28 KB
master	`EnrichedLog`	net472	151μs	111ns	416ns	0.683	0.228	4.46 KB
#6510	`EnrichedLog`	net6.0	118μs	150ns	543ns	0	0	4.28 KB
#6510	`EnrichedLog`	netcoreapp3.1	120μs	127ns	456ns	0.0597	0	4.28 KB
#6510	`EnrichedLog`	net472	151μs	297ns	1.15μs	0.678	0.226	4.46 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	3.07μs	1.38ns	5.34ns	0.0309	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.1μs	1.52ns	5.88ns	0.0284	2.2 KB
master	`EnrichedLog`	net472	4.85μs	1.48ns	5.74ns	0.32	2.02 KB
#6510	`EnrichedLog`	net6.0	2.99μs	1.32ns	5.1ns	0.0315	2.2 KB
#6510	`EnrichedLog`	netcoreapp3.1	4.18μs	2.15ns	8.34ns	0.029	2.2 KB
#6510	`EnrichedLog`	net472	4.93μs	1.36ns	5.07ns	0.318	2.02 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net6.0	1.45μs	0.662ns	2.56ns	0.0159	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.75μs	0.776ns	3ns	0.0149	1.14 KB
master	`SendReceive`	net472	2.2μs	1.96ns	7.34ns	0.183	1.16 KB
#6510	`SendReceive`	net6.0	1.36μs	0.482ns	1.8ns	0.0157	1.14 KB
#6510	`SendReceive`	netcoreapp3.1	1.8μs	1.33ns	5.16ns	0.0152	1.14 KB
#6510	`SendReceive`	net472	2.02μs	1.5ns	5.41ns	0.183	1.16 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.72μs	0.639ns	2.39ns	0.022	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.95μs	2.64ns	10.2ns	0.0218	1.65 KB
master	`EnrichedLog`	net472	4.38μs	2.57ns	9.62ns	0.322	2.04 KB
#6510	`EnrichedLog`	net6.0	2.74μs	0.759ns	2.94ns	0.0219	1.6 KB
#6510	`EnrichedLog`	netcoreapp3.1	3.97μs	1.4ns	5.41ns	0.0218	1.65 KB
#6510	`EnrichedLog`	net472	4.46μs	2.45ns	9.18ns	0.323	2.04 KB

Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6510

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1	1.152	630.48	547.43

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	405ns	0.229ns	0.887ns	0.00811	576 B
master	`StartFinishSpan`	netcoreapp3.1	627ns	1.82ns	7.05ns	0.00787	576 B
master	`StartFinishSpan`	net472	668ns	0.476ns	1.84ns	0.0915	578 B
master	`StartFinishScope`	net6.0	480ns	0.414ns	1.6ns	0.00975	696 B
master	`StartFinishScope`	netcoreapp3.1	747ns	0.676ns	2.62ns	0.00951	696 B
master	`StartFinishScope`	net472	899ns	0.539ns	2.09ns	0.105	658 B
#6510	`StartFinishSpan`	net6.0	403ns	0.369ns	1.43ns	0.00812	576 B
#6510	`StartFinishSpan`	netcoreapp3.1	547ns	0.439ns	1.7ns	0.00787	576 B
#6510	`StartFinishSpan`	net472	663ns	0.311ns	1.2ns	0.0915	578 B
#6510	`StartFinishScope`	net6.0	505ns	0.285ns	1.11ns	0.00987	696 B
#6510	`StartFinishScope`	netcoreapp3.1	761ns	0.455ns	1.58ns	0.00918	696 B
#6510	`StartFinishScope`	net472	895ns	0.591ns	2.29ns	0.104	658 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	707ns	0.623ns	2.41ns	0.00991	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	896ns	0.904ns	3.5ns	0.00945	696 B
master	`RunOnMethodBegin`	net472	1.07μs	0.802ns	3.11ns	0.104	658 B
#6510	`RunOnMethodBegin`	net6.0	681ns	0.611ns	2.37ns	0.00993	696 B
#6510	`RunOnMethodBegin`	netcoreapp3.1	933ns	2.91ns	11.3ns	0.00919	696 B
#6510	`RunOnMethodBegin`	net472	1.18μs	0.801ns	3.1ns	0.104	658 B

daniel-romano-DD · 2025-01-09T11:00:44Z

tracer/src/Datadog.Trace/AppSec/Waf/Waf.cs

@@ -113,7 +113,11 @@ public bool IsKnowAddressesSuported()

        public string[] GetKnownAddresses()
        {
-            return _wafLibraryInvoker.GetKnownAddresses(_wafHandle);
+            _wafLocker.EnterWriteLock();


Wouldn't this be handled in the waf, a it is an internal waf issue?

I talked to Anil and he mentioned that we should handle this concurrency issue. AFAIK, there are some other WAF calls that can have some concurrency issues that we should handle. Still, it could be a good topic for discussion if the WAF should handle it's own concurrency problems.

Also, _wafLibraryInvoker.GetKnownAddresses(_wafHandle); is already completely inside a try/catch block. Adding a new top level one would be needed?

daniel-romano-DD · 2025-01-09T11:01:16Z

tracer/src/Datadog.Trace/AppSec/Waf/Waf.cs

-            return _wafLibraryInvoker.GetKnownAddresses(_wafHandle);
+            _wafLocker.EnterWriteLock();
+            var result = _wafLibraryInvoker.GetKnownAddresses(_wafHandle);
+            _wafLocker.ExitWriteLock();


This could be try/catched. The Exit should be in a finally clause

Sure, I can do that. Still, it will not catch the System.AccessViolationException if it happens. It just crashes the process.

Seeing a lock release outside a scope guard just rings my alarms. But, up to you.

I have moved the try/catch block one level up. I hope this addresses any concerns.

I believe the concern is that if this throws, you'll never release the lock. I would expect something like this:

try { _wafLocker.EnterWriteLock(); var result = _wafLibraryInvoker.GetKnownAddresses(_wafHandle); } finally { _wafLocker.ExitWriteLock(); }

But then, if you're crashing the process anyway 🤷‍♂️

NachoEchevarria · 2025-01-09T11:35:42Z

tracer/src/Datadog.Trace/AppSec/Waf/NativeBindings/WafLibraryInvoker.cs

@@ -245,11 +245,6 @@ internal string[] GetKnownAddresses(IntPtr wafHandle)
        {
            try
            {
-                if (!IsKnowAddressesSuported())


We already check that in Security.UpdateActiveAddresses

daniel-romano-DD

With come comments and concerns.

NachoEchevarria · 2025-01-09T13:08:32Z

With come comments and concerns.

I have moved the try/catch block one level up. I hope this addresses any concerns.

robertpi

LGTM, thanks!

NachoEchevarria · 2025-01-10T08:20:31Z

Thanks for the feedback and reviews!

NachoEchevarria added 2 commits January 8, 2025 11:19

Add lock

42ba83e

Add unit tests

e26f469

NachoEchevarria changed the title ~~Nacho/fix access violation exception~~ [ASM] Fix access violation exception when reading WAF addresses Jan 8, 2025

NachoEchevarria added the area:asm label Jan 8, 2025

Merge branch 'master' into nacho/FixAccessViolationException

22f9429

NachoEchevarria marked this pull request as ready for review January 8, 2025 14:03

NachoEchevarria requested a review from a team as a code owner January 8, 2025 14:03

Merge branch 'master' into nacho/FixAccessViolationException

9530e88

daniel-romano-DD reviewed Jan 9, 2025

View reviewed changes

add if statement. Remove not needed.

29df674

NachoEchevarria commented Jan 9, 2025

View reviewed changes

daniel-romano-DD approved these changes Jan 9, 2025

View reviewed changes

Move try/catch block one level up

3eb9985

robertpi approved these changes Jan 9, 2025

View reviewed changes

NachoEchevarria merged commit fc3a5d3 into master Jan 10, 2025
60 of 64 checks passed

NachoEchevarria deleted the nacho/FixAccessViolationException branch January 10, 2025 08:20

github-actions bot added this to the vNext-v3 milestone Jan 10, 2025

[ASM] Fix access violation exception when reading WAF addresses #6510

[ASM] Fix access violation exception when reading WAF addresses #6510

Conversation

NachoEchevarria commented Jan 8, 2025 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

datadog-ddstaging bot commented Jan 8, 2025 • edited Loading

Datadog Report

andrewlock commented Jan 8, 2025 • edited Loading

Execution-Time Benchmarks Report ⏱️

andrewlock commented Jan 8, 2025 • edited Loading

Benchmarks Report for appsec 🐌

Benchmark details

Slower ⚠️ in #6510

Faster 🎉 in #6510

Raw results

Raw results

Raw results

Slower ⚠️ in #6510

More allocations ⚠️ in #6510

Fewer allocations 🎉 in #6510

Raw results

andrewlock commented Jan 8, 2025 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

More allocations ⚠️ in #6510

Fewer allocations 🎉 in #6510

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #6510

Raw results

Raw results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-romano-DD left a comment

Choose a reason for hiding this comment

NachoEchevarria commented Jan 9, 2025

robertpi left a comment

Choose a reason for hiding this comment

NachoEchevarria commented Jan 10, 2025

NachoEchevarria commented Jan 8, 2025 •

edited

Loading

datadog-ddstaging bot commented Jan 8, 2025 •

edited

Loading

andrewlock commented Jan 8, 2025 •

edited

Loading

andrewlock commented Jan 8, 2025 •

edited

Loading

andrewlock commented Jan 8, 2025 •

edited

Loading