[ASM] Prevent waf from breaking app if non compatible native library file is loaded by tracer by anna-git · Pull Request #3251 · DataDog/dd-trace-dotnet

anna-git · 2022-09-22T11:13:15Z

Summary of changes

If some export is null then it means we dont have the right underlying waf native file version, so dont init waf.
LibraryLoader can now load a specific version of the waf provided that the file's name contain it, for the purpose of Unit Tests only.
Added in Build.steps the ability to load an older version (arbitrarily 1.4 now) to rename the waf after unzipping

Reason for change

make sure waf's breaking change dont break the app

Implementation details

Depends on DataDog/system-tests#509

Test coverage

Add a unit test with older waf version to test.

Other details

dromanol · 2022-09-22T12:43:27Z

-                OSPlatformName.Windows => "ddwaf.dll",
+                OSPlatformName.MacOS => $"libddwaf{libVersion}.dylib",
+                OSPlatformName.Linux => $"libddwaf{libVersion}.so",
+                OSPlatformName.Windows => $"ddwaf{libVersion}.dll",


Maybe add an hyphen to separate the version if specified?

fixed in 596ff97, it was all to avoid the if 😅

robertpi · 2022-09-22T16:15:56Z

-    {
-        "win-x86", "win-x64"
-    };
+    readonly string[] WafWindowsArchitectureFolders = { "win-x86", "win-x64" };


Mostly the changes to this file are reformatting?

yes apart the added function to add an older waf file to the waf unit test's build directory , then ran into some trouble indenting it on Rider 😅

robertpi

LGTM, thanks for the quick solution!

