Skip to content

Fix: when calling Throwable.printStackTrace, logs from other threads may be inserted.#6487

Closed
valord577 wants to merge 2 commits intoGenymobile:devfrom
valord577:dev
Closed

Fix: when calling Throwable.printStackTrace, logs from other threads may be inserted.#6487
valord577 wants to merge 2 commits intoGenymobile:devfrom
valord577:dev

Conversation

@valord577
Copy link
Contributor

Fix: when calling Throwable.printStackTrace, logs from other threads may be inserted.

…s may be inserted.

Signed-off-by: valord577 <valord577@gmail.com>
@valord577
Copy link
Contributor Author

@rom1v Hi, I am new here. Do you have time to review this PR?

@rom1v
Copy link
Collaborator

rom1v commented Nov 18, 2025

Thank you for the PR. 👍

Indeed, I have noticed that some bug reports contain interleaved error messages (but I could never reproduced).

I am wondering, is it really necessary to use an intermediate StringWriter?

I think this should also work:

diff --git server/src/main/java/com/genymobile/scrcpy/util/Ln.java server/src/main/java/com/genymobile/scrcpy/util/Ln.java
index 46057b4d3..b9b548afc 100644
--- server/src/main/java/com/genymobile/scrcpy/util/Ln.java
+++ server/src/main/java/com/genymobile/scrcpy/util/Ln.java
@@ -77,9 +77,7 @@ public final class Ln {
             synchronized (CONSOLE_ERR) {
                 CONSOLE_ERR.print(PREFIX + "ERROR: " + message + '\n');
                 if (throwable != null) {
-                    StringWriter sw = new StringWriter();
-                    throwable.printStackTrace(new PrintWriter(sw));
-                    CONSOLE_ERR.print(sw.toString());
+                    throwable.printStackTrace(CONSOLE_ERR);
                 }
             }
         }
@@ -95,9 +93,7 @@ public final class Ln {
             synchronized (CONSOLE_ERR) {
                 CONSOLE_ERR.print(PREFIX + "ERROR: " + message + '\n');
                 if (throwable != null) {
-                    StringWriter sw = new StringWriter();
-                    throwable.printStackTrace(new PrintWriter(sw));
-                    CONSOLE_ERR.print(sw.toString());
+                    throwable.printStackTrace(CONSOLE_ERR);
                 }
             }
         }

@rom1v
Copy link
Collaborator

rom1v commented Nov 18, 2025

Indeed, I have noticed that some bug reports contain interleaved error messages (but I could never reproduced).

In fact, this should be a different problem, since throwable.printStackTrace(…) does lock the stream/writer:

    private void printStackTrace(PrintStreamOrWriter s) {
        // Guard against malicious overrides of Throwable.equals by
        // using a Set with identity equality semantics.
        Set<Throwable> dejaVu = Collections.newSetFromMap(new IdentityHashMap<>());
        dejaVu.add(this);

        synchronized (s.lock()) {
            // Print our stack trace
            s.println(this);
            StackTraceElement[] trace = getOurStackTrace();
            for (StackTraceElement traceElement : trace)
                s.println("\tat " + traceElement);

            // Print suppressed exceptions, if any
            for (Throwable se : getSuppressed())
                se.printEnclosedStackTrace(s, trace, SUPPRESSED_CAPTION, "\t", dejaVu);

            // Print cause, if any
            Throwable ourCause = getCause();
            if (ourCause != null)
                ourCause.printEnclosedStackTrace(s, trace, CAUSE_CAPTION, "", dejaVu);
        }
    }

Adding synchronization would only avoid another message inserted between the print() and printStackTrace() calls.

@valord577
Copy link
Contributor Author

throwable.printStackTrace(…) does lock the stream/writer

As I understand it, it locks not CONSOLE_ERR, but new WrappedPrintStream(s).

Maybe I've misunderstood?

// File: java/lang/Throwable.java

    public void printStackTrace(PrintStream s) {
        printStackTrace(new WrappedPrintStream(s));
                     // ^ Locks this object.
    }

    private void printStackTrace(PrintStreamOrWriter s) {
        ...
        ...
    }

Indeed, I have noticed that some bug reports contain interleaved error messages (but I could never reproduced).

Screenshot 2025-11-18 at 16 55 53

I am wondering, is it really necessary to use an intermediate StringWriter?

Adding synchronization would only avoid another message inserted between the print() and printStackTrace() calls.

Perhaps we can emulate Android's logcat to implement a custom printStackTrace method?

See the line contains lbbw.flush();.

