simon-git: putty (master): Simon Tatham

Commits to Tartarus hosted VCS tartarus-commits at lists.tartarus.org
Tue Jul 23 20:02:59 BST 2019


TL;DR:
  8872a97e gtkask.c: use dedicated PRNG for initial area choice.
  061ca8d8 ssh2userauth: be more careful about s->ki_scc being NULL.
  0716880f stripctrl: clean up precarious handling of 'width'.
  c713ce48 terminal.[ch]: refactor the 'pos' macros.
  80f5a009 Bounds-check terminal selection when clearing scrollback.

Repository:     https://git.tartarus.org/simon/putty.git
On the web:     https://git.tartarus.org/?p=simon/putty.git
Branch updated: master
Committer:      Simon Tatham <anakin at pobox.com>
Date:           2019-07-23 20:02:59

commit 8872a97ebd81c9b1b649453664ee8a2dea9ceb2d
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=8872a97ebd81c9b1b649453664ee8a2dea9ceb2d;hp=37bff968ebf98de0b5825ce3f482b5cf1163ea32
Author: Simon Tatham <anakin at pobox.com>
Date:   Tue Jul 23 18:40:09 2019 +0100

    gtkask.c: use dedicated PRNG for initial area choice.
    
    At Coverity's urging, I put in an instance of the Fortuna PRNG in
    place of the use of rand() to select which drawing area to light up
    next on a keypress. But Coverity turns out to be unhappy that I'm
    still using rand() to select the _initial_ area, before the user
    enters any input at all.
    
    It's even harder to imagine this being a genuine information leak than
    the previous complaint. But I don't really see a reason _not_ to
    switch it over, now it's been pointed out.

 unix/gtkask.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

commit 061ca8d8449e9b96ca684a60f509f143fd1111bc
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=061ca8d8449e9b96ca684a60f509f143fd1111bc;hp=8872a97ebd81c9b1b649453664ee8a2dea9ceb2d
Author: Simon Tatham <anakin at pobox.com>
Date:   Tue Jul 23 18:51:59 2019 +0100

    ssh2userauth: be more careful about s->ki_scc being NULL.
    
    Coverity points out that we're inconsistent about checking it for
    NULL: seat_stripctrl_new() *can* return NULL, so in principle, we can
    go through the initialisation code for s->ki_scc and have it still be
    NULL afterwards. I check that in the code that uses it to sanitise the
    prompt strings out of USERAUTH_INFO_REQUEST, but not in the code that
    sanitises the name or instruction strings. Now all uses are checked in
    the same way.
    
    This is only a latent bug, because the four main Seat implementations
    (GUI and console, on Windows and Unix) never return NULL. The only
    implementation of seat_stripctrl_new which _can_ return NULL is the
    trivial nullseat_stripctrl_new, currently only used by Uppity.
    
    However, of course, if that changes in future, this latent bug could
    turn into a real one, so better to fix it before that happens. Thanks
    Coverity.

 ssh2userauth.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

commit 0716880f68960309371c9807bfda02ad0d92c41c
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=0716880f68960309371c9807bfda02ad0d92c41c;hp=061ca8d8449e9b96ca684a60f509f143fd1111bc
Author: Simon Tatham <anakin at pobox.com>
Date:   Tue Jul 23 18:58:45 2019 +0100

    stripctrl: clean up precarious handling of 'width'.
    
    In both stripctrl_locale_put_wc() and stripctrl_term_put_wc(), we get
    a character width from either wcwidth or term_char_width, which might
    be -1 for a control character. Coverity complained that that width
    value is later passed to to stripctrl_check_line_limit, which takes a
    size_t parameter, i.e. it expects the width to be non-negative.
    
    This is another bug that doesn't actually cause damage, but the
    reasons why are quite fiddly:
    
     - The only width<0 characters we let through are those that got
       through stripctrl_ctrlchar_ok. (At both call sites, if that
       function is unhappy then we either take an early return or else
       replace the character _and_ the value of 'width' with a
       substitution.)
    
     - stripctrl_ctrlchar_ok only lets through \n, or \r if permit_cr is
       set.
    
     - stripctrl_check_line_limit ignores its width parameter if line
       limiting is turned off, or if it gets \n.
    
     - So the only way this can cause a problem is if permit_cr is set so
       that \r can get through ctrlchar_ok, *and* line limiting is turned
       on so that we get far enough through check_line_limit to choke on
       its negative width.
    
     - But in fact all the call sites that ever call
       stripctrl_enable_line_limiting do it with a StripCtrlChars that
       they got from a seat, and all the nontrivial implementations of
       seat_stripctrl_new pass false for permit_cr.
    
    So the combination of circumstances in which -1 gets implicitly
    converted to size_t *and* stripctrl_check_line_limit actually pays
    attention to it can never arise. But it's clearly foolish to make the
    correctness of the code depend on a proof that long - now Coverity has
    pointed it out, I'm not happy with it either! Fixed by substituting 0
    for the width in the questionable cases.

 stripctrl.c | 5 +++++
 1 file changed, 5 insertions(+)

commit c713ce4868bde0283c348dbc0f138dc3b93517a8
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=c713ce4868bde0283c348dbc0f138dc3b93517a8;hp=0716880f68960309371c9807bfda02ad0d92c41c
Author: Simon Tatham <anakin at pobox.com>
Date:   Tue Jul 23 19:16:36 2019 +0100

    terminal.[ch]: refactor the 'pos' macros.
    
    These are now inline functions (mostly, except for a couple of macro
    wrappers to preserve the old call syntax), and they live in terminal.h
    instead of at the top of terminal.c.
    
    Also, I've added a few comments for clarity, and renamed the posPlt()
    function, which didn't do at all what the name made it look (at least
    to a mathematician familiar with product orders) as if it did.

 terminal.c | 18 +------------
 terminal.h | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 17 deletions(-)

commit 80f5a009f647aacd492c6e1e7a5f450156cabe13
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=80f5a009f647aacd492c6e1e7a5f450156cabe13;hp=c713ce4868bde0283c348dbc0f138dc3b93517a8
Author: Simon Tatham <anakin at pobox.com>
Date:   Tue Jul 23 19:24:10 2019 +0100

    Bounds-check terminal selection when clearing scrollback.
    
    term_clrsb() was emptying the tree234 of scrollback, without checking
    whether term->selstart, term->selend and term->selanchor were pointing
    at places in the now-removed scrollback. If they were, then a
    subsequent extend-selection operation could give rise to the dreaded
    'line==NULL' assertion box.
    
    Thanks to the user who sent in one of those debugging dumps, that
    finally enabled us to track down (at least one case of) this
    long- standing but extremely rare crash!

 terminal.c | 23 +++++++++++++++++++++++
 terminal.h |  5 +++++
 2 files changed, 28 insertions(+)



More information about the tartarus-commits mailing list