feat: support adding and removing attachments after initialization

[bobbyg603]

Closes #25

Summary

• Add BugSplat.addAttachment(path) / BugSplat.removeAttachment(path) so crash attachments can be updated after init, matching setAttribute / removeAttribute.
• Crashpad copies --attachment paths into the handler argv at StartHandlerAtCrash and never rereads them. A small PIE wrapper (libbugsplat_handler.so) is exec'd instead: it loads <database>/bugsplat_attachments.list and execs libcrashpad_handler.so with the current set.
• Paths are resolved when a crash is uploaded (Crashpad skips missing files), matching Windows and Apple. Callers can attach a log file before anything has written to it.
• Init-time attachments go through the same list. Adding an already-attached path is a no-op; removing a path that is not attached is a no-op.

Also in this PR: promoted-field setters

Added alongside the attachment work because both close the same gap — bugsplat-unity could not reach either capability, and shipping them separately would mean two AAR releases for one Unity release.

public static void setUser(String user);
public static void setEmail(String email);
public static void setNotes(String notes);
public static void setKey(String key);

Callers previously had to know the reserved attribute names the backend promotes (BugSplatUser, BugSplatEmail, BugSplatNotes, BugSplatApplicationKey) and set them through setAttribute, which couples clients to a naming convention this SDK does not document.

• Native crashes reach BugSplat through Crashpad, which carries annotations and nothing else, so the setters still write those attributes.
• ANRs are committed directly rather than through Crashpad, so the values are retained in BugSplat and applied to the commit request as typed CommitOptions fields — user, email, notes, appKey — which ANR reports previously never carried.
• Values set before init are replayed once the native annotation list exists, so call order does not matter.
• The example app sets all four at startup so they can be confirmed on the dashboard.

Test plan

• ./gradlew :app:testDebugUnitTest — including new AttachmentPathTest (null/blank/newline rejection)
• ./gradlew :app:assembleDebug — wrapper packaged in the AAR for arm64-v8a, armeabi-v7a, and x86_64
• Wrapper is a PIE executable with 16KB (0x4000) LOAD alignment
• Example app compiles (:example:compileDebugJavaWithJavac); demo attaches app.log after init
• On device: addAttachment of a log written mid-session, crash, confirm the file on the BugSplat dashboard
• On device: removeAttachment, crash, confirm the file is absent

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds runtime attachment management to the BugSplat Android SDK by introducing addAttachment / removeAttachment APIs and implementing a Crashpad handler wrapper so attachment paths can be updated after initialization and resolved at upload time.

Changes:

• Add public Java APIs BugSplat.addAttachment(path) / BugSplat.removeAttachment(path) plus bridge + JNI plumbing.
• Persist attachment paths to <crashpad_db>/bugsplat_attachments.list and introduce a PIE wrapper (libbugsplat_handler.so) that reads the list at crash time and execs libcrashpad_handler.so with --attachment= args.
• Add validation utility + unit tests, update README and example app to demonstrate post-init attachment usage, and update build packaging to ship the wrapper.

File summaries

File	Description
README.md	Documents post-init attachment add/remove usage.
example/src/main/java/com/bugsplat/example/MainActivity.java	Demonstrates writing and attaching a log file after BugSplat.init.
app/src/test/java/com/bugsplat/android/AttachmentPathTest.java	Unit tests for attachment path validation (null/blank/newline rejection).
app/src/main/java/com/bugsplat/android/BugSplatBridge.java	Adds bridge-level add/remove attachment methods and new JNI declarations.
app/src/main/java/com/bugsplat/android/BugSplat.java	Exposes public SDK APIs with Javadoc for add/remove attachment.
app/src/main/java/com/bugsplat/android/AttachmentPath.java	Centralized path validation helper used by the new API surface.
app/src/main/cpp/native-lib.cpp	Implements attachment persistence, wrapper selection, and JNI add/remove attachment behavior.
app/src/main/cpp/include/bugsplat_utils.h	Updates createAttachments signature/contract to return strings and resolve at upload time.
app/src/main/cpp/include/bugsplat_attachments.h	Defines the shared list filename constant used by SDK + wrapper.
app/src/main/cpp/handler_wrapper.cpp	New exec-wrapper that reads the attachments list and appends --attachment= args.
app/src/main/cpp/CMakeLists.txt	Builds wrapper as PIE, copies into a Gradle-merged jniLibs directory, and links it.
app/build.gradle	Adds the wrapper output directory to jniLibs.srcDirs for packaging.