// File: android/util/Log.java

    public static int printlns(int bufID, int priority, @Nullable String tag, @NonNull String msg,
            @Nullable Throwable tr) {
        ImmediateLogWriter logWriter = new ImmediateLogWriter(bufID, priority, tag);
        // Acceptable buffer size. Get the native buffer size, subtract two zero terminators,
        // and the length of the tag.
        // Note: we implicitly accept possible truncation for Modified-UTF8 differences. It
        //       is too expensive to compute that ahead of time.
        int bufferSize = PreloadHolder.LOGGER_ENTRY_MAX_PAYLOAD    // Base.
                - 2                                                // Two terminators.
                - (tag != null ? tag.length() : 0)                 // Tag length.
                - 32;                                              // Some slack.
        // At least assume you can print *some* characters (tag is not too large).
        bufferSize = Math.max(bufferSize, 100);

        LineBreakBufferedWriter lbbw = new LineBreakBufferedWriter(logWriter, bufferSize);

        lbbw.println(msg);

        if (tr != null) {
            // This is to reduce the amount of log spew that apps do in the non-error
            // condition of the network being unavailable.
            Throwable t = tr;
            while (t != null) {
                if (t instanceof UnknownHostException) {
                    break;
                }
                if (t instanceof DeadSystemException) {
                    lbbw.println("DeadSystemException: The system died; "
                            + "earlier logs will point to the root cause");
                    break;
                }
                t = t.getCause();
            }
            if (t == null) {
                tr.printStackTrace(lbbw);
            }
        }

        lbbw.flush();
          // ^^^ Only flush logs here ^^^

        return logWriter.getWritten();
    }

@valord577
Copy link
Contributor Author

Synchronization locks only occur when errors are reported. Such situations should be avoided whenever possible. Therefore, the impact on performance should be minimal.

@valord577
Copy link
Contributor Author

I am wondering, is it really necessary to use an intermediate StringWriter?

It is necessary to use a string concatenation class to collect stack trace information. Otherwise, it will be printed directly to stderr/stdout.

    private void printStackTrace(PrintStreamOrWriter s) {

        Set<Throwable> dejaVu = Collections.newSetFromMap(new IdentityHashMap<>());
        dejaVu.add(this);

        synchronized (s.lock()) {
            s.println(this);
            StackTraceElement[] trace = getOurStackTrace();
            for (StackTraceElement traceElement : trace)
                s.println("\tat " + traceElement);
               // ^^^ Print message directly to stderr/stdout

            for (Throwable se : getSuppressed())
                se.printEnclosedStackTrace(s, trace, SUPPRESSED_CAPTION, "\t", dejaVu);
                // ^^^ Print message directly to stderr/stdout
            
            Throwable ourCause = getCause();
            if (ourCause != null)
                ourCause.printEnclosedStackTrace(s, trace, CAUSE_CAPTION, "", dejaVu);
                      // ^^^ Print message directly to stderr/stdout
        }
    }

@rom1v
Copy link
Collaborator

rom1v commented Nov 18, 2025

As I understand it, it locks not CONSOLE_ERR, but new WrappedPrintStream(s).

It locks s.lock():

synchronized (s.lock()) {
    // …
}

And WrappedPrintStream is defined as follow:

    private static class WrappedPrintStream extends PrintStreamOrWriter {
        private final PrintStream printStream;

        WrappedPrintStream(PrintStream printStream) {
            this.printStream = printStream;
        }

        Object lock() {
            return printStream;
        }

        void println(Object o) {
            printStream.println(o);
        }
    }

So it locks CONSOLE_ERR.


Screenshot 2025-11-18 at 16 55 53

👍

Yes, I think adding synchronized (CONSOLE_ERR) around the two calls will fix this issue.

Synchronization locks only occur when errors are reported. Such situations should be avoided whenever possible. Therefore, the impact on performance should be minimal.

Sure.

It is necessary to use a string concatenation class to collect stack trace information. Otherwise, it will be printed directly to stderr/stdout.

In any case, it is necessary to call printStackTrace() with a Writer argument.

But that Writer can be CONSOLE_ERR directly, without using an additional intermediate StringWriter.

…s may be inserted.

Signed-off-by: valord577 <valord577@gmail.com>
@valord577
Copy link
Contributor Author

LGTM. I change the code! 👍

rom1v pushed a commit that referenced this pull request Nov 18, 2025
Between the calls to CONSOLE_ERR.print() and
printStackTrace(CONSOLE_ERR), logs from other threads may be inserted.

Synchronizing access to CONSOLE_ERR ensures that logs from different
threads do not mix.

PR #6487 <#6487>

Signed-off-by: valord577 <valord577@gmail.com>
Signed-off-by: Romain Vimont <rom@rom1v.com>
@rom1v
Copy link
Collaborator

rom1v commented Nov 18, 2025

I squashed your two commits and updated the commit message: 7dd9bca

Does it look ok to you?

@valord577
Copy link
Contributor Author

LGTM. No problem.

@rom1v
Copy link
Collaborator

rom1v commented Nov 18, 2025

Merged into dev.

@rom1v rom1v closed this Nov 18, 2025
Linux4 added a commit to Linux4/scrcpy that referenced this pull request Feb 10, 2026
scrcpy v3.3.4

Changes since v3.3.3:
 - Fix permission denial error after Android upgrade (Genymobile#6523)
 - Fix state restoration on certain devices (Genymobile#6405, Genymobile#6540)
 - Fix UHID_OUTPUT message parsing (Genymobile#6415)
 - Fix failure when the uniqueId field is missing on certain devices (Genymobile#6461)
 - Fix error log interleaving (Genymobile#6487)
 - Fix startup issue on certain Meizu devices (Genymobile#6480)
 - Fix handling of non-integer ANDROID_PLATFORM in build script (Genymobile#6408)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants