dankamongmen / notcurses

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

properly terminate midsequence when out of input #2646 #2648

Closed dankamongmen closed 2 years ago

dankamongmen commented 2 years ago

We had this test inverted for however long. When we're in the middle of a possible escape sequence, terminate that sequence iff we're out of input. This resolves #2646 on its face, but I want to think about it longer; this is traditionally very complex and finicky code.

dankamongmen commented 2 years ago

I thought the rule was: if you terminate midsequence, and you had a full input buffer, try reading again, and only then call it input (as opposed to an escape). that would handle the case where we'd had a full escape written to us, that happened to be split across our input boundaries. this seems too simple.

dankamongmen commented 2 years ago

yeah this change breaks kitty; running notcurses-input there now shows cruft on startup.

dankamongmen commented 2 years ago

ok, this is much better. gonna test a bit, but i think this gets it.

dankamongmen commented 2 years ago

hrmm this still seems to cruft up kitty, though!

dankamongmen commented 2 years ago

hrmmm so in this situation, we only read 4095? despite providing an 8192B buffer...interesting...

block_on_input:2442:waiting on 1 fds (ibuf: 0/8192)                                                                                 
block_on_input:2469:poll returned 1                                                                                                 
block_on_input:2487:got events: tI                                                                                                  
read_input_nblock:2091:read 4095B from 0 (4097B left)                                                                               
FULLBUFEER: 0 4095 8192                                      <-----------------                                                                         
process_melange:2300:input 0 (0)/4095 [0x1b] ( )                                                                                    
process_escape:2128:initialized automaton to 1                                                                                      
process_escape:2140:walk result on 91 ([): 0 4                                                                                      
process_escape:2140:walk result on 49 (1): 0 24    

well that's lame. so i guess we can't rely on them being written atomically. in that case there's gonna be a fundamental race, right? ugh.

dankamongmen commented 2 years ago

well that's lame. so i guess we can't rely on them being written atomically. in that case there's gonna be a fundamental race, right? ugh.

alright, just make sure there was at least something else there. this fixes it for all situations i can find.

dankamongmen commented 2 years ago

erp, this is causing a segfault when we run notcurses-input < blah in kitty.

dankamongmen commented 2 years ago
==180114== Memcheck, a memory error detector
==180114== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==180114== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==180114== Command: ./notcurses-input
==180114== 
==180114== Thread 2:
==180114== Invalid read of size 1
==180114==    at 0x48871B8: process_escape (in.c:2117)
==180114==    by 0x488A2CE: process_escapes (in.c:2170)
==180114==    by 0x488A2CE: process_ibuf (in.c:2349)
==180114==    by 0x488A2CE: input_thread (in.c:2528)
==180114==    by 0x4CDA5C1: start_thread (in /usr/lib/libc.so.6)
==180114==    by 0x4D5F583: clone (in /usr/lib/libc.so.6)
==180114==  Address 0xffffffff95f292ff is not stack'd, malloc'd or (recently) free'd
==180114== 
==180114== 
==180114== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==180114==    at 0x4CDC34C: __pthread_kill_implementation (in /usr/lib/libc.so.6)
==180114==    by 0x4C8F4B7: raise (in /usr/lib/libc.so.6)
==180114==    by 0x4C8F55F: ??? (in /usr/lib/libc.so.6)
==180114==    by 0x48871B7: process_escape (in.c:2117)
==180114== 
==180114== HEAP SUMMARY:
==180114==     in use at exit: 869,897 bytes in 1,444 blocks
==180114==   total heap usage: 2,291 allocs, 847 frees, 963,649 bytes allocated
==180114== 
==180114== LEAK SUMMARY:
==180114==    definitely lost: 0 bytes in 0 blocks
==180114==    indirectly lost: 0 bytes in 0 blocks
==180114==      possibly lost: 400 bytes in 1 blocks
==180114==    still reachable: 867,481 bytes in 1,422 blocks
==180114==         suppressed: 0 bytes in 0 blocks
==180114== Rerun with --leak-check=full to see details of leaked memory
==180114== 
==180114== For lists of detected and suppressed errors, rerun with: -s
==180114== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
[grimes](0) $ 
dankamongmen commented 2 years ago

ok, i've got one which seems to work across all tests now...