From 64fcc6b060681d702ad1538a28cf70042c224e42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Mon, 22 Dec 2025 17:33:07 +0100 Subject: [PATCH 1/2] fix: Enforce UTF-8 for AppMap I/O and add encoding regression tests This commit ensures consistent UTF-8 character encoding for AppMap data across file system operations and HTTP responses, preventing corruption on systems where the default charset is not UTF-8 (e.g., Windows-1252). Key changes: - `RecordingSession.java`: Enforce `StandardCharsets.UTF_8` when creating writers for AppMap files, replacing reliance on default `FileWriter`. - `Recording.java`: Explicitly use `StandardCharsets.UTF_8` in `readFully` to correctly decode AppMap content during retrieval. - `ServletRequest.java`: Set `Content-Type: application/json; charset=UTF-8` for remote recording responses and calculate `Content-Length` based on UTF-8 byte size rather than string length. - `agent/test/encoding/`: Add a comprehensive regression test suite (`encoding.bats`, `UnicodeTest.java`, `ReadFullyTest.java`) to verify encoding handling for both reading and writing operations under non-UTF-8 environment settings. --- .../hooks/remoterecording/ServletRequest.java | 7 ++- .../com/appland/appmap/record/Recording.java | 5 +- .../appmap/record/RecordingSession.java | 7 ++- agent/test/encoding/ReadFullyTest.java | 47 ++++++++++++++ agent/test/encoding/UnicodeTest.java | 41 ++++++++++++ agent/test/encoding/appmap.yml | 3 + agent/test/encoding/encoding.bats | 63 +++++++++++++++++++ agent/test/encoding/encoding_test.cp1252 | 5 ++ 8 files changed, 170 insertions(+), 8 deletions(-) create mode 100644 agent/test/encoding/ReadFullyTest.java create mode 100644 agent/test/encoding/UnicodeTest.java create mode 100644 agent/test/encoding/appmap.yml create mode 100755 agent/test/encoding/encoding.bats create mode 100644 agent/test/encoding/encoding_test.cp1252 diff --git a/agent/src/main/java/com/appland/appmap/process/hooks/remoterecording/ServletRequest.java b/agent/src/main/java/com/appland/appmap/process/hooks/remoterecording/ServletRequest.java index 25bdd7ec..a058fdc3 100644 --- a/agent/src/main/java/com/appland/appmap/process/hooks/remoterecording/ServletRequest.java +++ b/agent/src/main/java/com/appland/appmap/process/hooks/remoterecording/ServletRequest.java @@ -2,6 +2,7 @@ import java.io.IOException; import java.io.PrintWriter; +import java.nio.charset.StandardCharsets; import com.appland.appmap.record.Recording; import com.appland.appmap.reflect.HttpServletRequest; @@ -29,8 +30,8 @@ public void setStatus(int status) { } public void writeJson(String responseJson) throws IOException { - res.setContentType("application/json"); - res.setContentLength(responseJson.length()); + res.setContentType("application/json; charset=UTF-8"); + res.setContentLength(responseJson.getBytes(StandardCharsets.UTF_8).length); res.setStatus(HttpServletResponse.SC_OK); PrintWriter writer = res.getWriter(); @@ -39,7 +40,7 @@ public void writeJson(String responseJson) throws IOException { } public void writeRecording(Recording recording) throws IOException { - res.setContentType("application/json"); + res.setContentType("application/json; charset=UTF-8"); res.setContentLength(recording.size()); recording.readFully(true, res.getWriter()); } diff --git a/agent/src/main/java/com/appland/appmap/record/Recording.java b/agent/src/main/java/com/appland/appmap/record/Recording.java index 649b37d2..b66daacb 100644 --- a/agent/src/main/java/com/appland/appmap/record/Recording.java +++ b/agent/src/main/java/com/appland/appmap/record/Recording.java @@ -5,11 +5,12 @@ import java.io.File; import java.io.FileInputStream; -import java.io.FileReader; import java.io.IOException; import java.io.InputStream; +import java.io.InputStreamReader; import java.io.Reader; import java.io.Writer; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -100,7 +101,7 @@ public Path moveTo(Path targetPath) { } public void readFully(boolean delete, Writer writer) throws IOException { - try (final Reader reader = new FileReader(this.file)) { + try (final Reader reader = new InputStreamReader(new FileInputStream(this.file), StandardCharsets.UTF_8)) { char[] buffer = new char[2048]; int bytesRead; while ((bytesRead = reader.read(buffer)) != -1) { diff --git a/agent/src/main/java/com/appland/appmap/record/RecordingSession.java b/agent/src/main/java/com/appland/appmap/record/RecordingSession.java index 583177fd..ec443f04 100644 --- a/agent/src/main/java/com/appland/appmap/record/RecordingSession.java +++ b/agent/src/main/java/com/appland/appmap/record/RecordingSession.java @@ -1,12 +1,13 @@ package com.appland.appmap.record; import java.io.File; -import java.io.FileWriter; +import java.io.FileOutputStream; import java.io.IOException; import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.RandomAccessFile; import java.io.Writer; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; @@ -98,7 +99,7 @@ public synchronized Recording checkpoint() { public void write(int b) throws IOException { raf.write(b); } - }); + }, StandardCharsets.UTF_8); raf.seek(targetPath.toFile().length()); if ( eventReceived ) { @@ -162,7 +163,7 @@ void start() { try { this.tmpPath = Files.createTempFile(null, ".appmap.json"); this.tmpPath.toFile().deleteOnExit(); - this.serializer = AppMapSerializer.open(new FileWriter(this.tmpPath.toFile())); + this.serializer = AppMapSerializer.open(new OutputStreamWriter(new FileOutputStream(this.tmpPath.toFile()), StandardCharsets.UTF_8)); } catch (IOException e) { this.tmpPath = null; this.serializer = null; diff --git a/agent/test/encoding/ReadFullyTest.java b/agent/test/encoding/ReadFullyTest.java new file mode 100644 index 00000000..68b20c55 --- /dev/null +++ b/agent/test/encoding/ReadFullyTest.java @@ -0,0 +1,47 @@ +package test.pkg; + +import com.appland.appmap.config.AppMapConfig; +import com.appland.appmap.record.Recording; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystems; + +public class ReadFullyTest { + public static void main(String[] args) { + try { + runTest(); + } catch (Exception e) { + e.printStackTrace(); + System.exit(1); + } + } + + public static void runTest() throws IOException { + // Initialize AppMapConfig + AppMapConfig.initialize(FileSystems.getDefault()); + + // 1. Create a dummy AppMap file with known UTF-8 content + String content = "Check: \u26A0\uFE0F \u041F\u0440\u0438\u0432\u0435\u0442"; + File tempFile = File.createTempFile("readfully", ".appmap.json"); + tempFile.deleteOnExit(); + + try (Writer fw = new OutputStreamWriter(new FileOutputStream(tempFile), StandardCharsets.UTF_8)) { + fw.write(content); + } + + // 2. Create a Recording object pointing to it + Recording recording = new Recording("test", tempFile); + + // 3. Call readFully and write to stdout using UTF-8 + // This validates that readFully correctly reads the UTF-8 file bytes into characters + // regardless of the system's default encoding (which we will set to something else in BATS). + Writer stdoutWriter = new OutputStreamWriter(System.out, StandardCharsets.UTF_8); + recording.readFully(false, stdoutWriter); + stdoutWriter.flush(); + } +} diff --git a/agent/test/encoding/UnicodeTest.java b/agent/test/encoding/UnicodeTest.java new file mode 100644 index 00000000..2f30dc8f --- /dev/null +++ b/agent/test/encoding/UnicodeTest.java @@ -0,0 +1,41 @@ +package test.pkg; + +import java.nio.file.Files; +import java.nio.file.Paths; +import java.io.IOException; + +public class UnicodeTest { + public static String echo(String input) { + return input; + } + + public static byte[] echoBytes(byte[] input) { + return input.clone(); + } + + public static void main(String[] args) { + try { + runTest(); + } catch (IOException e) { + e.printStackTrace(); + // exit 1 + System.exit(1); + } + } + + public static void runTest() throws IOException { + byte[] allBytes = Files.readAllBytes(Paths.get("encoding_test.cp1252")); + + String allString = new String(allBytes, "Cp1252"); + String echoedString = echo(allString); + + // print out the echoed string + System.out.println(echoedString); + + byte[] echoedBytes = echoBytes(allBytes); + // print out the echoed bytes as hex + for (byte b : echoedBytes) { + System.out.printf("%02X ", b); + } + } +} diff --git a/agent/test/encoding/appmap.yml b/agent/test/encoding/appmap.yml new file mode 100644 index 00000000..1b191a20 --- /dev/null +++ b/agent/test/encoding/appmap.yml @@ -0,0 +1,3 @@ +name: encoding +packages: +- path: test.pkg diff --git a/agent/test/encoding/encoding.bats b/agent/test/encoding/encoding.bats new file mode 100755 index 00000000..44e913b8 --- /dev/null +++ b/agent/test/encoding/encoding.bats @@ -0,0 +1,63 @@ +#!/usr/bin/env bats +# shellcheck disable=SC2164 + +load '../helper' + +sep="$JAVA_PATH_SEPARATOR" +AGENT_JAR="$(find_agent_jar)" +java_cmd="java -cp ${BATS_TEST_DIRNAME}/build -javaagent:'${AGENT_JAR}'" + +setup() { + cd "${BATS_TEST_DIRNAME}" + + mkdir -p build + # Compile tests. Output to build so package structure 'test/pkg' works. + javac -d ./build UnicodeTest.java + + # Require the agent jar on the classpath to find the Recording class. + javac -cp "${AGENT_JAR}" -d ./build ReadFullyTest.java + + rm -rf "${BATS_TEST_DIRNAME}/tmp/appmap" + _configure_logging +} + +@test "AppMap file encoding with Windows-1252" { + # Run with windows-1252 encoding. + # We assert that the generated file is valid UTF-8 and contains the correct characters, + # even though the JVM default encoding is Windows-1252. + local cmd="${java_cmd} -Dfile.encoding=windows-1252 -Dappmap.recording.auto=true test.pkg.UnicodeTest" + [[ $BATS_VERBOSE_RUN == 1 ]] && echo "cmd: $cmd" >&3 + + eval "$cmd" + + # Verify the output file exists — it should be the only AppMap file generated, with random name + # so glob for tmp/appmap/java/*.appmap.json + appmap_file=$(ls tmp/appmap/java/*.appmap.json) + [ -f "$appmap_file" ] + + # Verify it is valid JSON + jq . "$appmap_file" > /dev/null + + # Verify it is valid UTF-8 + iconv -f UTF-8 -t UTF-8 "$appmap_file" > /dev/null + + # Verify it contains the expected Unicode characters + grep -q "Euro: €, Accent: é, Quote: „" "$appmap_file" +} + +@test "Recording.readFully works with Windows-1252 default encoding" { + # Run ReadFullyTest with windows-1252 default encoding. + # We also need to add the agent jar to the classpath so it can find the Recording class. + local cmd="java -cp ${BATS_TEST_DIRNAME}/build${sep}${AGENT_JAR} -Dfile.encoding=windows-1252 test.pkg.ReadFullyTest" + [[ $BATS_VERBOSE_RUN == 1 ]] && echo "cmd: $cmd" >&3 + + run eval "$cmd" + + [ "$status" -eq 0 ] + [[ "$output" == *"Check: ⚠️ Привет"* ]] +} + +teardown() { + rm -rf tmp + rm -rf build +} diff --git a/agent/test/encoding/encoding_test.cp1252 b/agent/test/encoding/encoding_test.cp1252 new file mode 100644 index 00000000..10b7f7d6 --- /dev/null +++ b/agent/test/encoding/encoding_test.cp1252 @@ -0,0 +1,5 @@ +Euro: , Accent: , Quote: +---SEPARATOR--- +:7ǞI +---BINARY--- +ޭ \ No newline at end of file From 6f5c34e0e883face6f41e607762edae1b0a323a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Tue, 13 Jan 2026 14:44:54 +0100 Subject: [PATCH 2/2] fix: Close underlying Writers in RecordingSession to prevent resource leaks The fastjson JSONWriter.close() method does not close the underlying Writer passed to its constructor - it only closes the internal SerializeWriter, which has no effect. This caused FileOutputStream and OutputStreamWriter instances to leak in RecordingSession. Changes: - Make AppMapSerializer implement AutoCloseable and track the underlying Writer to close it explicitly - Add try-with-resources for AppMapSerializer in checkpoint() to ensure proper cleanup on exceptions - Add try-finally in stop() to ensure serializer is closed even if write() fails - Consolidate JSON finalization and closing logic into close() method, removing the redundant finish() method Co-Authored-By: Claude Sonnet 4.5 --- .../appmap/record/AppMapSerializer.java | 27 ++++++++--- .../appmap/record/RecordingSession.java | 48 ++++++++++++------- 2 files changed, 50 insertions(+), 25 deletions(-) diff --git a/agent/src/main/java/com/appland/appmap/record/AppMapSerializer.java b/agent/src/main/java/com/appland/appmap/record/AppMapSerializer.java index 62f08ecb..7d7959d8 100644 --- a/agent/src/main/java/com/appland/appmap/record/AppMapSerializer.java +++ b/agent/src/main/java/com/appland/appmap/record/AppMapSerializer.java @@ -17,7 +17,7 @@ /** * Writes AppMap data to JSON. */ -public class AppMapSerializer { +public class AppMapSerializer implements AutoCloseable { public static class FileSections { public static final String Version = "version"; public static final String Metadata = "metadata"; @@ -37,10 +37,13 @@ private class SectionInfo { } private final JSONWriter json; + private final Writer underlyingWriter; private SectionInfo currentSection = null; private final HashSet sectionsWritten = new HashSet(); + private boolean closed = false; private AppMapSerializer(Writer writer) { + this.underlyingWriter = writer; this.json = new JSONWriter(writer); // The eventUpdates object contains Event objects that are also in the // events array. Setting DisableCircularReferenceDetect tells fastjson that @@ -73,7 +76,7 @@ public void write(CodeObjectTree classMap, Metadata metadata, Map updates) throws IOException { } /** - * Closes outstanding JSON objects and closes the writer. + * Closes outstanding JSON objects and closes the underlying writer. Safe to call multiple times. * @throws IOException If a writer error occurs */ - private void finish() throws IOException { - this.setCurrentSection("EOF", ""); - this.json.endObject(); - this.json.close(); + @Override + public void close() throws IOException { + if (!this.closed) { + try { + this.setCurrentSection("EOF", ""); + this.json.endObject(); + this.json.close(); + } finally { + // JSONWriter.close() does not close the underlying writer, so we must do it explicitly + // Always close the underlying writer, even if JSON finalization fails + this.underlyingWriter.close(); + this.closed = true; + } + } } } diff --git a/agent/src/main/java/com/appland/appmap/record/RecordingSession.java b/agent/src/main/java/com/appland/appmap/record/RecordingSession.java index ec443f04..d8baef67 100644 --- a/agent/src/main/java/com/appland/appmap/record/RecordingSession.java +++ b/agent/src/main/java/com/appland/appmap/record/RecordingSession.java @@ -93,22 +93,24 @@ public synchronized Recording checkpoint() { // By using RandomAccessFile we can erase that character. // If we don't let the JSON writer write the "begin object" token, it refuses // to do anything else properly either. - RandomAccessFile raf = new RandomAccessFile(targetPath.toFile(), "rw"); - Writer fw = new OutputStreamWriter(new OutputStream() { - @Override - public void write(int b) throws IOException { - raf.write(b); + try (RandomAccessFile raf = new RandomAccessFile(targetPath.toFile(), "rw")) { + Writer fw = new OutputStreamWriter(new OutputStream() { + @Override + public void write(int b) throws IOException { + raf.write(b); + } + }, StandardCharsets.UTF_8); + raf.seek(targetPath.toFile().length()); + + if (eventReceived) { + fw.write("],"); } - }, StandardCharsets.UTF_8); - raf.seek(targetPath.toFile().length()); + fw.flush(); - if ( eventReceived ) { - fw.write("],"); + try (AppMapSerializer serializer = AppMapSerializer.reopen(fw, raf)) { + serializer.write(this.getClassMap(), this.metadata, this.eventUpdates); + } } - fw.flush(); - - AppMapSerializer serializer = AppMapSerializer.reopen(fw, raf); - serializer.write(this.getClassMap(), this.metadata, this.eventUpdates); } catch (IOException e) { throw new RuntimeException(e); } @@ -124,16 +126,24 @@ public synchronized Recording stop() { throw new IllegalStateException("AppMap: Unable to stop the recording because no recording is in progress."); } + File file = this.tmpPath.toFile(); try { this.serializer.write(this.getClassMap(), this.metadata, this.eventUpdates); } catch (IOException e) { throw new RuntimeException(e); + } finally { + // Ensure serializer is closed even if write() throws an exception + try { + if (this.serializer != null) { + this.serializer.close(); + } + } catch (IOException e) { + logger.error("Failed to close serializer", e); + } + this.serializer = null; + this.tmpPath = null; } - File file = this.tmpPath.toFile(); - this.serializer = null; - this.tmpPath = null; - logger.debug("Recording finished"); logger.debug("Wrote recording to file {}", file.getPath()); @@ -163,7 +173,9 @@ void start() { try { this.tmpPath = Files.createTempFile(null, ".appmap.json"); this.tmpPath.toFile().deleteOnExit(); - this.serializer = AppMapSerializer.open(new OutputStreamWriter(new FileOutputStream(this.tmpPath.toFile()), StandardCharsets.UTF_8)); + FileOutputStream fileOutputStream = new FileOutputStream(this.tmpPath.toFile()); + OutputStreamWriter writer = new OutputStreamWriter(fileOutputStream, StandardCharsets.UTF_8); + this.serializer = AppMapSerializer.open(writer); } catch (IOException e) { this.tmpPath = null; this.serializer = null;