Segfault when using wrong number separator

[thexhr]

Hi,

I am running latest git version. Whenever I add a numerical value with "," as separator instead of "." sc-im crashes. If you need more, just let me know.

How to reproduce

Open sc-im and enter =1,1 and I get a segfault immediately.

Stacktrace

By compiling with -g3 i get the following:

(gdb) bt
#0  yyparse () at y.tab.c:3957
#1  0x000003788172dc4a in send_to_interp (oper=0x3788177d410) at cmds.c:1230
#2  0x000003788172db84 in enter_cell_content (r=0, c=0, submode=0x7f7ffffc5600 "let", content=0x7f7ffffc5200) at cmds.c:1205
#3  0x000003788173ab40 in do_insertmode (sb=0x37ad0e35090) at cmds_insert.c:208
#4  0x00000378817507f6 in exec_single_cmd (sb=0x37ad0e35090) at input.c:301
#5  0x00000378817508b7 in exec_mult (buf=0x37ad0e35090, timeout=0) at input.c:360
#6  0x000003788175070e in handle_mult (cmd_multiplier=0x3788177ca04, buf=0x37ad0e35090, timeout=0) at input.c:340
#7  0x0000037881750219 in handle_input (buffer=0x37ad0e35090) at input.c:176
#8  0x000003788175e43e in main (argc=1, argv=0x7f7ffffc6178) at main.c:354
(gdb) l
354                 handle_input(buffer);
355
356             // if we are not in ncurses
357             } else if (fgetws(nocurses_buffer, BUFFERSIZE, f) != NULL) {
358                 sc_debug("Interp will receive: %ls", nocurses_buffer);
359                 send_to_interp(nocurses_buffer);
360             }
361
362             /* shall_quit=1 means :q
363                shall_quit=2 means :q! */

Environment

$ uname -a
OpenBSD sigma.xosc.org 6.8 GENERIC.MP#297 amd64

$ cc --version
OpenBSD clang version 10.0.1 
Target: amd64-unknown-openbsd6.8
Thread model: posix
InstalledDir: /usr/bin

$ lua51 -v        
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio

[poetaman]

@thexhr I don't get segfault on opening sc-im and doing =1,1<CR>. I instead correctly get "syntax error", and value assigned to cell is 1.00 (it drops everything after comma). Am on macOS ARM64 though.

[andmarti1424]

I also get syntax error! and no seg fault.
Edit: and it drops value to 1.00

[andmarti1424]

Anyone had this issue as well? @thexhr could you please run it with gdb and send the stacktrace? thanks

[thexhr]

@andmarti1424 I already included a stacktrace in the first post. Do you need something else?

[andmarti1424]

Yep @thexhr Sorry. I saw this now.
Does this still happens to you?
Really weird. Seems bison/yacc crashing. What is your bison version?
Could you happen to have valgrind installed. Could you execute with it and send its log? It might give me more information. Thanks.

[thexhr]

T.b.H. I cannot reproduce it now since the latest master doesn't compile on OpenBSD :/

ld: error: undefined symbol: init_extended_pair
>>> referenced by tui.c:1497
>>>               tui.o:(ui_start_colors)
cc: error: linker command failed with exit code 1 (use -v to see invocation)
gmake: *** [Makefile:192: sc-im] Error 1

While this is bad for progressing this bug I don't want to derail the issue with compile problems that could be related to OpenBSD's curses lib (?).

[andmarti1424]

Yep. That is because the version of ncurses you have dont have that function defined.. You might need version 6.2 or just replace the two init_extended_color calls on color.c with init_color..
(lines 580 and 642). Will try to autodetect that..

[andmarti1424]

Could you please tell me the version of ncurses you have?

[andmarti1424]

Anyways. I uploaded a change so it try to detect the version of ncurses and use plain init_color if version is prior to 6.1..

[thexhr]

