Skip to content

Commit

Permalink
Revert "[Crashtracking] Fix the handling of COMPlus_DbgMiniDumpName (#…
Browse files Browse the repository at this point in the history 
…5980 -> v2) (#6001)"

This reverts commit 86e60bf.

We are suspecting that this is causing application crashes on .NET 6.0 with the Continuous Profiler enabled.
  • Loading branch information
@bouwkast
bouwkast committed Sep 17, 2024
1 parent 40a8c34 commit ca79e6c
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 83 deletions.
52 changes: 13 additions & 39 deletions profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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')
{
Expand All @@ -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);
}
}

Expand Down
8 changes: 2 additions & 6 deletions tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/ConsoleTestHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,12 @@ protected Task<ProcessHelper> StartConsoleWithArgs(string args, bool enableProfi
return (executable, args);
}

protected Task<ProcessHelper> StartConsole(EnvironmentHelper environmentHelper, bool enableProfiler, string args, params (string Key, string Value)[] environmentVariables)
protected async Task<ProcessHelper> 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<ProcessHelper> 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;
Expand Down
39 changes: 1 addition & 38 deletions tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit ca79e6c

Please sign in to comment.