Skip to content

Commit

Permalink
prevent use-after-free of viewport_bline when reusing a bview.
Browse files Browse the repository at this point in the history
this bug surfaced as a blank display after `C-q p` or `C-q o`
commands, both of which involve replacing the buffer of an existing
bview. `git bisect` pointed to commit dd28398. stepping through
`buffer_get_bline_w_hint` revealed that we were using a freed bline
as our `opt_hint`, which caused that function to always return
`last_line`, which in turn broke a bunch of stuff. so the bug existed
before this commit, but it was silent. reproducing through valgrind
yielded the following:

```
Invalid read of size 8
   at 0x1375D6: buffer_get_bline_w_hint (buffer.c:715)
   by 0x11B93A: bview_rectify_viewport (bview.c:388)
   by 0x11BE08: bview_resize (bview.c:147)
   by 0x11C41B: bview_open (bview.c:67)
   by 0x1417E5: _cmd_fsearch_inner (cmd.c:1805)
   by 0x13C8A9: cmd_fsearch (cmd.c:605)
   by 0x1238E6: _editor_loop (editor.c:927)
   by 0x11CE51: editor_run (editor.c:153)
   by 0x12EDE0: main (main.c:20)
 Address 0x5257130 is 32 bytes inside a block of size 128 free'd
   at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/...
   by 0x13969F: _buffer_bline_free (buffer.c:1493)
   by 0x135C68: buffer_destroy (buffer.c:205)
   by 0x11B3E4: _bview_deinit (bview.c:599)
   by 0x11C2B8: _bview_init (bview.c:451)
   by 0x11C2B8: bview_open (bview.c:66)
   by 0x1417E5: _cmd_fsearch_inner (cmd.c:1805)
   by 0x13C8A9: cmd_fsearch (cmd.c:605)
   by 0x1238E6: _editor_loop (editor.c:927)
   by 0x11CE51: editor_run (editor.c:153)
   by 0x12EDE0: main (main.c:20)
```

unsetting `viewport_bline` in `_bview_deinit` fixes the bug. other
objects like cursors, marks, srules, etc. which hold bline pointers
are already deinit'd in this function, so we should be covered there.

this is another case where smart pointers might have been useful.

it's hard to write a test for this one as it involves viewport code which
is not executed in headless mode (which tests run in).
  • Loading branch information
adsr committed Jan 9, 2021
1 parent 38d679d commit 8fb367d
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 0 deletions.
5 changes: 5 additions & 0 deletions bview.c
Original file line number Diff line number Diff line change
Expand Up @@ -598,8 +598,13 @@ static void _bview_deinit(bview_t *self) {
if (self->buffer->ref_count < 1) {
buffer_destroy(self->buffer);
}
self->buffer = NULL;
}

// Unset viewport_bline as it may point to a freed bline at this point if
// we are re-using a bview, e.g., after cmd_open_replace_file.
self->viewport_bline = NULL;

// Free last_search
if (self->last_search) {
free(self->last_search);
Expand Down
1 change: 1 addition & 0 deletions tests/func/test_browse.sh
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ expected[hello]='^bview.[[:digit:]]+.buffer.path=./hello$'
source "$this_dir/test.sh"

popd &>/dev/null
pushed=0

0 comments on commit 8fb367d

Please sign in to comment.