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