search

martanne / dvtm

dvtm brings the concept of tiling window management, popularized by X11-window managers like dwm to the console. As a console window manager it tries to make it easy to work with multiple console based programs.
MIT License
843 stars 108 forks source link

Ensure cursor is clamped after buffer swap #124

Open phillid opened 3 years ago

phillid commented 3 years ago

vt_resize resizes both buffers of the given Vt* (involving a realloc), but can only correctly clamp the cursor of the active buffer. This means that when it comes time to switch to the other buffer in interpret_csi_priv_mode, we might be switching to a buffer which has a cursor pointing to old memory. Thus, when we switch buffers it's necessary to ensure the cursor is clamped to avoid memory errors.

This is a bug I've observed for a few years but never often enough to worry me. After I was able to pin it down to activities such as opening of manpages and resizing terminals, I boiled it down to be reproducible as:

  1. Open a manpage in dvtm, buffer swap to alt
  2. Close the manpage and return to the shell, buffer swaps to norm
  3. Resize the pane to have fewer rows than before, alt+norm are resized but only norm has its cursor clamped
  4. Open a manpage again, UAF causes crash since unclamped curs_row on alt buffer is still pointing to before-resize allocation

With some exploratory testing I have seen crashes identical and nearly identical to the following fixed by this patch:

phillid commented 3 years ago

Marking this as draft until I've had some more time using this patch as daily driver to ensure there aren't regressions, but I'd appreciate if review/approval could be given in the meantime :grinning:

luke-clifton commented 3 years ago

This is likely the bug that has been bothering me for a while. Occasionally dvtm crashes when using man or less. Sometimes my OS gives me a warning about a "use after free", but usually it just dies.

I'll be running this patch as well. Thank you for finding it!

phillid commented 3 years ago

Marking this as ready. I've run this as my daily at home and at work and haven't seen any issues.

@martanne if you get a chance to look at this that'd be great :+1:

rpmohn commented 3 years ago

Looks like a great bug fix, I will also test it out!

phillid commented 2 years ago

@martanne I forgot about this MR - have you had a chance to take a look at it by any chance?

artemkobets commented 2 years ago

Works great so far. Thanks for investigating this issue! It's been annoying me for a while, glad someone's already done research on it.