Fixed: fully consume unknown CSI sequences containing non-numeric parameter byte #4417
Conversation
|
I have compiled and tested this PR I can confirm that this passes the test case given in the issue, Tested device: Samsung Galaxy S III SPH-L710, Android 7.1.2 |
…ameter and intermediate bytes Standard ECMA-48: Control Functions for Coded Character Sets specifies the format of CSI commands. - https://en.wikipedia.org/wiki/ANSI_escape_code#Control_Sequence_Introducer_commands - https://invisible-island.net/xterm/ecma-48-parameter-format.html#section5.4 Previously unsupported bytes would be echoed to the terminal. ```shell $ printf '\x1b[=u' # PF u $ printf '\x1b[=5u' # PPF 5u $ printf '\x1b[=5!u' # PPIF 5!u $ printf '\x1b[=5!%u' # PPIIF 5!0 $ printf '\x1b[=?5!%u' # PPPIIF ?5!0 ``` This fixes a problem with fish shell 4.0.0 which uses that sequence. Closes termux#4338 Co-authored-by: @krobelus <aclopte@gmail.com> Co-authored-by: @agnostic-apollo <agnosticapollo@gmail.com>
9161c07 to
7c0821a
Compare
|
Test this and let me know, I will merge within next 24 hr. |
|
I'm not sure if it's easy for me to test but FWIW the changes look good. |
|
You can grab a build from https://github.com/termux/termux-app/actions/runs/13851563135?pr=4417 if you are using github releases instead of F-Droid. There is no need to handle invalid sequences as there would be tonne/or infinite possibilities, the above was handled because it was valid, but unsupported by our terminal currently. The |
|
On Fri, Mar 14, 2025 at 01:55:33AM -0700, agnostic-apollo wrote:
agnostic-apollo left a comment (termux/termux-app#4417)
You can grab a build from https://github.com/termux/termux-app/actions/runs/13851563135?pr=4417 if you are using github releases instead of F-Droid.
I don't have Android Studio or Android set up.
There is no need to handle invalid sequences as there would be
tonne/or infinite possibilities, the above was handled because
it was valid, but unsupported by our terminal currently. The `!`
is already captured by `ESC_CSI_EXCLAMATION`, but only `CSI ! p`
is valid for a DECSTR reset, other parameters would result in error
being logged if logging is enabled.
`CSI ! ! a` is a syntactically valid CSI command, we should consume the whole thing.
|
|
Maybe I can run it in Chrome, not sure. Hard to tell how much time that will take me
|
Under what spec is it valid and what operation is supposed to be performed?
Github action already builds the APKs for you, download the attached zip, extract the apk and install. |
|
I have tested the newer version. For me it is still working the same ( |
|
On Fri, Mar 14, 2025 at 02:30:51AM -0700, agnostic-apollo wrote:
agnostic-apollo left a comment (termux/termux-app#4417)
>`CSI ! ! a` is a syntactically valid CSI command, we should consume the whole thing.
Under what spec is it valid and what operation is supposed to be performed?
Given
1. ECMA-48 does not limit the number of intermediate bytes
2. the combination of intermediate bytes and final byte identifies the control function
3. Regarding the final byte, it says: `Bit combinations 07/00 to 07/14 are available as Final Bytes of control sequences for private (or experimental) use.`
it seems to me that in theory someone might legally use `CSI ! ! 0x70` (aka `CSI ! ! p`) for a private-use command
(and private-use ones might become de-facto standard.. I think fixterm's `CSI u` is an example but that might be inaccurate)
|
I already tested that part, checking fish shell, or at least no apparent issue with text editors is what would need testing. |
|
oh ok, now I have tested fish with termux/termux-packages#23686 reverted, vim, and vim inside fish with termux/termux-packages#23686 reverted, and nothing seems to be wrong. |
|
The only supported byte after a
|
|
Thanks for testing @robertkirkman. That should probably be enough. |
|
I agree that it's enough testing for this fish version. Every new CSI related command is sent at fish startup so errors should be noticeable. Thanks everyone for your work. We've had a bunch of reports about this since late December (from the fish beta release); I'll try to act faster next time. Now on to getting nerd-sniped: I'm not thinking of any commands that are in use today. I'm also not talking about private mode. Only about commands that To me it looks like a complete CSI parser should parse any (unknown) "private or experimental use" commands. |
But an intermediate byte must always be proceeded by a valid parameter byte, which
This pull already started parsing all private and experimental ranges and ignoring them. Any other character outside the supported ranges should not be used and should be an error, until someone publishes a standard. There is enough range left for private/experimental use left by the standard for someone to have to go outside it.
Check the code table in following link and compare the Additionally, the 5.1.2 section of 3rd and 4.1.2 section of 2nd editors of the standard had an explicit error condition statement for using unsupported combinations, which seems to have gotten removed in later editions for some reason.
|
|
You are welcome and thanks again for the pull. |
- Fixes #26707 - Revert fish-shell/fish-shell@70bd49f as planned and previously noted in #26466 - After fish-shell/fish-shell@eb4cec1, `fix-import-when-posix-spawn-disabled.patch` is necessary to prevent compilation error `failed to resolve: use of undeclared type 'Pid'` - After fish-shell/fish-shell@91ee45b, `revert-6644cc9.patch` is no longer necessary to prevent a compilation failure, and can be removed. - termux/termux-app#4417 has been in [Termux 0.118.3](https://github.com/termux/termux-app/releases/tag/v0.118.3) for 4 months, meaning that `status test-feature keyboard-protocols && set -U fish_features no-keyboard-protocols` can be removed.
- Fixes termux/termux-packages#26707 - Revert fish-shell/fish-shell@70bd49f as planned and previously noted in termux/termux-packages#26466 - After fish-shell/fish-shell@eb4cec1, `fix-import-when-posix-spawn-disabled.patch` is necessary to prevent compilation error `failed to resolve: use of undeclared type 'Pid'` - After fish-shell/fish-shell@91ee45b, `revert-6644cc9.patch` is no longer necessary to prevent a compilation failure, and can be removed. - termux/termux-app#4417 has been in [Termux 0.118.3](https://github.com/termux/termux-app/releases/tag/v0.118.3) for 4 months, meaning that `status test-feature keyboard-protocols && set -U fish_features no-keyboard-protocols` can be removed.
Standard ECMA-48: Control Functions for Coded Character Sets specifies
the format of CSI commands. Wikipedia has a concise description:
https://en.wikipedia.org/wiki/ANSI_escape_code#Control_Sequence_Introducer_commands
Do this for at least for sequences like "CSI = 5 u" that
contain non-numeric parameter bytes.
This fixes a problem with fish shell 4.0.0 which (for better or worse)
uses that sequence.
This patch introduces a slight inconsistency: for the above example,
"unknownSequence('u')" will be called. The resulting log message may
be confusing, because we do support "CSI u". We should fix this to
log the entire unknown sequence. I can try doing that.
In future we should also fully consume all other unknown CSI commands.
A contrived example would be "CSI ! ! a". If desired, I'm happy to
fix those as well, as I have some context already (but I don't think
that would block this patch).
Closes #4338