Review details

• Files reviewed: 12/12 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bobbyg603]

Summary

The wrapper-plus-list-file approach is the right way around Crashpad freezing --attachment argv at StartHandlerAtCrash, and the implementation is mostly solid: PIE libbugsplat_handler.so is packaged in the AAR for all three ABIs, the list is atomically replaced (tmp + fsync + rename), and post-init add/remove is mutex-protected. Crashpad still copies attachment bytes when the minidump is written (missing files skipped then), not at HTTP upload; the public docs get that wrong. Dominant remaining risks are silent attachment loss when list persistence fails (especially at init, where the wrapper path no longer puts paths in argv) and init-time paths bypassing the newline validation the new line-oriented file format requires.

Issue counts by severity

• bugs: 0
• suggestions: 4
• nits: 1

[bobbyg603]

[suggestion] The new API/Javadoc (and README / bugsplat_utils.h) say the file is read "when a crash is uploaded." Crashpad's Linux/Android handler copies --attachment files in WriteMinidumpToDatabase at crash-dump time (FileReader::Open failure → log and skip). A file that appears only after the process dies — for example on the next launch before pending reports upload — will not be attached. Native comments in native-lib.cpp already say "crash time"; the public contract should match that. "Does not need to exist yet" is still true at addAttachment time.

Suggestion: Say the path is resolved when the crash is captured (handler copies the file into the Crashpad database; missing files are skipped). Keep the "need not exist at add/init time" guarantee. Update README and bugsplat_utils.h the same way.

[bobbyg603]

Fixed in 29e6dca. Javadoc, README, and bugsplat_utils.h now say the path is copied when the crash is captured (missing files skipped). The file still need not exist at add/init time.

[bobbyg603]

[suggestion] persistAttachmentsLocked()'s return value is ignored at init, in addAttachmentPath, and in removeAttachmentPath. With the wrapper (the normal path), init-time attachments are no longer passed into StartHandlerAtCrash — they live only in bugsplat_attachments.list. A failed write therefore drops those attachments with no signal to the caller, and a failed add still mutates g_attachments, so a retry of the same path is a no-op while the wrapper keeps reading the old file.

Suggestion: Honor the persist result: on failure, roll back the in-memory change (or never commit until persist succeeds), and at init either log loudly or fall back to passing attachmentPaths into StartHandlerAtCrash so existing init attachments are not lost.

[bobbyg603]

Fixed in 29e6dca. Persist failures roll back in-memory state. At init, if persist fails we still pass the snapshot into StartHandlerAtCrash so those attachments are not dropped, and we log an error.

[bobbyg603]

[suggestion] AttachmentPath.validate rejects null/blank/newline paths for addAttachment/removeAttachment, but init(..., attachments) still feeds strings straight into createAttachments and then into the line-oriented list file. A path containing \n or \r becomes two --attachment arguments at crash time. That corruption did not exist when paths were FilePath objects in argv.

Suggestion: Validate (or skip and log) each init attachment with the same rules as AttachmentPath.validate before persisting.

[bobbyg603]

Fixed in 29e6dca. Init-time paths go through the same absolute / no-newline checks as addAttachment before they are written to the list file.

[bobbyg603]

[suggestion] The wrapper locates libcrashpad_handler.so via readlink("/proc/self/exe"). If that fails it returns 1 without exec'ing the real handler, so the crash is lost. Crashpad's StartHandlerAtCrash already passes the absolute wrapper path as argv[0]; that is a more direct sibling-path source than /proc/self/exe (which can fail or pick up a (deleted) suffix after an in-place APK update).

