dankamongmen / notcurses

blingful character graphics/TUI library. definitely not curses.
https://nick-black.com/dankwiki/index.php/Notcurses
Other
3.6k stars 112 forks source link

Assertion failed: (ictx->amata.used < buflen), functio n process_escape, file in.c, line 2114. #2590

Closed christianparpart closed 2 years ago

christianparpart commented 2 years ago
COLORTERM=truecolor
GNUTERM='sixelgd size 1600,300 truecolor font arial 16'
LANG=en_US.UTF-8
TERM=contour
TERMINAL_NAME=contour
TERMINAL_VERSION_STRING=0.3.0-unreleased-background-iamges-977f3923
TERMINAL_VERSION_TRIPLE=0.3.0
TERMINFO_DIRS=/Applications/contour.app/Contents/Resources/terminfo

latest master 779386abe196e5d0ecff7286d587bfa3f6d8e50b (but i'm experiencing that since some time now)

Contour, latest master and background-images branch.

Now

I think this started to happen as soon as I implemented non-blocking writes to the PTY master (client application's stdin).

This may entirely be my fault but I cannot see any problems in my unit test cases (so far). And then I realized that assert()'ion failure. What can I do to make notcurses happy? Do you actually expect responses to be received across multiple read calls?

Many thanks, Christian.

christianparpart commented 2 years ago

I forgot to mention, I was running on OS/X at that moment. I could not get it to crash on Linux just yet. :)

dankamongmen commented 2 years ago

thanks for the report; i'll look into this asap!

dankamongmen commented 2 years ago

This may entirely be my fault but I cannot see any problems in my unit test cases (so far). And then I realized that assert()'ion failure. What can I do to make notcurses happy? Do you actually expect responses to be received across multiple read calls?

Notcurses ought support a multibyte sequence spread across two read()s, so long as the subsequent elements are immediately available. essentially, i do not require it come in an atomic read(), but i do* expect you to atomically write() it.

the alternative to this is that i hold the first element for an arbitrary amount of time (see NCURSES's ESCDELAY).

christianparpart commented 2 years ago

Ugh, aren't you scared about slow network connections (ssh/telnet/...) where a large packet might be split apart? In that case you won't have control over where exactly the packet is split. This may very well be in the middle of a VT sequence.

i do not require it come in an atomic read(), but i do expect you to atomically write() it.

I am atomically writing whatever is non-blockingly written. if write() can only write N/2 bytes instead of N, then i'll select() until i can write() again. Maybe I'm thinking to networky here. My sole idea was to be interruptible at any time. REgardless of stdout/stdin spam and I assumed non-blocking writes would be best. Hmmmm...

p.s.: my thinking behind always was: what iff the client isn't consuming stdin, not at all, but you're pasting big data in (or whatever). I'll think about it.

dankamongmen commented 2 years ago

finally have a chance to look at this. so are we pretty sure that we're getting an actual break in the escape?

Ugh, aren't you scared about slow network connections (ssh/telnet/...) where a large packet might be split apart? In that case you won't have control over where exactly the packet is split. This may very well be in the middle of a VT sequence.

i am, but have not been seeing this in practice, even once to the best of my knowledge. if the input is not redirected (i.e. is coming from the terminal), it's almost certainly a keyboard, and doesn't generate data rapidly enough for this to be a problem. if it is redirected, i read from the terminal distinctly, so i still wouldn't expect to see it.

the ssh case is interesting. so i guess we're worrying here about e.g.

cat longfile | ssh -t host notcursesprogram

we need to force the TTY allocation, or the notcurses program simply isn't going to start on the remote side. but if we actually try to do this, ssh refuses:

[schwarzgerat](0) $ cat stackless_accepted.pdf | ssh killermike -t notcurses-info
Pseudo-terminal will not be allocated because stdin is not a terminal.
interrogate_terminfo:1288:terminfo error -1 for [] (see terminfo(3ncurses))
[schwarzgerat](1) $ 

ahh, and i can now reproduce your failure:

[schwarzgerat](1) $ ssh killermike -t notcurses-info
notcurses-info: ./src/lib/in.c:2103: process_escape: Assertion `ictx->amata.used < buflen' failed.
Connection to killermike.orthanc closed.
[schwarzgerat](255) $ 

so, in short, yes i am worried about this arising, but the only alternatives seem to be:

although hrmmm at least with the Kitty protocol i can distinguish between a user-originated Escape and part of an escape sequence, i'm pretty sure. you should support the Kitty protocol =]. let me see if XTMODKEYS can similarly resolve this -- i don't think it can, but maybe it can?

dankamongmen commented 2 years ago

so this is failing on say ssh killermike -t notcurses-info because of the gigantic palette report. that assert()ion is certainly an error. at worst, we would deliver some of the info as user-originated input.

i agree, though; that's still not too great. and this will happen whenever we run over ssh, whether we're doing it on a command line like above, or interactively. ok, this is a definite problem.

dankamongmen commented 2 years ago

hrmm, i'm finding this difficult to reproduce with a version built from source:

 ssh killermike -t src/dankamongmen/notcurses/build/notcurses-info 

consistently works as expected. have i already fixed this issue in master, or is it being built some different way? hrmm.

dankamongmen commented 2 years ago

the code around this area has definitely changed. hrmmm, if assert()s are firing, does that mean we're building with CMAKE_BUILD_TYPE=Debug on debian?!

dankamongmen commented 2 years ago

ok yeah we can trigger this when built with CMAKE_BUILD_TYPE=Debug, which leads me to believe that Debian is inserting that into our build definition. either way, let's get this resolved first.

dankamongmen commented 2 years ago

notcurses-info: /home/dank/src/dankamongmen/notcurses/src/lib/in.c:2115: process_escape: Assertionictx->amata.used < buflen' failed.`

it's now on 2115

dankamongmen commented 2 years ago
USED: 0 LEN: 79
USED: 0 LEN: 51
USED: 0 LEN: 23
USED: 23 LEN: 23
notcurses-info: /home/dank/src/dankamongmen/notcurses/src/lib/in.c:2116: process_escape: Assertion `ictx->amata.used < buflen' failed.

do we simply have a bad assert() here? i think that ought be <=.

dankamongmen commented 2 years ago

yep, that seems to fix things up for the intended case where the write actually has no discernible time between arrivals.

@christianparpart , can you still crash it? i don't think you'll be able to, and i don't think you'll be able to make terminal input look like user input without a crafted input. the crash ought definitely be fixed.

christianparpart commented 2 years ago

@dankamongmen Sorry for the late response. I hereby conform that your work is working for me. Many thanks, man. :)