Skip to content

Do not use java.nio in crashtracking init (premain) #11080

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

Open
amarziali wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not use java.nio in crashtracking init (premain) #11080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
210b32e
Do not use java.nio in crashtracking init (premain)
amarziali Apr 10, 2026
aba8a0e
techdebt
amarziali Apr 10, 2026
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
54 changes: 27 additions & 27 deletions dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/ConfigManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@
import datadog.trace.util.RandomUtils;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.nio.charset.StandardCharsets;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -157,8 +161,8 @@ public StoredConfig build() {

private ConfigManager() {}

private static String getBaseName(Path path) {
String filename = path.getFileName().toString();
private static String getBaseName(File file) {
String filename = file.getName();
int dotIndex = filename.lastIndexOf('.');
if (dotIndex == -1) {
return filename;
Expand All @@ -185,18 +189,20 @@ private static void writeEntry(BufferedWriter writer, CharSequence key, CharSequ
writer.newLine();
}

public static void writeConfigToPath(Path scriptPath, String... additionalEntries) {
String cfgFileName = getBaseName(scriptPath) + PID_PREFIX + PidHelper.getPid() + ".cfg";
Path cfgPath = scriptPath.resolveSibling(cfgFileName);
writeConfigToFile(Config.get(), cfgPath, additionalEntries);
public static void writeConfigToPath(File scriptFile, String... additionalEntries) {
String cfgFileName = getBaseName(scriptFile) + PID_PREFIX + PidHelper.getPid() + ".cfg";
File cfgFile = new File(scriptFile.getParentFile(), cfgFileName);
writeConfigToFile(Config.get(), cfgFile, additionalEntries);
}

// @VisibleForTesting
static void writeConfigToFile(Config config, Path cfgPath, String... additionalEntries) {
static void writeConfigToFile(Config config, File cfgFile, String... additionalEntries) {
final WellKnownTags wellKnownTags = config.getWellKnownTags();

LOGGER.debug("Writing config file: {}", cfgPath);
try (BufferedWriter bw = Files.newBufferedWriter(cfgPath)) {
LOGGER.debug("Writing config file: {}", cfgFile);
try (BufferedWriter bw =
new BufferedWriter(
new OutputStreamWriter(new FileOutputStream(cfgFile), StandardCharsets.UTF_8))) {
for (int i = 0; i < additionalEntries.length; i += 2) {
writeEntry(bw, additionalEntries[i], additionalEntries[i + 1]);
}
Expand All @@ -217,27 +223,21 @@ static void writeConfigToFile(Config config, Path cfgPath, String... additionalE
new Thread(
AGENT_THREAD_GROUP,
() -> {
try {
LOGGER.debug("Deleting config file: {}", cfgPath);
Files.deleteIfExists(cfgPath);
} catch (IOException e) {
LOGGER.warn(SEND_TELEMETRY, "Failed deleting config file: {}", cfgPath, e);
}
LOGGER.debug("Deleting config file: {}", cfgFile);
cfgFile.delete();
Copy link
Copy Markdown
Contributor

@jbachorik jbachorik Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Silent failure on config file deletion

The original Files.deleteIfExists() call logged a WARN with SEND_TELEMETRY on failure. The replacement cfgFile.delete() returns false on failure but the return value is unchecked, silently dropping that telemetry signal.

Suggested change
cfgFile.delete();
LOGGER.debug("Deleting config file: {}", cfgFile);
if (!cfgFile.delete() && cfgFile.exists()) {
LOGGER.warn(SEND_TELEMETRY, "Failed deleting config file: {}", cfgFile);
}

}));
LOGGER.debug("Config file written: {}", cfgPath);
LOGGER.debug("Config file written: {}", cfgFile);
} catch (IOException e) {
LOGGER.warn(SEND_TELEMETRY, "Failed writing config file: {}", cfgPath);
try {
Files.deleteIfExists(cfgPath);
} catch (IOException ignored) {
// ignore
}
LOGGER.warn(SEND_TELEMETRY, "Failed writing config file: {}", cfgFile);
cfgFile.delete();
Copy link
Copy Markdown
Contributor

@jbachorik jbachorik Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Undocumented best-effort delete

The original code had an explicit catch (IOException ignored) { } that documented the intent to swallow the error. The replacement is also silent, but without that comment future readers may assume this is an oversight.

Suggested change
cfgFile.delete();
cfgFile.delete(); // best-effort cleanup; failure is acceptable here

}
}

@Nullable
public static StoredConfig readConfig(Config config, Path scriptPath) {
try (final BufferedReader reader = Files.newBufferedReader(scriptPath)) {
public static StoredConfig readConfig(Config config, File scriptFile) {
try (final BufferedReader reader =
new BufferedReader(
new InputStreamReader(new FileInputStream(scriptFile), StandardCharsets.UTF_8))) {
final StoredConfig.Builder cfgBuilder = new StoredConfig.Builder(config);
String line;
while ((line = reader.readLine()) != null) {
Expand Down Expand Up @@ -284,7 +284,7 @@ public static StoredConfig readConfig(Config config, Path scriptPath) {
}
return cfgBuilder.build();
} catch (Throwable t) {
LOGGER.error("Failed to read config file: {}", scriptPath, t);
LOGGER.error("Failed to read config file: {}", scriptFile, t);
}
return null;
}
Expand Down
73 changes: 34 additions & 39 deletions ...ent-crashtracking/src/main/java/datadog/crashtracking/CrashUploaderScriptInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,23 @@

import static datadog.crashtracking.ConfigManager.writeConfigToPath;
import static datadog.crashtracking.Initializer.LOG;
import static datadog.crashtracking.Initializer.RWXRWXRWX;
import static datadog.crashtracking.Initializer.R_XR_XR_X;
import static datadog.crashtracking.Initializer.findAgentJar;
import static datadog.crashtracking.Initializer.getCrashUploaderTemplate;
import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
import static java.nio.file.attribute.PosixFilePermissions.fromString;
import static java.util.Locale.ROOT;

import datadog.environment.SystemProperties;
import datadog.trace.util.PidHelper;
import datadog.trace.util.Strings;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.io.OutputStreamWriter;
import java.nio.charset.StandardCharsets;

public final class CrashUploaderScriptInitializer {
private static final String SETUP_FAILURE_MESSAGE = "Crash tracking will not work properly.";
Expand Down Expand Up @@ -54,71 +50,70 @@ static void initialize(String onErrorVal, String onErrorFile, String javacorePat
return;
}

Path scriptPath = Paths.get(onErrorVal.replace(" %p", ""));
File scriptFile = new File(onErrorVal.replace(" %p", ""));
boolean isDDCrashUploader =
scriptPath.getFileName().toString().toLowerCase(ROOT).contains("dd_crash_uploader");
if (isDDCrashUploader && !copyCrashUploaderScript(scriptPath, onErrorFile, agentJar)) {
scriptFile.getName().toLowerCase(ROOT).contains("dd_crash_uploader");
if (isDDCrashUploader && !copyCrashUploaderScript(scriptFile, onErrorFile, agentJar)) {
return;
}

if (javacorePath != null && !javacorePath.isEmpty()) {
writeConfigToPath(scriptPath, "agent", agentJar, "javacore_path", javacorePath);
writeConfigToPath(scriptFile, "agent", agentJar, "javacore_path", javacorePath);
} else {
writeConfigToPath(scriptPath, "agent", agentJar, "hs_err", onErrorFile);
writeConfigToPath(scriptFile, "agent", agentJar, "hs_err", onErrorFile);
}
}

private static boolean copyCrashUploaderScript(
Path scriptPath, String onErrorFile, String agentJar) {
Path scriptDirectory = scriptPath.getParent();
try {
Files.createDirectories(scriptDirectory, asFileAttribute(fromString(RWXRWXRWX)));
} catch (UnsupportedOperationException e) {
LOG.warn(
SEND_TELEMETRY,
"Unsupported permissions '" + RWXRWXRWX + "' for {}. " + SETUP_FAILURE_MESSAGE,
scriptDirectory);
return false;
} catch (FileAlreadyExistsException ignored) {
// can be safely ignored; if the folder exists we will just reuse it
if (!Files.isWritable(scriptDirectory)) {
File scriptFile, String onErrorFile, String agentJar) {
File scriptDirectory = scriptFile.getParentFile();
if (!scriptDirectory.exists()) {
if (!scriptDirectory.mkdirs()) {
Copy link
Copy Markdown
Contributor

@jbachorik jbachorik Apr 10, 2026

Choose a reason for hiding this comment

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

Non-atomic directory creation and permission setting

The original Files.createDirectories() with PosixFilePermissions set permissions atomically on creation. The replacement mkdirs() followed by separate setReadable/setWritable/setExecutable calls introduces a race window between directory creation and permission application.

This is init-time code so the practical impact is minimal, but worth documenting the accepted trade-off with a comment.

LOG.warn(
SEND_TELEMETRY, "Read only directory {}. " + SETUP_FAILURE_MESSAGE, scriptDirectory);
SEND_TELEMETRY,
"Failed to create writable crash tracking script folder {}. " + SETUP_FAILURE_MESSAGE,
scriptDirectory);
return false;
}
} catch (IOException e) {
LOG.warn(
SEND_TELEMETRY,
"Failed to create writable crash tracking script folder {}. " + SETUP_FAILURE_MESSAGE,
scriptDirectory);
scriptDirectory.setReadable(true, false);
scriptDirectory.setWritable(true, false);
scriptDirectory.setExecutable(true, false);
Copy link
Copy Markdown
Contributor

@jbachorik jbachorik Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Return values of permission calls are ignored

setReadable(), setWritable(), and setExecutable() all return boolean indicating success, but the return values are not checked here (or in the analogous block in OOMENotifierScriptInitializer). A silent permission failure can cause hard-to-diagnose downstream crashes.

Suggested change
scriptDirectory.setExecutable(true, false);
if (!scriptDirectory.setReadable(true, false)
|| !scriptDirectory.setWritable(true, false)
|| !scriptDirectory.setExecutable(true, false)) {
LOG.warn(
SEND_TELEMETRY,
"Failed to set permissions on crash tracking script folder {}. {}",
scriptDirectory, SETUP_FAILURE_MESSAGE);
}

}
if (!scriptDirectory.canWrite()) {
LOG.warn(SEND_TELEMETRY, "Read only directory {}. " + SETUP_FAILURE_MESSAGE, scriptDirectory);
return false;
}
try {
LOG.debug("Writing crash uploader script: {}", scriptPath);
writeCrashUploaderScript(getCrashUploaderTemplate(), scriptPath, agentJar, onErrorFile);
LOG.debug("Writing crash uploader script: {}", scriptFile);
writeCrashUploaderScript(getCrashUploaderTemplate(), scriptFile, agentJar, onErrorFile);
} catch (IOException e) {
LOG.warn(
SEND_TELEMETRY,
"Failed to copy crash tracking script {}. " + SETUP_FAILURE_MESSAGE,
scriptPath);
scriptFile);
return false;
}
return true;
}

private static void writeCrashUploaderScript(
InputStream template, Path scriptPath, String execClass, String crashFile)
InputStream template, File scriptFile, String execClass, String crashFile)
throws IOException {
if (!Files.exists(scriptPath)) {
if (!scriptFile.exists()) {
try (BufferedReader br = new BufferedReader(new InputStreamReader(template));
BufferedWriter bw = Files.newBufferedWriter(scriptPath)) {
BufferedWriter bw =
new BufferedWriter(
new OutputStreamWriter(
new FileOutputStream(scriptFile), StandardCharsets.UTF_8))) {
String line;
while ((line = br.readLine()) != null) {
bw.write(template(line, execClass, crashFile));
bw.newLine();
}
}
Files.setPosixFilePermissions(scriptPath, fromString(R_XR_XR_X));
scriptFile.setReadable(true, false);
scriptFile.setWritable(false, false);
scriptFile.setExecutable(true, false);
}
}

Expand Down
33 changes: 9 additions & 24 deletions dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package datadog.crashtracking;

import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
import static java.util.Comparator.reverseOrder;
import static java.util.Locale.ROOT;

import com.datadoghq.profiler.JVMAccess;
Expand All @@ -12,25 +11,19 @@
import datadog.libs.ddprof.DdprofLibraryLoader;
import datadog.trace.api.Platform;
import datadog.trace.util.TempLocationManager;
import java.io.IOException;
import java.io.File;
import java.io.InputStream;
import java.lang.management.ManagementFactory;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;
import java.util.StringTokenizer;
import java.util.function.Predicate;
import java.util.stream.Stream;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public final class Initializer {
static final Logger LOG = LoggerFactory.getLogger(Initializer.class);
static final String PID_PREFIX = "_pid";
static final String RWXRWXRWX = "rwxrwxrwx";
static final String R_XR_XR_X = "r-xr-xr-x";

private interface FlagAccess {
String getValue(String flagName);
Expand Down Expand Up @@ -251,8 +244,8 @@ private static boolean isXdumpToolConfigured() {
*/
private static String getJ9CrashUploaderScriptPath() {
String scriptFileName = getScriptFileName("dd_crash_uploader");
Path scriptPath = TempLocationManager.getInstance().getTempDir().resolve(scriptFileName);
return scriptPath.toString();
String tempDir = TempLocationManager.getInstance().getTempDir().toString();
return tempDir + File.separator + scriptFileName;
}

static InputStream getCrashUploaderTemplate() {
Expand Down Expand Up @@ -280,19 +273,11 @@ static String findAgentJar() {
else if (selfClass.startsWith("file:")) {
int idx = selfClass.lastIndexOf("dd-java-agent");
if (idx > -1) {
Path libsPath = Paths.get(selfClass.substring(5, idx + 13), "build", "libs");
try (Stream<Path> files = Files.walk(libsPath)) {
Predicate<Path> isJarFile =
p -> p.getFileName().toString().toLowerCase(ROOT).endsWith(".jar");
agentPath =
files
.sorted(reverseOrder())
.filter(isJarFile)
.findFirst()
.map(Path::toString)
.orElse(null);
} catch (IOException ignored) {
// Ignore failure to get agent path
File libsDir = new File(selfClass.substring(5, idx + 13), "build/libs");
File[] jars = libsDir.listFiles(f -> f.getName().toLowerCase(ROOT).endsWith(".jar"));
if (jars != null && jars.length > 0) {
Arrays.sort(jars, (a, b) -> b.getName().compareTo(a.getName()));
agentPath = jars[0].getAbsolutePath();
}
}
}
Expand Down
Loading
Loading