Suggestion: Derive the handler directory from argv[0] (fall back to /proc/self/exe if needed). If the sibling libcrashpad_handler.so still cannot be resolved, execv the original argv rather than exiting — a dump without extra attachments is better than none.

[bobbyg603]

Fixed in 29e6dca: the wrapper now resolves libcrashpad_handler.so from argv[0] first, then /proc/self/exe (stripping a (deleted) suffix).

Did not execv the original argv when lookup fails: argv[0] is this wrapper, so that would recurse. A dump still requires locating the sibling handler.

[bobbyg603]

[nit] #include <dirent.h> is unused.

Suggestion: Remove it.

[bobbyg603]

Fixed in 29e6dca. Removed the unused #include <dirent.h>.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

app/src/main/cpp/native-lib.cpp:105

• jniInitBugSplat copies multiple GetStringUTFChars results into std::string but never calls ReleaseStringUTFChars, which leaks/pins the underlying Java string storage for the lifetime of the process (and also leaks on early returns). Consider copying into std::string and releasing immediately for each input jstring.

    string dataDir = env->GetStringUTFChars(data_dir, nullptr);
    string libDir = env->GetStringUTFChars(lib_dir, nullptr);

    // Crashpad file paths. Prefer the wrapper so post-init add/remove of
    // attachments is reflected at crash time; fall back to the stock handler.

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new handler wrapper can mis-parse overlong list-file lines into unintended extra attachments; the parsing should be hardened before approval.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

app/src/main/cpp/handler_wrapper.cpp:113

• The wrapper reads attachment paths with fgets(line, PATH_MAX, file) and then treats each buffer as a complete path. If a path line exceeds PATH_MAX-1 (or the list file is corrupted), fgets returns a truncated chunk without a trailing newline; the next fgets call will read the remainder as a second “path”, producing unintended --attachment= args. Detect truncated reads and skip/drain the remainder of the overlong line before continuing.

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new handler wrapper/selection logic needs robustness fixes (executability checks and safe parsing/truncation handling) to avoid initialization/crash-time failures and unintended attachments.

Review details

Suppressed comments (7)

Previously missed (4) — in code that hasn't changed since the last review.

app/src/main/cpp/handler_wrapper.cpp:61

• handlerFromExePath only checks F_OK for libcrashpad_handler.so, but this wrapper immediately execvs that path. If the file exists but is not executable, execv will fail at crash time. Check X_OK to ensure the resolved handler is actually executable.
  app/src/main/cpp/handler_wrapper.cpp:123
• The wrapper reads bugsplat_attachments.list with a fixed PATH_MAX buffer and treats every fgets chunk as a distinct attachment. If a line exceeds the buffer (or the file is corrupted), one path can be split into multiple unintended attachments (and the wrapper will also silently ignore any attachments beyond MAX_EXTRA_ATTACHMENTS). Detect and skip overlong lines, validate absolute paths, and log when the list is truncated.
  app/src/main/cpp/native-lib.cpp:108
• useWrapper only checks that libbugsplat_handler.so exists. If the file is present but not executable (permissions/corrupt packaging), the SDK will still try to start it as the Crashpad handler and initialization can fail. Prefer checking X_OK (or otherwise verifying executability) before selecting the wrapper.
  app/src/main/java/com/bugsplat/android/BugSplat.java:130
• setUser's Javadoc implies it affects subsequent native crash reports, but if it’s called before init the native annotation list isn’t registered yet (native logs "setAttribute called before init" and drops the update). Either persist/apply the value on init or document that this should be called after init to affect native crashes.

This issue also appears in the following locations of the same file:

• line 140
• line 152
• line 164

app/src/main/java/com/bugsplat/android/BugSplat.java:142

• Same as setUser: if setEmail is called before init, the native attribute update is dropped because the annotation list isn’t initialized yet. Either apply stored values during init or document that callers must call this after init for native crash reports.

     * Set the user email reported with subsequent crash, ANR, and feedback reports.
     *
     * <p>Passing null clears the value.</p>

app/src/main/java/com/bugsplat/android/BugSplat.java:154

