Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Crashtracking] Fix the handling of COMPlus_DbgMiniDumpName #5980

Merged
merged 7 commits into from
Sep 6, 2024
Merged

[Crashtracking] Fix the handling of COMPlus_DbgMiniDumpName #5980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f637fd1
Save the value of COMPlus_DbgMiniDumpName for child processes
kevingosse Sep 3, 2024
0d5f190
Fix
kevingosse Sep 4, 2024
312ffc5
Saner solution
kevingosse Sep 4, 2024
a9999f8
Add test
kevingosse Sep 4, 2024
ebb1796
Update CreatedumpTests.cs
kevingosse Sep 4, 2024
7626b8c
Update CreatedumpTests.cs
kevingosse Sep 4, 2024
2088be8
Update CreatedumpTests.cs
kevingosse Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 39 additions & 13 deletions profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ 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 @@ -206,6 +207,16 @@ 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 @@ -287,17 +298,17 @@ void initLibrary(void)

if (crashHandler != NULL && crashHandler[0] != '\0')
{
char* enableMiniDump = getenv(DOTNET_DbgEnableMiniDump);
char* enableMiniDump = real_getenv(DOTNET_DbgEnableMiniDump);

if (enableMiniDump == NULL)
{
enableMiniDump = getenv(COMPlus_DbgEnableMiniDump);
enableMiniDump = real_getenv(COMPlus_DbgEnableMiniDump);
}

if (enableMiniDump != NULL && enableMiniDump[0] == '1')
{
// If DOTNET_DbgEnableMiniDump is set, the crash handler should call createdump when done
char* passthrough = getenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH);
// 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 (passthrough == NULL || passthrough[0] == '\0')
{
Expand All @@ -306,25 +317,40 @@ 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
setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "1", 1);
real_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
setenv(COMPlus_DbgEnableMiniDump, "1", 1);
setenv(DOTNET_DbgEnableMiniDump, "1", 1);
setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "0", 1);
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);
}

originalMiniDumpName = getenv(DOTNET_DbgMiniDumpName);
if (originalMiniDumpName == NULL)
if (originalMiniDumpName != NULL && originalMiniDumpName[0] != '\0')
{
originalMiniDumpName = getenv(COMPlus_DbgMiniDumpName);
// Save the original value in DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME so that child processes can retrieve it
real_setenv(DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME, originalMiniDumpName, 1);
}
setenv(COMPlus_DbgMiniDumpName, datadogCrashMarker, 1);
setenv(DOTNET_DbgMiniDumpName, datadogCrashMarker, 1);

real_setenv(COMPlus_DbgMiniDumpName, datadogCrashMarker, 1);
real_setenv(DOTNET_DbgMiniDumpName, datadogCrashMarker, 1);
Comment on lines +349 to +353
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a silly question, but is there a reason we have to clobber and restore the original values? Why can't we just store our marker in the DD_INTERNAL_ env instead?

}
}

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

protected async Task<ProcessHelper> StartConsole(EnvironmentHelper environmentHelper, bool enableProfiler, string args, params (string Key, string Value)[] environmentVariables)
protected 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);
}

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: 38 additions & 1 deletion tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs
View file
Open in desktop
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";
private const string CreatedumpExpectedOutput = "Writing minidump with heap to file /dev/null";
private const string CrashReportExpectedOutput = "The crash may have been caused by automatic instrumentation";

public CreatedumpTests(ITestOutputHelper output)
Expand Down Expand Up @@ -94,6 +94,41 @@ 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 @@ -483,6 +518,8 @@ 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
Loading