fpjohnston / TECO-64

Enhanced and portable version of TECO text editor in C.
24 stars 6 forks source link

Goto gets NCA error when buffer is empty #21

Open LdBeth opened 3 months ago

LdBeth commented 3 months ago

I found the bug when testing part of squ.tes from TECOC lib, below has a minimal example to reproduce the error.

The conditions needed to reproduce the bug I found are:

In most of the case in squ.tes when the macro is called the buffer wouldn't be empty, but I have tested the same example in TECOC and it has no problem even when the buffer is empty.

As a side note rewrite goto using loop '< >' can work around this issue.

This bug reproduces both on macOS (built with Clang) and Linux (built with GCC).

*eitest.tes``

Delete CR/LF (Y/N) <N>? 12^U?NCA   Negative argument to comma
^D                              ! set decimal !
0ED                             ! set edit mode zero !
0^X                             ! set search mode zero !
0,128ET                         ! set abort-on-error !

! R get Response from user !
@^UR*
^YX0
^YK                             ! kill last inserted string XXX: if move outside macro won't reproduce !
.U1                             ! num 1 <- current point !
ZJ                              ! go to end !
.U2                             ! num 2 <- save buffer end pos !

! display the prompt on a new, clean, line !

!PMPT!

13^T10^T                        ! type <CR><LF> !
        :G0                     !   type str 0 !
        Q2,ZT                    !   and input so far !

!GETCH!
^TU0                            ! read char to num 0 !
Q0-21 "E                        ! if char = ^U !
        Q2,ZK                   !  XXX: when the buffer is empty !
                                !  XXX: the goto would fail by ?NCA !
        @O!PMPT!                !   go to PROMPT !
'                               ! fi !
Q0-27 "E                        ! if char = <ESC> !
        13^T 10^T               !   type <CR> <LF> !
        @O!DONE!                !   go to DONE XXX: if input is empty this would also fail !
'                               ! fi !
Q0@I//                          ! insert input char to buffer !
@O!GETCH!                       ! go to GETCH !

!DONE!

Q1J                             ! restore to original buffer position !
*                               ! end of ^UR !

@I+Delete CR/LF (Y/N) <N>? +    ! str 0 <- display prompt !

MR
HT
^[^[
LdBeth commented 3 months ago

Ok I have did some debug with lldb and found the NCA is caused by confirm_cmd() in cmd_exec.c and although I do not fully understand how the command syntax check works, I think the problem is when go to is used inside a macro TECO-64 scans the text before the label and thinks ^Y gives a negative range for the current buffer is empty when it it actually valid syntax and won't be executed anyway. I guess some of the program logic is not fully correct there, but I believe it is not difficult to fix by adding a different check option when looking for go to labels.

LdBeth commented 3 months ago

So the good news is by rebuild TECO-64 with nstrict=1, I can get rid of the NCA error and I have changed enough of squ.tes to have it handle alternative style comment and paired delimiter (only works for ^U), but that's enough to bootstrap the modified the version of squ.tes, that is make it squish itself and the squished version works identically to unquished itself. And I think filling the rest of the gap in the language (ignore space when looking for delimiter, handle ! in enclosed parentheses, etc) is not that difficult.

Here is a tiny benchmark:

$ time teco -T 'test.tec=sqush.tes/D:Y/B:Y/C:+ :/T:Y/E:Y/P/A:Y' -E sqush.tec
Squishing "/Users/ldbeth/sqush/sqush.tes
Superseding existing file 'test.tec'
Creating "/Users/ldbeth/sqush/test.tec
    0m04.13s real     0m04.13s user     0m00.00s system
$ time teco -T 'test.tec=sqush.tes/D:Y/B:Y/C:+ :/T:Y/E:Y/P/A:Y' -E sqush.tes

Squishing "/Users/ldbeth/sqush/sqush.tes
Superseding existing file 'test.tec'
Creating "/Users/ldbeth/sqush/test.tec
    0m30.61s real     0m30.59s user     0m00.00s system
$ ls -lh sqush.*
-rw-r--r--@ 1 ldbeth  staff   4.8K Feb 11 14:46 sqush.tec
-rw-r--r--  1 ldbeth  staff    88K Feb 11 14:44 sqush.tes
LdBeth commented 3 months ago

So now I have understood how the confirm_cmd works, I found the cause is pretty obvious:

The goto command uses skip_cmd() to scan for labels, which calls scan_cmd() and dispatches to scan_*() functions to consume command arguments, but for many commands, the confirm assertion has been put to the scan_*() part instead of exec_*() part, the X is one of them.

The fix is then very obvious, moving all the confirm checks to the negativity of the command to the exec_*() function.