• Same as setUser: if setNotes is called before init, the native attribute update is dropped because the annotation list isn’t initialized yet. Either apply stored values during init or document that callers must call this after init for native crash reports.

     * Set the notes reported with subsequent crash, ANR, and feedback reports.
     *
     * <p>Passing null clears the value.</p>

app/src/main/java/com/bugsplat/android/BugSplat.java:166

• Same as setUser: if setKey is called before init, the native attribute update is dropped because the annotation list isn’t initialized yet. Either apply stored values during init or document that callers must call this after init for native crash reports.

     * Set the application key reported with subsequent crash, ANR, and feedback reports.
     *
     * <p>Passing null clears the value.</p>

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There’s a confirmed persist-failure edge case that can cause stale attachments to be added at crash time, and the PR also introduces additional public API not described in the PR’s stated scope.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

app/src/main/cpp/handler_wrapper.cpp:113

• The handler wrapper currently turns any non-empty line from bugsplat_attachments.list into a --attachment= argument. Adding a minimal absolute-path check here hardens against a corrupted/stale list file producing malformed attachment args.
  app/src/main/cpp/native-lib.cpp:164
• If persisting the attachments list fails and a stale bugsplat_attachments.list from a previous run exists, the wrapper will still read it at crash time and append outdated attachments on top of the argv snapshot. This can produce unexpected extra attachments even though init logged a persist failure. Consider deleting the list file on persist failure so the wrapper won’t pick up stale entries.
  app/src/main/java/com/bugsplat/android/BugSplat.java:116
• This change adds new public API for promoted fields (BugSplatUser/BugSplatEmail/BugSplatNotes/BugSplatApplicationKey) along with new setters (setUser/setEmail/setNotes/setKey) and updates ANR commits to use them, but the PR description/linked issue describe only post-init attachment add/remove. Please either document these new APIs and call out the additional scope in the PR description, or move them to a separate PR to keep the attachment change focused.

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[bobbyg603]

Addressed the three suppressed findings from the latest review, plus the setUser-before-init note from the earlier one, in 4344bf3.

native-lib.cpp:164 — stale list on persist failure. Fixed. Confirmed: when persisting fails, init falls back to the argv snapshot, but a list file from a previous run stays on disk and the wrapper still reads it at crash time, appending paths this session never registered. The file is now unlinked when persisting fails, so the wrapper finds nothing and argv is the only source.

handler_wrapper.cpp:113 — validate list lines. Fixed. Lines that are not absolute paths are skipped rather than turned into --attachment= arguments. Only absolute paths are ever written, so anything else means a corrupted or stale file.

BugSplat.java:116 — undocumented scope. Fixed in the PR description. The setters are deliberate here rather than split out: bugsplat-unity needs both capabilities for its 5.0.0 release, and splitting them would mean two AAR releases for one Unity release. The description now documents the API, the Crashpad-vs-ANR routing, and the example-app usage.

setUser before init (earlier review). Fixed. The native annotation list does not exist until jniInitBugSplat runs, so an early setUser was logged and dropped. The stored values are replayed from initBugSplat once the list exists, so call order no longer matters.

:app:assembleDebug and :app:testDebugUnitTest pass. The stale-list and pre-init paths are both covered only by inspection — neither has a unit test, since both need the native library loaded.

[Copilot]

🟡 Changes recommended

The review found concrete native/bridge issues (JNI UTF string resource handling and attachment removal no-op semantics) that can cause leaks/misleading failures and should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

app/src/main/cpp/native-lib.cpp:105

• jniInitBugSplat uses GetStringUTFChars() for data_dir/lib_dir/database/application/version but never releases those UTF buffers. Per JNI contract this leaks memory (and may pin the Java strings), and also the wrapper existence check should verify executability (X_OK) rather than just existence (F_OK) to avoid selecting a non-executable wrapper and losing crash reports.
  app/src/main/cpp/native-lib.cpp:432
• removeAttachmentPath() always rewrites the attachments list file even when the requested path wasn't attached. If persisting fails (e.g., transient I/O error), this turns what should be a no-op into a reported failure, which contradicts the documented behavior that removing a non-attached path is a no-op.

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Lite