notcurses icon indicating copy to clipboard operation
notcurses copied to clipboard

an escape sequence broken across multiple read()'s is not handled quite right

Open tstack opened this issue 10 months ago • 3 comments

I think the last change for #2647 is causing problems when I'm testing with WSL/Windows Terminal.

I'm working with Windows 11 24H2 and installed Ubuntu 24.04 WSL. Using Windows Terminal, I start a shell in ubuntu and run my program. Sometimes it works and other times it stalls out. Tracing things through, the DA1 response is getting split across two read() calls. The first call returns 16 bytes with \e[?61;6;7;14;21;. Since the c terminator did not come through, process_escape() returns with -16 and midescape set to 1. But, then process_melange() hits the || *bufused == origlencondition and clears themidescape` and the bytes there are treated as regular input. Since the DA1 response never comes through, initialization stalls out.

The relevant change:

https://github.com/dankamongmen/notcurses/commit/c8882178ed5564e4d59bd94e1db26f89796da822#diff-b7dbcafb0f8345e04cd585f962cdc2d12c9e79b68e13fe6099aeece3b785ce2bL2300-R2301

tstack avatar Feb 03 '25 06:02 tstack

Oops, I forgot to mention that when this occurs, the following assert is raised (don't pay attention to line number, it's in my customized version):

Assertion failed: ictx->amata.used <= buflen (../../../../../../src/third-party/notcurses/src/lib/in.c: process_escape: 2219)

tstack avatar Feb 04 '25 13:02 tstack

I'm also seeing this behaviour with microsoft terminal; I ended up applying this patch:

diff --git a/src/lib/in.c b/src/lib/in.c
index 064b645bc..ab9a23249 100644
--- a/src/lib/in.c
+++ b/src/lib/in.c
@@ -2410,7 +2410,7 @@ process_melange(inputctx* ictx, const unsigned char* buf, int* bufused){
       consumed = process_escape(ictx, buf + offset, *bufused);
       if(consumed < 0){
         if(ictx->midescape){
-          if(*bufused != -consumed || *bufused == origlen){
+          if(*bufused != -consumed || consumed == -1){
             // not at the end; treat it as input. no need to move between
             // buffers; simply ensure we process it as input, and don't mark
             // anything as consumed.

with the assumption that it is very unlikely for the escape sequence to be broken right after the initial ESC. This keeps the old fix for escape not being seen + it causing a busy loop, but also prevents occasional broken output

zhiayang avatar Nov 10 '25 09:11 zhiayang

I ended up making the same || consumed == -1 change.

tstack avatar Nov 11 '25 05:11 tstack