Fixed: Widen TerminalRow's index so very long lines don't overflow it. #3840
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
So I looked into it, it does fix the crash, but like you said terminal still becomes laggy, the code for jackpal/Android-Terminal-Emulator@9a47042 jackpal/Android-Terminal-Emulator#338 a18ee58#diff-f84d215b18106c037e01986a3968fa54b74691174a78fcc99493f745d3805be5 |
|
I sent this as a separate PR because I think it's a sensible incremental change. Both because using a short as a progress index in an array whose size isn't obviously bounded to 2^15 is unambiguously, locally wrong, and because the upgrade from "crash" to "lag recoverable with |
What do you mean, start working on it ASAP! Finish what you started! Chop chop! I'll take another look on upper bounds and why short was used before, and if it was just a mistake. If it's reasonable, then will merge, I agree, lag is better than crash. |
|
Check fcd7a34 for fix and details, hopefully it doesn't break anything. The build in https://github.com/termux/termux-app/actions/runs/8378462868 should be tested before this is merged. cc @Kimapr |
|
Changing from |
… to prevent buffer overflows The exception below causing app crash happens because of malicious input where combining characters keep getting added to same column of the row and this increases the size of `mSpaceUsed` and `mText`, eventually causing a buffer overflow of `mSpaceUsed`, which is limited to max `32767` value as per java `short` limit, but the limit itself isn't the issue, but an endless number of combining characters being added. Check `MAX_COMBINING_CHARACTERS_PER_COLUMN` field javadocs for why the limit `15` was chosen. ``` curl -o matroska.js https://kimapr.net/lappy/matroska.js cat matroska.js ``` The `charCount` below refers to value of `Character.charCount(codePoint)`, like before `oldCharactersUsedForColumn` is appended to `newCharactersUsedForColumn`. ``` TerminalRow: codePoint=112, mColumns=98, mText=637, columnToSet=18, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=510, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=511, newNextColumnIndex=511, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=40, mColumns=98, mText=637, columnToSet=19, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=511, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=512, newNextColumnIndex=512, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=40, mColumns=98, mText=637, columnToSet=20, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=512, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=513, newNextColumnIndex=513, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=101, mColumns=98, mText=637, columnToSet=21, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=513, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=514, newNextColumnIndex=514, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=917772, mColumns=98, mText=147, columnToSet=18, mSpaceUsed=98, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=3, oldNextColumnIndex=19, newNextColumnIndex=21, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 I TerminalRow: codePoint=65024, mColumns=98, mText=147, columnToSet=18, mSpaceUsed=100, javaCharDifference=1, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=3, newCharactersUsedForColumn=4, oldNextColumnIndex=21, newNextColumnIndex=22, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917772, mColumns=98, mText=147, columnToSet=18, mSpaceUsed=101, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=4, newCharactersUsedForColumn=6, oldNextColumnIndex=22, newNextColumnIndex=24, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 ... TerminalRow: codePoint=917959, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=32763, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=32666, newCharactersUsedForColumn=32668, oldNextColumnIndex=32684, newNextColumnIndex=32686, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917939, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=32765, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=32668, newCharactersUsedForColumn=32670, oldNextColumnIndex=32686, newNextColumnIndex=32688, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917961, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=32767, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=32670, newCharactersUsedForColumn=32672, oldNextColumnIndex=32688, newNextColumnIndex=32690, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917804, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=-32767, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=3, oldNextColumnIndex=19, newNextColumnIndex=21, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 ``` ``` java.lang.ArrayIndexOutOfBoundsException: src.length=32781 srcPos=19 dst.length=32781 dstPos=21 length=-32786 at java.lang.System.arraycopy(System.java:469) at com.termux.terminal.TerminalRow.setChar(TerminalRow.java:196) at com.termux.terminal.TerminalBuffer.setChar(TerminalBuffer.java:455) at com.termux.terminal.TerminalEmulator.emitCodePoint(TerminalEmulator.java:2380) at com.termux.terminal.TerminalEmulator.processCodePoint(TerminalEmulator.java:624) at com.termux.terminal.TerminalEmulator.processByte(TerminalEmulator.java:520) at com.termux.terminal.TerminalEmulator.append(TerminalEmulator.java:487) at com.termux.terminal.TerminalSession$MainThreadHandler.handleMessage(TerminalSession.java:358) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:223) at android.app.ActivityThread.main(ActivityThread.java:7664) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947) ``` See also following links for history of related changes to `TerminalRow` for combining characters. Note that jackpal terminal does not crash for above, which termux-app is based on, but changes were done by fornwall in initial commit of termux-app to change the behaviour, hence the crash, but he added the `FIXME: Put a limit of combining characters` comment as a note to solve the current issue in future, which is now. - jackpal/Android-Terminal-Emulator@9a47042 - jackpal/Android-Terminal-Emulator#338 - termux@a18ee58#diff-f84d215b18106c037e01986a3968fa54b74691174a78fcc99493f745d3805be5 Closes termux#3839
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This prevents the crash in #3839, but not by enough to call it done. Termux gets stuck on the very bad line and is responsive, but laggy enough to trigger ANRs. It's possible to ^C in this state and get your terminal back with no apparent damage.