Crash in csi_dispatch #522

Amanieu commented 2021年05月16日 14:04:02 +02:00

I noticed this when accidentally cating an executable file.

Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007efdbd8bb687 in __memmove_avx_unaligned_erms () from /usr/lib/libc.so.6
#1 0x000055a4c71922a9 in csi_dispatch (term=0x55a4c8c276f0, final=<optimized out>) at ../csi.c:1065
#2 0x000055a4c71902f0 in action_csi_dispatch (c=<optimized out>, term=0x55a4c8c276f0) at ../vt.c:499
#3 state_csi_param_switch (data=<optimized out>, term=0x55a4c8c276f0) at ../vt.c:887
#4 vt_from_slave (term=term@entry=0x55a4c8c276f0, data=data@entry=0x7ffde003fab0 "234円067円", len=<optimized out>) at ../vt.c:1144
#5 0x000055a4c7183981 in fdm_ptmx (fdm=0x55a4c8bb5a60, fd=6, events=<optimized out>, data=0x55a4c8c276f0) at ../terminal.c:255
#6 0x000055a4c716327b in fdm_poll (fdm=fdm@entry=0x55a4c8bb5a60) at ../fdm.c:459
#7 0x000055a4c715a1f7 in main (argc=<optimized out>, argv=<optimized out>) at ../main.c:535

Here is the relevant code in csi_dispatch:

 case 'P': {
 /* DCH: Delete character(s) */
 /* Number of characters to delete */
 int count = min(
 vt_param_get(term, 0, 1), term->cols - term->grid->cursor.point.col);
 /* Number of characters left after deletion (on current line) */
 int remaining = term->cols - (term->grid->cursor.point.col + count);
 /* 'Delete' characters by moving the remaining ones */
 memmove(&term->grid->cur_row->cells[term->grid->cursor.point.col],
 &term->grid->cur_row->cells[term->grid->cursor.point.col + count],
 remaining * sizeof(term->grid->cur_row->cells[0]));
 for (size_t c = 0; c < remaining; c++)
 term->grid->cur_row->cells[term->grid->cursor.point.col + c].attrs.clean = 0;
 term->grid->cur_row->dirty = true;
 /* Erase the remainder of the line */
 term_erase(
 term,
 &(struct coord){term->grid->cursor.point.col + remaining, term->grid->cursor.point.row},
 &(struct coord){term->cols - 1, term->grid->cursor.point.row});
 term->grid->cursor.lcf = false;
 break;
 }

The crash happens because count is -966033864: min treats its inputs as signed numbers and vt_param_get(term, 0, 1) returned -966033864.

I noticed this when accidentally `cat`ing an executable file. ``` Program terminated with signal SIGSEGV, Segmentation fault. #0 0x00007efdbd8bb687 in __memmove_avx_unaligned_erms () from /usr/lib/libc.so.6 #1 0x000055a4c71922a9 in csi_dispatch (term=0x55a4c8c276f0, final=<optimized out>) at ../csi.c:1065 #2 0x000055a4c71902f0 in action_csi_dispatch (c=<optimized out>, term=0x55a4c8c276f0) at ../vt.c:499 #3 state_csi_param_switch (data=<optimized out>, term=0x55a4c8c276f0) at ../vt.c:887 #4 vt_from_slave (term=term@entry=0x55a4c8c276f0, data=data@entry=0x7ffde003fab0 "234円067円", len=<optimized out>) at ../vt.c:1144 #5 0x000055a4c7183981 in fdm_ptmx (fdm=0x55a4c8bb5a60, fd=6, events=<optimized out>, data=0x55a4c8c276f0) at ../terminal.c:255 #6 0x000055a4c716327b in fdm_poll (fdm=fdm@entry=0x55a4c8bb5a60) at ../fdm.c:459 #7 0x000055a4c715a1f7 in main (argc=<optimized out>, argv=<optimized out>) at ../main.c:535 ``` Here is the relevant code in `csi_dispatch`: ```c case 'P': { /* DCH: Delete character(s) */ /* Number of characters to delete */ int count = min( vt_param_get(term, 0, 1), term->cols - term->grid->cursor.point.col); /* Number of characters left after deletion (on current line) */ int remaining = term->cols - (term->grid->cursor.point.col + count); /* 'Delete' characters by moving the remaining ones */ memmove(&term->grid->cur_row->cells[term->grid->cursor.point.col], &term->grid->cur_row->cells[term->grid->cursor.point.col + count], remaining * sizeof(term->grid->cur_row->cells[0])); for (size_t c = 0; c < remaining; c++) term->grid->cur_row->cells[term->grid->cursor.point.col + c].attrs.clean = 0; term->grid->cur_row->dirty = true; /* Erase the remainder of the line */ term_erase( term, &(struct coord){term->grid->cursor.point.col + remaining, term->grid->cursor.point.row}, &(struct coord){term->cols - 1, term->grid->cursor.point.row}); term->grid->cursor.lcf = false; break; } ``` The crash happens because `count` is `-966033864`: `min` treats its inputs as signed numbers and `vt_param_get(term, 0, 1)` returned `-966033864`.

craigbarnes commented 2021年05月16日 15:12:12 +02:00

It looks like the negative value is coming from the implicit unsigned -> int conversion in vt_param_get(). All the uses of int in that function should probably be changed to unsigned, since that's what type vt_param::value is and valid CSI params can't be negative.

Reproducer:

printf "033円[4294967295P"

It looks like the negative value is coming from the implicit `unsigned` -> `int` conversion in `vt_param_get()`. All the uses of `int` in that function should probably be changed to `unsigned`, since that's what type `vt_param::value` is and valid CSI params can't be negative. Reproducer: ```sh printf "033円[4294967295P" ```

dnkl commented 2021年05月16日 15:50:58 +02:00

All the uses of int in that function should probably be changed to unsigned, since that's what type vt_param::value is and valid CSI params can't be negative.

@craigbarnes I agree competely. This is pretty bad design...

But, it appears all uses of vt_param_get() assigns to signed int. This of course needs to be changed as well.

There may be places where negative values are valid intermediate values.

> All the uses of int in that function should probably be changed to unsigned, since that's what type vt_param::value is and valid CSI params can't be negative. @craigbarnes I agree competely. This is pretty bad design... But, it appears all **uses** of `vt_param_get()` assigns to `signed int`. This of course needs to be changed as well. There _may_ be places where negative values are valid intermediate values.

dnkl commented 2021年05月16日 18:31:44 +02:00

Change of plan; we regularly do signed arithmetic, meaning we want vt_param_get() to return an int.

What we'll do instead is limit parameter values to INT_MAX.

Change of plan; we regularly do signed arithmetic, meaning we _want_ `vt_param_get()` to return an `int`. What we'll do instead is limit parameter values to `INT_MAX`.