ncurses is part of OpenBSD base system so there is no installed package. Since I were not sure how to find out the version number I looked into /usr/include/ncurses.h and found the following (which is also backed by the information on https://www.openbsd.org/68.html, so it's 5.7):

/* These are defined only in curses.h, and are used for conditional compiles */
#define NCURSES_VERSION_MAJOR 5
#define NCURSES_VERSION_MINOR 7
#define NCURSES_VERSION_PATCH 20081102

/* This is defined in more than one ncurses header, for identification */
#undef  NCURSES_VERSION
#define NCURSES_VERSION "5.7"

I tried compiling latest master c12c527 and it makes no difference. Build still fails with the same error as above.

[andmarti1424]

Yes. Kinda old.. init_extended_color was introduced on v6.1. In my second to last commit I have added a check for that macros NCURSES_VERSION_MAJOR and NCURSES_VERSION_MAJOR.. so that those Init_extended_color gets replaced with init_color.. But for some reason it is not working for you.

[andmarti1424]

If you can help me with this.. I would ask you to replace the init_extended_color with init_color. If that builds ok, then theres a problem in the macro evaluation I added. the one that evaluates NCURSES_VERSION_MAJOR and NCURSES_VERSION_MAJOR. please let me know. thanks.

[thexhr]

I think we always looked at the wrong place. The linker complains about a missing symbol (comment from above). The following changes made it compile successfully. OpenBSD's version of ncurses doesn't contain init_extended_pair so I replaceed it with init_pair.

Since sc-im immediately segfaults when launching, my change is bogus and only here to proof that the code compiles.

 diff --git a/src/tui.c b/src/tui.c
 index d36dbcf..f0a4967 100644
 --- a/src/tui.c
 +++ b/src/tui.c
 @@ -1494,7 +1494,8 @@ void ui_start_colors() {
               * NOTE: calling init_pair with -1 sets it with default
               * terminal foreground and background colors
               */
 -            init_extended_pair( i*def+j+1, i-1, j-1); // i is fg and j is bg
 +            //init_extended_pair( i*def+j+1, i-1, j-1); // i is fg and j is bg
 +            init_pair( i*def+j+1, i-1, j-1); // i is fg and j is bg
          }
      }
  }

I looked at 45326a5 which touched ui_start_colors the last time.

[andmarti1424]

@thexhr
Didnt get it quiet well. Changing init_extended_pair to init_pair permit the building process to complete. But did it get rid of the seg fault with the number separator?
BTW I have just added the NCURSES macro evaluation to init_extended_pair as well. (was only on init_extended_color)..

[thexhr]

Yes, using latest master I can now build successfully. Thanks for the changes!!
No, I didn't change anything regarding the original number separator seg fault.

It now segfaults right upon start due to a ncurses error:

Program terminated with signal 11, Segmentation fault.
Reading symbols from /usr/lib/libpthread.so.26.1...done.
Loaded symbols for /usr/lib/libpthread.so.26.1
Loaded symbols for /home/xhr/Downloads/Software/sc-im/src/sc-im
Reading symbols from /usr/lib/libm.so.10.1...done.
Loaded symbols for /usr/lib/libm.so.10.1
Reading symbols from /usr/lib/libcurses.so.14.0...done.
Loaded symbols for /usr/lib/libcurses.so.14.0
Reading symbols from /usr/local/lib/liblua5.1.so.5.1...done.
Loaded symbols for /usr/local/lib/liblua5.1.so.5.1
Symbols already loaded for /usr/lib/libpthread.so.26.1
Reading symbols from /usr/lib/libc.so.96.0...done.
Loaded symbols for /usr/lib/libc.so.96.0
Reading symbols from /usr/libexec/ld.so...Error while reading shared library symbols:
Dwarf Error: wrong version in compilation unit header (is 4, should be 2) [in module /usr/libexec/ld.so]
#0  0x00000b610903bda4 in __vfprintf (fp=Variable "fp" is not available.
) at /usr/src/lib/libc/stdio/vfprintf.c:111
111             err = __sfvwrite(fp, uio);
(gdb) bt
#1  0x00000b61090bdf65 in vsprintf (str=0x7f7fffbf39b0 "", fmt=Variable "fmt" is not available.                                                                                                                                             
) at /usr/src/lib/libc/stdio/vsprintf.c:56                                                                                                                                                                                                  
#2  0x00000b5ed63382c6 in ui_sc_msg (s=0xb5ed62e8091 "Cannot handle italic attribute. Please update your ncurses library", type=9) at tui.c:245                                                                                             
#3  0x00000b5ed63385e7 in ui_set_ucolor (w=0xb612a3a4b00, uc=0xb5ed6350c94, bg_override=-1) at tui.c:1441                                                                                                                                   
#4  0x00000b5ed633830c in ui_sc_msg (s=0xb5ed62e8091 "Cannot handle italic attribute. Please update your ncurses library", type=9) at tui.c:249
[...]
(gdb) l
1441        sc_error("Cannot handle italic attribute. Please update your ncurses library");
1442    #endif

The last 2 messages in the backtrace about the error in tui.c:249 and tui.c:1441 repeat in a loop.

[andmarti1424]

@thexhr Do you happen to be opening a file? If so, please attach it here. Thanks.

[thexhr]

No, I just launched sc-im.

[andmarti1424]

@thexhr Could you attach your scimrc ? Thanks

[thexhr]

