From ca79e6ccef9031635f4ac9b8d592398cf2b7d53f Mon Sep 17 00:00:00 2001 From: Steven Bouwkamp Date: Tue, 17 Sep 2024 18:22:48 -0400 Subject: [PATCH] Revert "[Crashtracking] Fix the handling of COMPlus_DbgMiniDumpName (#5980 -> v2) (#6001)" This reverts commit 86e60bfc0a0aa05c320455cd104cf427b01e22dd. We are suspecting that this is causing application crashes on .NET 6.0 with the Continuous Profiler enabled. --- .../functions_to_wrap.c | 52 +++++-------------- .../ConsoleTestHelper.cs | 8 +-- .../CreatedumpTests.cs | 39 +------------- 3 files changed, 16 insertions(+), 83 deletions(-) diff --git a/profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c b/profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c index 10a924e61fe9..0aa4d0699bde 100644 --- a/profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c +++ b/profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c @@ -181,7 +181,6 @@ static const char* datadogCrashMarker = "datadog_crashtracking"; #define COMPlus_DbgEnableMiniDump "COMPlus_DbgEnableMiniDump" #define DOTNET_DbgMiniDumpName "DOTNET_DbgMiniDumpName" #define COMPlus_DbgMiniDumpName "COMPlus_DbgMiniDumpName" -#define DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME "DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME" __attribute__((constructor)) void initLibrary(void) @@ -199,16 +198,6 @@ void initLibrary(void) } } - // Bash provides its own version of the getenv/setenv functions - // Fetch the original ones and use those instead - char *(*real_getenv)(const char *) = (char *(*)(const char *))dlsym(RTLD_NEXT, "getenv"); - int (*real_setenv)(const char *, const char *, int) = (int (*)(const char *, const char *, int))dlsym(RTLD_NEXT, "setenv"); - - if (real_getenv == NULL || real_setenv == NULL) - { - return; - } - // If crashtracking is enabled, check the value of DOTNET_DbgEnableMiniDump // If set, set DD_TRACE_CRASH_HANDLER_PASSTHROUGH to indicate dd-dotnet that it should call createdump // If not set, set it to 1 so that .NET calls createdump in case of crash @@ -290,17 +279,17 @@ void initLibrary(void) if (crashHandler != NULL && crashHandler[0] != '\0') { - char* enableMiniDump = real_getenv(DOTNET_DbgEnableMiniDump); + char* enableMiniDump = getenv(DOTNET_DbgEnableMiniDump); if (enableMiniDump == NULL) { - enableMiniDump = real_getenv(COMPlus_DbgEnableMiniDump); + enableMiniDump = getenv(COMPlus_DbgEnableMiniDump); } if (enableMiniDump != NULL && enableMiniDump[0] == '1') { - // Passthrough is expected by dd-dotnet to know whether it should forward the call to createdump - char* passthrough = real_getenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH); + // If DOTNET_DbgEnableMiniDump is set, the crash handler should call createdump when done + char* passthrough = getenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH); if (passthrough == NULL || passthrough[0] == '\0') { @@ -309,40 +298,25 @@ void initLibrary(void) // - dotnet run sets DOTNET_DbgEnableMiniDump=1 // - dotnet then launches the target app // - the target app thinks DOTNET_DbgEnableMiniDump has been set by the user and enables passthrough - real_setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "1", 1); + setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "1", 1); } } else { // If DOTNET_DbgEnableMiniDump is not set, we set it so that the crash handler is called, // but we instruct it to not call createdump afterwards - real_setenv(COMPlus_DbgEnableMiniDump, "1", 1); - real_setenv(DOTNET_DbgEnableMiniDump, "1", 1); - real_setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "0", 1); - } - - originalMiniDumpName = real_getenv(DOTNET_DbgMiniDumpName); - - if (originalMiniDumpName == NULL || strncmp(originalMiniDumpName, datadogCrashMarker, strlen(datadogCrashMarker)) == 0) - { - originalMiniDumpName = real_getenv(COMPlus_DbgMiniDumpName); - } - - if (originalMiniDumpName != NULL && strncmp(originalMiniDumpName, datadogCrashMarker, strlen(datadogCrashMarker)) == 0) - { - // If LD_PRELOAD was set in the parent process, then we replaced COMPlus_DbgMiniDumpName with datadogCrashMarker and lost the original value - // We use DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME to retrieve it - originalMiniDumpName = real_getenv(DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME); + setenv(COMPlus_DbgEnableMiniDump, "1", 1); + setenv(DOTNET_DbgEnableMiniDump, "1", 1); + setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "0", 1); } - if (originalMiniDumpName != NULL && originalMiniDumpName[0] != '\0') + originalMiniDumpName = getenv(DOTNET_DbgMiniDumpName); + if (originalMiniDumpName == NULL) { - // Save the original value in DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME so that child processes can retrieve it - real_setenv(DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME, originalMiniDumpName, 1); + originalMiniDumpName = getenv(COMPlus_DbgMiniDumpName); } - - real_setenv(COMPlus_DbgMiniDumpName, datadogCrashMarker, 1); - real_setenv(DOTNET_DbgMiniDumpName, datadogCrashMarker, 1); + setenv(COMPlus_DbgMiniDumpName, datadogCrashMarker, 1); + setenv(DOTNET_DbgMiniDumpName, datadogCrashMarker, 1); } } diff --git a/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/ConsoleTestHelper.cs b/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/ConsoleTestHelper.cs index 2127323fa1bc..db4deb864f20 100644 --- a/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/ConsoleTestHelper.cs +++ b/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/ConsoleTestHelper.cs @@ -49,16 +49,12 @@ protected Task StartConsoleWithArgs(string args, bool enableProfi return (executable, args); } - protected Task StartConsole(EnvironmentHelper environmentHelper, bool enableProfiler, string args, params (string Key, string Value)[] environmentVariables) + protected async Task StartConsole(EnvironmentHelper environmentHelper, bool enableProfiler, string args, params (string Key, string Value)[] environmentVariables) { var (executable, baseArgs) = PrepareSampleApp(environmentHelper); - args = $"{baseArgs} {args}"; - return StartConsole(executable, args, environmentHelper, enableProfiler, environmentVariables); - } + args = $"{baseArgs} {args}"; - protected async Task StartConsole(string executable, string args, EnvironmentHelper environmentHelper, bool enableProfiler, params (string Key, string Value)[] environmentVariables) - { var processStart = new ProcessStartInfo(executable, args) { UseShellExecute = false, RedirectStandardError = true, RedirectStandardOutput = true }; MockTracerAgent? agent = null; diff --git a/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs b/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs index 666edc57eb23..c4b2b118a2bf 100644 --- a/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs +++ b/tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs @@ -24,7 +24,7 @@ namespace Datadog.Trace.Tools.dd_dotnet.ArtifactTests; public class CreatedumpTests : ConsoleTestHelper { - private const string CreatedumpExpectedOutput = "Writing minidump with heap to file /dev/null"; + private const string CreatedumpExpectedOutput = "Writing minidump"; private const string CrashReportExpectedOutput = "The crash may have been caused by automatic instrumentation"; public CreatedumpTests(ITestOutputHelper output) @@ -94,41 +94,6 @@ public async Task Passthrough(string passthrough, bool shouldCallCreatedump) } } - [SkippableFact] - public async Task BashScript() - { - // This tests the case when an app is called through a bash script - // This scenario has unique challenges because: - // - The COMPlus_DbgMiniDumpName environment variable that we override is then inherited by the child - // - Bash overrides the getenv/setenv functions, which cause some unexpected behaviors - - SkipOn.Platform(SkipOn.PlatformValue.Windows); - SkipOn.Platform(SkipOn.PlatformValue.MacOs); - - using var reportFile = new TemporaryFile(); - - (string, string)[] environment = [LdPreloadConfig, .. CreatedumpConfig, CrashReportConfig(reportFile)]; - - var (executable, args) = PrepareSampleApp(EnvironmentHelper); - - var bashScript = $"#!/bin/bash\n{executable} {args} crash-datadog\n"; - using var bashFile = new TemporaryFile(); - bashFile.SetContent(bashScript); - - using var helper = await StartConsole("/bin/bash", bashFile.Path, EnvironmentHelper, false, environment); - - await helper.Task; - - using var assertionScope = new AssertionScope(); - assertionScope.AddReportable("stdout", helper.StandardOutput); - assertionScope.AddReportable("stderr", helper.ErrorOutput); - - helper.StandardOutput.Should().Contain(CrashReportExpectedOutput); - File.Exists(reportFile.Path).Should().BeTrue(); - - helper.StandardOutput.Should().Contain(CreatedumpExpectedOutput); - } - [SkippableTheory] [InlineData(true)] [InlineData(false)] @@ -518,8 +483,6 @@ public TemporaryFile() public string GetContent() => File.ReadAllText(Path); - public void SetContent(string content) => File.WriteAllText(Path, content); - public void Dispose() { File.Delete(Path);