SamCoVT / TaliForth2

A Subroutine Threaded Code (STC) ANSI-like Forth for the 65c02
Other
28 stars 4 forks source link

c65 ENTER does not repeat previous command #100

Closed SamCoVT closed 1 month ago

SamCoVT commented 2 months ago

On Linux (I haven't tested on any other platform yet), pressing ENTER does not repeat the next command. Here's an example run with n and then ENTER twice:

PC f02a  nv-bdIZc  A 00 X 76 Y 00 SP f9 > n
kernel_getc:
*  f027  ad 04 f0    lda  io_getc
PC f027  nv-bdIZc  A 00 X 76 Y 00 SP f9 > 
unbalanced parentheses
    (BZ
-------^
kernel_getc:
*  f027  ad 04 f0    lda  io_getc
PC f027  nv-bdIZc  A 00 X 76 Y 00 SP f9 > 
unexpected

----^

It looks like there is garbage in the string being processed. I don't see that (BZ string anywhere on my screen. I also tried ^M and ^J in case it was a line ending issue, but it responds to both of those as an ENTER (that's nice, at least) and with the same behavior noted above.

patricksurry commented 2 months ago

Weird! I can't reproduce on OS X, is this in WSL? When I start with c65/c65 -r taliforth-c65.bin -g and do n, then all of enter, ^J and ^M repeat correctly for me.

It still smells somehow like a line-ending issue to me. I wonder if it's carrying over the line-ending character(s) into the command string and not detecting it as empty? If you want to try a little experiment I'd be curious how it behaves if you add two lines just before the if here:

printf("len %d\n", strlen(line));
line[strcspn(line, "\r\n")] = 0;

if(strlen(line)) {
...
SamCoVT commented 2 months ago

This is on native linux. It appears the length is zero (I got a warning about strlen returning a size_t, and how %d should perhaps be %ld - it made me chuckle, but it should still work for this application). Looking at xt_drop here for reference, pressing n to proceed over the underflow check and then pressing enter a couple of times to try to keep using the n command, then typing the full word next and enter after that. The garbage changes a bit.

xt_drop:
*B 869a  20 8a d7    jsr  underflow_1
PC 869a  nv-bdIzc  A 86 X 76 Y 01 SP f9 > n
len 1
*  869d  e8          inx  
PC 869d  Nv-bdIzc  A 86 X 76 Y 01 SP f9 > 
len 0
unexpected
    =��U
----^
*  869d  e8          inx  
PC 869d  Nv-bdIzc  A 86 X 76 Y 01 SP f9 > 
len 0
unexpected
    =��U
----^
*  869d  e8          inx  
PC 869d  Nv-bdIzc  A 86 X 76 Y 01 SP f9 > 
PC 869d  Nv-bdIzc  A 86 X 76 Y 01 SP f9 > next
len 4
*  869e  e8          inx  
PC 869e  nv-bdIzc  A 86 X 77 Y 01 SP f9 > 
len 0
unexpected
    �U
----^
*  869e  e8          inx  
PC 869e  nv-bdIzc  A 86 X 77 Y 01 SP f9 > 
SamCoVT commented 2 months ago

TLDR It looks like a "pointer use after free" bug.

The "unexpected" error message appears to come from show_error() when passed EX_EXTRA and I can only see that being handled in parse_end(). I did double check with a quick throwaway program and verify that isblank(0) returns 0 (it should, and it does).

I do see that monitor_command() calls free(line), but then that line is trying to be accessed again when you call _repeat_cmd->handler()?

cmd_next() (from calling the repeated handler via pointer) calls _cmd_single(STEP_NEXT) which calls parse_end() (if/when parse_int() fails) and now you are using a pointer into line, which has already been freed once the previous n/next command completed.

patricksurry commented 2 months ago

aha, yes, that's dumb on my part. i refactored the parsing stuff without thinking about the repeating case. It relies on parse_start() getting called which isn't happening correctly in the repeat case, and will leave a cursor pointing at the previously freed string. Kind of surprising it works at all for me, but I guess depends on how low-level allocation behaves. Should be easy to fix.