I have none :) I traced the program's execution and saw that it tries to open files and config folders but they're all empty:

 69488 sc-im    CALL  open(0x7f7ffffbb4b0,0<O_RDONLY>)
 69488 sc-im    NAMI  "/home/xhr/.config/sc-im/.scimrc.bak"
 69488 sc-im    RET   open -1 errno 2 No such file or directory
 69488 sc-im    CALL  kbind(0x7f7ffffbb498,24,0x1b2bc8224c7c277d)
 69488 sc-im    RET   kbind 0
 69488 sc-im    CALL  open(0x7f7ffffbb5e0,0<O_RDONLY>)
 69488 sc-im    NAMI  "/home/xhr/.config/sc-im/scimrc"
 69488 sc-im    RET   open -1 errno 2 No such file or directory

[andmarti1424]

Yes. Thats normal. I still dont see whats causing the segfault.

[thexhr]

The error is triggered by the sc_error() in tui.c:1441 and related to the fact that the ncurses version does not support A_ITALIC. I double checked by looking into /usr/include/ncurses.h. When I replace the sc_error with a no-op, sc-im compiles and starts!

 diff --git a/src/tui.c b/src/tui.c
 index d1bc32f..9c3adfb 100644
 --- a/src/tui.c
 +++ b/src/tui.c
 @@ -1434,11 +1434,12 @@ void ui_set_ucolor(WINDOW * w, struct ucolor * uc, int bg_override) {
      short color;
      long attr = A_NORMAL;
      if (uc->bold)      attr |= A_BOLD;
 -
 +#if 0
  #ifdef A_ITALIC
      if (uc->italic)    attr |= A_ITALIC;
  #else
      sc_error("Cannot handle italic attribute. Please update your ncurses library");
 +#endif
  #endif
      if (uc->dim)       attr |= A_DIM;
      if (uc->reverse)   attr |= A_REVERSE;

So I guess you could add some more macro magic and then we should have resolved all curses related compiling errors. Not quite the intention of the original bug report, but IMO a good outcome. Thanks a lot for your help so far, much appreciated!

Since sc-im now starts, I tested the original error trigger (=1,1) and still get a segfault:

Program terminated with signal 11, Segmentation fault.
Reading symbols from /usr/lib/libpthread.so.26.1...done.
Loaded symbols for /usr/lib/libpthread.so.26.1
Loaded symbols for /home/xhr/Downloads/Software/sc-im/src/sc-im
Reading symbols from /usr/lib/libm.so.10.1...done.
Loaded symbols for /usr/lib/libm.so.10.1
Reading symbols from /usr/lib/libcurses.so.14.0...done.
Loaded symbols for /usr/lib/libcurses.so.14.0
Reading symbols from /usr/local/lib/liblua5.1.so.5.1...done.
Loaded symbols for /usr/local/lib/liblua5.1.so.5.1
Symbols already loaded for /usr/lib/libpthread.so.26.1
Reading symbols from /usr/lib/libc.so.96.0...done.
Loaded symbols for /usr/lib/libc.so.96.0
Reading symbols from /usr/libexec/ld.so...Error while reading shared library symbols:
Dwarf Error: wrong version in compilation unit header (is 4, should be 2) [in module /usr/libexec/ld.so]
#0  yyparse () at y.tab.c:4162
4162    y.tab.c: No such file or directory.
        in y.tab.c
(gdb) bt
#0  yyparse () at y.tab.c:4162
#1  0x00000e7c64e821b2 in send_to_interp (oper=0xe7c64ed32b0) at cmds.c:1245
#2  0x00000e7c64e820f4 in enter_cell_content (r=0, c=0, submode=0x7f7ffffd1d80 "let", content=0x7f7ffffd1980) at cmds.c:1220
#3  0x00000e7c64e8faa0 in do_insertmode (sb=0xe7edc4e75a0) at cmds_insert.c:208
#4  0x00000e7c64ead576 in exec_single_cmd (sb=0xe7edc4e75a0) at input.c:311
#5  0x00000e7c64ead637 in exec_mult (buf=0xe7edc4e75a0, timeout=0) at input.c:374
#6  0x00000e7c64ead47e in handle_mult (cmd_multiplier=0xe7c64ed4724, buf=0xe7edc4e75a0, timeout=0) at input.c:350
#7  0x00000e7c64eacf91 in handle_input (buffer=0xe7edc4e75a0) at input.c:186
#8  0x00000e7c64ebb705 in main (argc=1, argv=0x7f7ffffd2928) at main.c:352
Current language:  auto; currently minimal
(gdb) l
352                 handle_input(buffer);
353
354             // if we are not in ncurses
355             } else if (fgetws(nocurses_buffer, BUFFERSIZE, f) != NULL) {
356                 sc_debug("Interp will receive: %ls", nocurses_buffer);
357                 send_to_interp(nocurses_buffer);
358             }
359
360             /* shall_quit=1 means :q
361                shall_quit=2 means :q! */
(gdb) 

