Crash in csi_dispatch #522

Closed
opened 5 months ago by Amanieu · 3 comments
Amanieu commented 5 months ago

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`.
Collaborator

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" ```
Owner

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 added the
bug
label 5 months ago
dnkl self-assigned this 5 months ago
craigbarnes self-assigned this 5 months ago
dnkl was unassigned by craigbarnes 5 months ago
Owner

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`.
craigbarnes closed this issue 5 months ago
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.