andrewlock · 2022-09-22T16:34:35Z

        {
+            if (wafNative.ExportErrorHappened)
+            {
+                Log.Error("Waf couldn't initialize properly because of missing methods in native library, please check the version of libddwaf");


nit: I wonder, can we be more explicit about which version we expect here? Probably not 🤔 I think at the least we should give some specific guidance. e.g. make sure you don't have multiple versions of Datadog.Trace, install the latest, etc

we cant, unless we build some kinda compatibility matrice of tracer versions / waf versions..
I added more guidance in the comment in 9fe3b5d

andrewlock · 2022-09-22T16:37:49Z

            var configObj = GetConfigObj(rulesFile, argCache, encoder);
            if (configObj == null)
            {
+                Log.Error("Waf couldn't initialize properly because of an unusable rule file, please check the submitted rulefile");


Same comment here, is there any way to have something actionable for the user in the logs? e.g. check for multiple tracer versions etc etc "if you manually provided a rule file, make sure it's valid for the latest version of the Waf" etc

fixed in 9fe3b5d

andrewlock · 2022-09-22T16:39:00Z


        internal static InitializationResult Configure(string rulesFile, WafNative wafNative, Encoder encoder, string obfuscationParameterKeyRegex, string obfuscationParameterValueRegex)
        {
+            if (wafNative.ExportErrorHappened)


Doesn't need to be this PR, but I wonder, can/should we add this to telemetry/configuration log

It's a good idea, we already have the WAF version in the configuration log, so adding this should be straight forward.

I noticed there's quite a lot of stuff that's in the config log, but not startup telemetry, I guess we could add the missing stuff as "additional values" ?

I added this error in the startup log if it's true . As we want to have it for next release, I suggest we do a subsequent PR on adding security info to startup telemetry ( ConfigurationTelemetryCollector for me to remember 😅 )

andrewlock · 2022-09-23T11:05:51Z

Benchmarks Report 🐌

Benchmarks for #3251 compared to master:

All benchmarks have the same speed
All benchmarks have the same 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.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	720μs	926ns	3.34μs	0.355	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	453μs	365ns	1.41μs	0	2.58 KB
#3251	`WriteAndFlushEnrichedTraces`	net472	716μs	778ns	3.01μs	0.357	3.18 KB
#3251	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	463μs	133ns	479ns	0	2.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	197ns	0.0703ns	0.263ns	0.0676	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	255ns	0.269ns	1.04ns	0.00577	0	424 B
master	`AllCycleMoreComplexBody`	net472	196ns	0.063ns	0.218ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	253ns	0.153ns	0.573ns	0.00542	0	400 B
master	`BodyExtractorSimpleBody`	net472	280ns	0.138ns	0.517ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	232ns	0.166ns	0.621ns	0.00372	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	15.8μs	10.2ns	38.1ns	1.21	0.0158	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.3μs	5.06ns	19.6ns	0.0919	0	6.75 KB
#3251	`AllCycleSimpleBody`	net472	200ns	0.18ns	0.672ns	0.0675	0	425 B
#3251	`AllCycleSimpleBody`	netcoreapp3.1	256ns	0.156ns	0.604ns	0.00577	0	424 B
#3251	`AllCycleMoreComplexBody`	net472	195ns	0.591ns	2.29ns	0.0637	0	401 B
#3251	`AllCycleMoreComplexBody`	netcoreapp3.1	250ns	0.151ns	0.564ns	0.0054	0	400 B
#3251	`BodyExtractorSimpleBody`	net472	276ns	0.287ns	1.07ns	0.0573	0	361 B
#3251	`BodyExtractorSimpleBody`	netcoreapp3.1	235ns	0.139ns	0.519ns	0.00376	0	272 B
#3251	`BodyExtractorMoreComplexBody`	net472	16.4μs	16.1ns	62.4ns	1.2	0.0164	7.62 KB
#3251	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.3μs	6.15ns	23ns	0.0922	0	6.75 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	181μs	245ns	918ns	0.271	20.59 KB
#3251	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3251	`SendRequest`	netcoreapp3.1	181μs	282ns	1.09μs	0.18	20.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.86μs	1.22ns	4.39ns	0.15	0.000934	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.46μs	0.653ns	2.45ns	0.0124	0	936 B
#3251	`ExecuteNonQuery`	net472	1.97μs	1.38ns	4.96ns	0.15	0.000985	947 B
#3251	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	0.473ns	1.77ns	0.0129	0	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.61μs	1.84ns	6.9ns	0.182	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.51μs	0.888ns	3.32ns	0.0144	1.1 KB
master	`CallElasticsearchAsync`	net472	2.72μs	1.65ns	6.4ns	0.205	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.59μs	0.64ns	2.48ns	0.0166	1.22 KB
#3251	`CallElasticsearch`	net472	2.58μs	2.03ns	7.86ns	0.183	1.16 KB
#3251	`CallElasticsearch`	netcoreapp3.1	1.49μs	2.42ns	9.36ns	0.0154	1.1 KB
#3251	`CallElasticsearchAsync`	net472	2.79μs	1.76ns	6.6ns	0.205	1.29 KB
#3251	`CallElasticsearchAsync`	netcoreapp3.1	1.69μs	0.851ns	3.19ns	0.0161	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.9μs	2.49ns	9.63ns	0.225	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.8μs	0.916ns	3.43ns	0.0189	1.34 KB
#3251	`ExecuteAsync`	net472	2.79μs	2.74ns	10.3ns	0.226	1.42 KB
#3251	`ExecuteAsync`	netcoreapp3.1	1.76μs	0.913ns	3.42ns	0.0185	1.34 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.88μs	5.5ns	21.3ns	0.44	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.78μs	1.53ns	5.91ns	0.0341	2.6 KB
#3251	`SendAsync`	net472	5.84μs	3.59ns	13.9ns	0.44	2.77 KB
#3251	`SendAsync`	netcoreapp3.1	3.76μs	2.01ns	7.52ns	0.034	2.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	3.09μs	2.07ns	7.75ns	0.288	0	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.53μs	1.94ns	7.01ns	0.0254	0	1.85 KB
#3251	`EnrichedLog`	net472	3.15μs	4.69ns	18.1ns	0.288	0.00157	1.81 KB
#3251	`EnrichedLog`	netcoreapp3.1	2.48μs	2.45ns	9.17ns	0.0258	0	1.85 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	115ns	444ns	0.682	0.227	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	115μs	189ns	733ns	0.0581	0	4.49 KB
#3251	`EnrichedLog`	net472	150μs	164ns	636ns	0.674	0.225	4.65 KB
#3251	`EnrichedLog`	netcoreapp3.1	118μs	295ns	1.14μs	0.0597	0	4.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.97μs	5.03ns	18.1ns	0.568	0.00299	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.51μs	1.18ns	4.1ns	0.0518	0	3.91 KB
#3251	`EnrichedLog`	net472	5.94μs	2.32ns	8.98ns	0.568	0.00295	3.59 KB
#3251	`EnrichedLog`	netcoreapp3.1	4.56μs	2.84ns	11ns	0.0524	0	3.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.3μs	3.86ns	15ns	0.217	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.79μs	1.29ns	4.84ns	0.018	1.32 KB
#3251	`SendReceive`	net472	2.19μs	2.38ns	8.92ns	0.217	1.37 KB
#3251	`SendReceive`	netcoreapp3.1	1.82μs	1.04ns	3.9ns	0.0181	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	4.95μs	1.82ns	7.05ns	0.353	0.00247	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.31μs	2.17ns	7.83ns	0.0235	0	1.8 KB
#3251	`EnrichedLog`	net472	4.96μs	2.32ns	8.97ns	0.352	0	2.23 KB
#3251	`EnrichedLog`	netcoreapp3.1	4.25μs	4.36ns	16.3ns	0.0234	0	1.8 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.16μs	0.618ns	2.39ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	943ns	0.322ns	1.2ns	0.0104	760 B
master	`StartFinishScope`	net472	1.49μs	2.73ns	10.6ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.04μs	0.404ns	1.57ns	0.012	880 B
#3251	`StartFinishSpan`	net472	1.2μs	0.599ns	2.32ns	0.129	810 B
#3251	`StartFinishSpan`	netcoreapp3.1	958ns	0.368ns	1.43ns	0.0105	760 B
#3251	`StartFinishScope`	net472	1.48μs	1.31ns	5.09ns	0.141	891 B
#3251	`StartFinishScope`	netcoreapp3.1	1.05μs	0.527ns	2.04ns	0.0119	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.49μs	0.87ns	3.26ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	0.432ns	1.56ns	0.0118	880 B
#3251	`RunOnMethodBegin`	net472	1.49μs	0.626ns	2.42ns	0.141	891 B
#3251	`RunOnMethodBegin`	netcoreapp3.1	1.22μs	0.554ns	2.07ns	0.0122	880 B

andrewlock · 2022-09-23T12:46:02Z

Code Coverage Report 📊

✔️ Merging #3251 into master will not change line coverage
✔️ Merging #3251 into master will not change branch coverage
⛔ Merging #3251 into master will will increase complexity by 11

	master	#3251	Change
Lines	`18039` / `24950`	`18051` / `24964`
Lines %	`72%`	`72%`	`0%` ✔️
Branches	`10523` / `15302`	`10533` / `15312`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`16527`	`16538`	`11` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3251	Change
Lines %	`72%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`16527`	`16538`	`11` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Debugger.PInvoke.DebuggerNativeMethods	`6%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.Agent.Transports.HttpStreamRequestFactory	`12%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.Agent.AgentTransportStrategy	`22%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.Agent.StreamFactories.NamedPipeClientStreamFactory	`52%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.Http.HttpClient.WinHttpHandler.WinHttpHandlerIntegration	`100%` ✔️	`0%` ✔️	`0` ✔️

View the full reports for further details:

This comment has been minimized.

Sign in to view

anna-git force-pushed the anna/asm/fix-waf-retrocompatibility branch from d245f37 to 6e14a38 Compare September 22, 2022 12:03

This comment has been minimized.

Sign in to view

dromanol reviewed Sep 22, 2022

View reviewed changes

anna-git added 8 commits September 22, 2022 15:10

Gracefully shutdown waf if missing exports

4e60adc

add unit tests for a specific version

0f02ce2

fix indent

580740f

fix architecture for linux

1e833ad

fix indent

4ce032c

test more precise

f404bed

fix path

c016f9e

add hyphen

596ff97

anna-git force-pushed the anna/asm/fix-waf-retrocompatibility branch from b679958 to 596ff97 Compare September 22, 2022 13:10

This comment has been minimized.

Sign in to view

anna-git changed the title ~~Anna/asm/fix waf retrocompatibility~~ [ASM] Prevent waf from breaking app if non compatible native library file Sep 22, 2022

anna-git changed the title ~~[ASM] Prevent waf from breaking app if non compatible native library file~~ [ASM] Prevent waf from breaking app if non compatible native library file is loaded by tracer Sep 22, 2022

anna-git added the area:asm label Sep 22, 2022

anna-git marked this pull request as ready for review September 22, 2022 14:20

anna-git requested review from a team as code owners September 22, 2022 14:20

robertpi reviewed Sep 22, 2022

View reviewed changes

robertpi approved these changes Sep 22, 2022

View reviewed changes

andrewlock approved these changes Sep 22, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

anna-git added 2 commits September 23, 2022 11:12

better logs and startup loging if errors in libddwaf exports

9fe3b5d

fix indenting

3f3cb3e

This comment has been minimized.

Sign in to view

waf might not be init

ec72d0b

anna-git merged commit 8070df6 into master Sep 23, 2022

anna-git deleted the anna/asm/fix-waf-retrocompatibility branch September 23, 2022 12:49

github-actions Bot added this to the vNext milestone Sep 23, 2022

Uh oh!

Conversation

anna-git commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertpi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

andrewlock commented Sep 23, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Sep 23, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anna-git commented Sep 22, 2022 •

edited

Loading