[andmarti1424]

I might know the cause of the ncurses issue..
There is a loop in ui_set_ucolor.
If A_ITALIC is not defined (say you have a no compatible ncurses version)..
then it calls sc_error which is a macro that calls ui_set_ucolor!!
I removed those two lines and it might not segfault because of that for you.

The other 1,1 thing, I am completely lost.
Here it validates correctly.

[thexhr]

Thanks for the ui_set_ucolor fix. I can confirm that it fixes the issue I had.

I upgraded my gdb version and now got one additional line in the backtrace regarding the yacc file and some more caller infos.

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000005a7e198b7b6 in yyparse () at y.tab.c:4166
4166    y.tab.c: No such file or directory.
(gdb) bt
#0  0x000005a7e198b7b6 in yyparse () at y.tab.c:4166
#1  0x000005a7e1941102 in send_to_interp (oper=0x5a7e1992320 <interp_line> L"let A0 = 1,1") at cmds.c:1254
#2  0x000005a7e1941044 in enter_cell_content (r=0, c=0, submode=0x7f7ffffe74a0 "let", content=0x7f7ffffe70a0 L"1,1") at cmds.c:1229
#3  0x000005a7e194eb20 in do_insertmode (sb=0x5aab579a640) at cmds_insert.c:208
#4  0x000005a7e1965586 in exec_single_cmd (sb=0x5aab579a640) at input.c:311
#5  0x000005a7e1965647 in exec_mult (buf=0x5aab579a640, timeout=0) at input.c:374
#6  0x000005a7e196548e in handle_mult (cmd_multiplier=0x5a7e1993714 <cmd_multiplier>, buf=0x5aab579a640, timeout=0) at input.c:350
#7  0x000005a7e1964fa1 in handle_input (buffer=0x5aab579a640) at input.c:186
#8  0x000005a7e1973725 in main (argc=1, argv=0x7f7ffffe8048) at main.c:352
(gdb) l
4161    in y.tab.c
(gdb) 

What I could offer is SSH access to one of my OpenBSD machines. Let me know if that's an option for you.

[andmarti1424]

do you happen to have bison or yacc? please attach your gram.c file please.
ssh is may be to much. i dont think i would see a lot more.
btw do you happen to have valgrind?

[thexhr]

I have yacc. It is the version from Robert Paul Corbett for the original 4BSD. I couldn't attach the gram.c file here so I uploaded it to https://xosc.org/misc/gram.c

I have valgrind 3.10.1 installed but it's somehow broken :(

[thexhr]

It's related to OpenBSD's yacc. Compiling with Bison 3.3.2 fixes the segfault and I get the syntax error!

[andmarti1424]

Is there a chance you use bison rather than yacc?

[andmarti1424]

I also might send my gram.c for you to use rather than generating one with yacc

[andmarti1424]

That is then. I knew. Haha

[thexhr]

Thanks a lot for your help @andmarti1424 ! I'll prepare a package for OpenBSD port and add bison as build dependency.

The good result of this issue is that latest master now builds fine one OpenBSD :)

[andmarti1424]

Thanks to you. I will comment this on a wiki just in a case

[thexhr]

One of the OpenBSD senior developers looked at the case and found a bug in the gram.y file. He proposed the following fix and wrote "the issue is a recursive call of yyparse() in the syntax error handling code. Original yacc does not allow that. "

diff --git a/src/gram.y b/src/gram.y
index 873417b..03b1735 100755
--- a/src/gram.y
+++ b/src/gram.y
@@ -814,9 +814,8 @@ command:
     |    error   {
                      sc_error("syntax error");
                      line[0]='\0';
-                     linelim = 0;
-                     yyparse();
                      linelim = -1;
+                     yyclearin;
                      yyerrok;
                  };

He also noted a change of semantics due to his patch: "Slight change in behaviour: after an syntax error, the whole input is discarded instead of doing an attempt to parse it partly (this is what I think the current code tries)"

[andmarti1424]

I am not yacc/bison expert. I changed that and tested it, and didnt find any issues. So Ill commit it.
Did it happen to build it correctly with that change with yacc?

[omoerbeek]

I am not yacc/bison expert. I changed that and tested it, and didnt find any issues. So Ill commit it.
Did it happen to build it correctly with that change with yacc?

Hi, I am the author of the patch. I did test this with both yacc and bison on OpenBSD and saw no differences. But note that originally, entering =1,1 would a report syntax error and fill the cell with 1.0, while with the patch it will report a syntax error but not enter anything into the cell.

[andmarti1424]

hello @omoerbeek. thanks for your patch.
the new behaviour is better than the previous IMO.