UBsan pointed out another memcpy from NULL (again with length 0) in
the prompts_t system. When I looked at it, I realised that firstly
prompt_ensure_result_size was an early not-so-good implementation of
sgrowarray_nm that would benefit from being replaced with a call to
the real one, and secondly, the whole system for storing prompt
results should really have been replaced with strbufs with the no-move
option, because that's doing all the same jobs better.
So, now each prompt_t holds a strbuf in place of its previous manually
managed string. prompt_ensure_result_size is gone (the console
prompt-reading functions use strbuf_append, and everything else just
adds to the strbuf in the usual marshal.c way). New functions exist to
retrieve a prompt_t's result, either by reference or copied.
(cherry picked from commit cd6bc14f04)
These are better than my previous approach of just assigning to
sb->len, because firstly they check by assertion that the new length
is within range, and secondly they preserve the invariant that the
byte stored in the buffer just after the length runs out is \0.
Switched to using the new functions everywhere a grep could turn up
opportunities.
(cherry picked from commit 5891142aee)
A user reports that if the ^E answerback string is configured to be
empty, then causing the answerback to be sent fails the assertion in
ldisc_send introduced in commit c269dd013.
I thought I'd caught all of the remaining cases of this in commit
4634cd47f, but apparently not.
(cherry picked from commit 43a63019f5)
If the user is scrolled back in the scrollback when a screen-swap
takes place, and if we're not configured to reset the scrollback
completely on the grounds that the swap is display activity, then we
should do the same thing we do for other kinds of display activity:
strive to keep the scroll position pointing at the same text. In this
case, that means adjusting term->disptop by the number of virtual
lines added to the scrollback to allow the main screen to be viewed
while the alt screen is active.
This improves the quality of behaviour in that corner case, but more
importantly, it should also fix a case of the dreaded line==NULL
assertion failure, which someone just reported against 0.73 when
exiting tmux (hence, switching away from the alt screen) while
scrolled back in a purely virtual scrollback buffer: the virtual
scrollback lines vanished, but disptop was still set to a negative
value, which made it out of range.
(cherry picked from commit 22453b46da)
A long time ago, in commit 09f86ce7e, I introduced a separate copy of
the saved cursor position (used by the ESC 7 / ESC 8 sequences) for
the main and alternate screens. The idea was to fix mishandling of an
input sequence of the form
ESC 7 (save cursor)
ESC [?47h (switch to alternate screen)
...
ESC 7 ESC 8 (save and restore cursor, while in alternate screen)
...
ESC [?47l (switch back from alternate screen)
ESC 8 (restore cursor, expecting it to match the _first_ ESC 7)
in which, before the fix, the second ESC 7 would overwrite the
position saved by the first one. So the final ESC 8 would restore the
cursor position to wherever it happened to have been saved in the
alternate screen, instead of where it was saved before switching _to_
the alternate screen.
I've recently noticed that the same bug still happens if you use the
alternative escape sequences ESC[?1047h and ESC[?1047l to switch to
the alternate screen, instead of ESC[?47h and ESC[?47l. This is
because that version of the escape sequence sets the internal flag
'keep_cur_pos' in the call to swap_screen, whose job is to arrange
that the actual cursor position doesn't change at the instant of the
switch. But the code that swaps the _saved_ cursor position in and out
is also conditioned on keep_cur_pos, so the 1047 variant of the
screen-swap sequence was bypassing that too, and behaving as if there
was just a single saved cursor position inside and outside the
alternate screen.
I don't know why I did it that way in 2006. It could have been
deliberate for some reason, or it could just have been mindless copy
and paste from the existing cursor-related swap code. But checking
with xterm now, it definitely seems to be wrong: the 1047 screen swap
preserves the _actual_ cursor position across the swap, but still has
independent _saved_ cursor positions in the two screens. So now PuTTY
does the same.
(cherry picked from commit 421a8ca5d9)
The redesign in commit 9fccb065a arranged that all keystroke data goes
via term_keyinput_internal, which calls term_bracketed_paste_stop just
in case the keystroke had interrupted an in-progress paste.
But, embarrassingly, I forgot that _pasted_ data also goes via
term_keyinput_internal, and bracketed paste mode certainly should not
be terminated before _that_ is sent! I should have conditionalised the
call to term_bracketed_paste_stop on the 'interactive' flag parameter,
which is precisely there to tell the difference between pastes and
true keyboard input.
The number of people has been steadily increasing who read our source
code with an editor that thinks tab stops are 4 spaces apart, as
opposed to the traditional tty-derived 8 that the PuTTY code expects.
So I've been wondering for ages about just fixing it, and switching to
a spaces-only policy throughout the code. And I recently found out
about 'git blame -w', which should make this change not too disruptive
for the purposes of source-control archaeology; so perhaps now is the
time.
While I'm at it, I've also taken the opportunity to remove all the
trailing spaces from source lines (on the basis that git dislikes
them, and is the only thing that seems to have a strong opinion one
way or the other).
Apologies to anyone downstream of this code who has complicated patch
sets to rebase past this change. I don't intend it to be needed again.
If the cursor is on the rightmost column of the terminal and
term->wrapnext is set, and the user asks to erase from the current
position to the end of (at least) the line, what should happen?
PuTTY's previous behaviour was to ignore term->wrapnext, and do the
same thing we would have done without it: erase from the current
physical cursor position to EOL inclusive, i.e. blank the character
cell we just printed.
But this is unfortunate if a program writes an interleaving of
printing characters and ESC[K, which I recently found out is what gcc
does in its colour-highlighted error messages: if the last printed
char just before an ESC[K pushes the cursor into the deferred-wrap
state, then the ESC[K blanks that character, and then we wrap to the
next line. So one character of the error message ends up missing.
xfce4-terminal and gnome-terminal take the approach in this situation
of regarding the cursor position as _right_ at the end of the line, so
no character cells get cleared at all, and the error message displays
as intended. I think that's more sensible, so I've switched to doing
the same thing.
(xterm has different behaviour again: it blanks the character cell and
also clears its analogue of the wrapnext flag. So in _their_ handling
of this sequence of output, one character of the error message is
still missing, but it looks as if it's been _omitted_ rather than
replaced by a space.)
Secondly, in the course of fixing that, I looked at the check_boundary
call in erase_lots, which is supposed to ensure that if a wide CJK
character straddles the boundary between what's being erased and what
isn't, then both halves of the character are deleted. I had to modify
that anyway because I was moving that very boundary, and in doing so,
I noticed that even according to the previous behaviour, it had an
off-by-one error. In the case where you send ESC[1K (meaning erase up
to and including the cursor position), the call to check_boundary was
performed on the _left_ edge of the cursor's character cell, when it
should have been the right edge. So you could end up with an
erase_char in the left half (i.e. a space) and still have the magic
value UCSWIDE in the right half, causing the terminal to think you had
a double-width U+0020 on the screen, which isn't supposed to be able
to happen.
Any terminal event that modifies the range of screen+scrollback
currently covered by the mouse selection should call deselect(), so
that users won't be misled into thinking that the new version of the
highlighted text would be pasted, and also so that using the
MBT_EXTEND action to modify the selection bounds won't start from
a selection you never had in the first place.
Clearing the scrollback is such an event, if the selection covered any
of the scrollback at all; so we should apply the same policy here as
everywhere else.
This reverts commit 80f5a009f6.
After a bit more thought, I've decided it's the wrong way to solve the
problem. We shouldn't really be _changing_ the current selection
bounds in response to an event that touches the range they cover. With
this fix in place, if you clear the scrollback while a selection
partly overlaps it, and then extend the modified selection, you'll get
a selection one of whose endpoints is something you never specified as
a selection endpoint at all, and possibly paste the wrong text.
A better fix is to do the same thing we do about any other event that
touches the range covered by the selection: get rid of the selection
completely. For ease of cherry-picking (in case anyone needs to apply
the good fix in some downstream branch, or whatever), I'll make that
change separately in the next commit.
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!
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.
Commit 71e42b04a's refactoring of terminal keyboard input, in the case
where a Unicode string derived from a keystroke is translated into
some other charset to put on the wire, had allocated the output buffer
for that translation one byte too small.
Having decided that the terminal's local echo setting shouldn't be
allowed to propagate through to termios, I think the local edit
setting shouldn't either. Also, no other terminal emulator I know
seems to implement this sequence, and if you enable it, things get
very confused in general. I think it's generally better off absent; if
somebody turns out to have been using it, then we'll at least be able
to find out what it's good for.
This sequence (ESC[12l, ESC[12h) enables and disables local echo in
the terminal. We were previously implementing it by gatewaying it
directly through to the local echo facility in the line discipline,
which in turn would pass it on to the terminal it was running in (if
it was Plink).
This seems to be at odds with how other terminals do it: they treat
SRM as its own entirely separate thing, in which the terminal
_emulator_ performs its own echoing of input keypress data,
independently of whether the Unix terminal device (or closest
equivalent) is doing the same thing or not.
Now we're doing it the same way as everyone else (or at least I think
so): the new internal terminal function that the term_keyinput pair
feed to is also implementing SRM-driven local echo as another of its
side effects. One observable effect is that SRM now doesn't interfere
with the termios settings of the terminal it's running in; another is
that the echo now only applies to real keypress data, and not
sequences auto-generated by the terminal.
The functions that previously lived in it now live in terminal.c
itself; they've been renamed term_keyinput and term_keyinputw, and
their function is to add data to the terminal's user input buffer from
a char or wchar_t string respectively.
They sit more comfortably in terminal.c anyway, because their whole
point is to translate into the character encoding that the terminal is
currently configured to use. Also, making them part of the terminal
code means they can also take care of calling term_seen_key_event(),
which simplifies most of the call sites in the GTK and Windows front
ends.
Generation of text _inside_ terminal.c, from responses to query escape
sequences, is therefore not done by calling those external entry
points: we send those responses directly to the ldisc, so that they
don't count as keypresses for all the user-facing purposes like bell
overload handling and scrollback reset. To make _that_ convenient,
I've arranged that most of the code that previously lived in
lpage_send and luni_send is now in separate translation functions, so
those can still be called from situations where you're not going to do
the default thing with the translated data.
(However, pasted data _does_ still count as close enough to a keypress
to call term_seen_key_event - but it clears the 'interactive' flag
when the data is passed on to the line discipline, which tweaks a
minor detail of control-char handling in line ending mode but mostly
just means pastes aren't interrupted.)
Having explicitly _stated_ in commit 4dcc0fddf the principle that if
you ever queue a toplevel callback on a freeable object then you
should also call delete_callbacks_for_context on that object before
freeing it, I realised I'd never actually gone through and checked
methodically at every call site of queue_toplevel_callback. So I did,
and naturally, I found several missing ones.
TranslateKey() on Windows passed all numeric-keypad key events to this
function in terminal.c, and accepted whatever it gave back. That
included the handling for the trivial case of the numeric keypad, when
Num Lock is on and application keypad mode hasn't overridden it, so
that the keypad should be returning actual digits. In that case,
format_numeric_keypad_key() itself was returning the same ASCII
character I had passed in to it as a keypad identifier, and
TranslateKey was returning that in turn as the final translation.
Unfortunately, that means that with Num Lock on, the numeric keypad
translates into what _I_ used as the logical keypad codes inside the
source code, not what the local keyboard layout thinks are the right
codes. In particular, the key I identified as keypad '.' would render
as '.' even on a German keyboard where it ought to produce ','.
Fixed by removing the fallback case in format_numeric_keypad_key()
itself, so now it returns the empty string if it didn't produce an
escape sequence as its translation. Instead, the special case is in
window.c, which checks for a zero-length output string and handles it
by falling through to the keyboard-layout specific ToUnicode code
further down TranslateKey().
On the GTK side, no change is needed here: the GTK keyboard handler
does things in the opposite order, by trying the local input method
_first_ (unless it can see a reason up front to override it), and only
calling format_numeric_keypad_key() if that didn't provide a
translation. So the fallback ASCII translation in the latter was
already not used.
Those two flags had the opposite sense to what you might expect: each
one is the value of the Conf entry corresponding to the checkbox that
_disables_ the corresponding terminal feature. So term->bidi is true
if and only if bidi is _off_.
I think that confusion of naming probably contributed to the control-
flow error fixed in the previous commit, just by increasing cognitive
load until I couldn't remember which flags were set where any more! So
now I've renamed the two fields of Terminal, and the corresponding
Conf keywords, to be called "no_bidi" and "no_arabicshaping", in line
with other 'disable this feature' flags, so that it's clear what the
sense should be.
The bidi algorithm is called on the array term->wcFrom, modifying it
in place. Then the Arabic-shaping algorithm - which can't work in
place because it needs to check the original value of array entries
it's already modified - is called, copying term->wcFrom to term->wcTo
as a side effect. Then the cleanup code expects the final version of
the line to be in wcTo. So if shaping is turned off, we still need to
copy wcFrom into wcTo, even if we don't modify it en route.
Previously, that copy was done under an if statement whose condition
boils down to 'if bidi is enabled but shaping is not'. So if that code
was ever reached with _both_ bidi and shaping turned off, then nothing
at all would copy wcFrom into wcTo, and wcTo would be filled with
nonsense.
Before trust sigils were introduced, that was OK, because the whole
function body was skipped if both bidi and shaping were turned off.
But now trust-sigil handling lives in there too, so we can get into
that code with the previously disallowed combination of flags. If
you're lucky, this means that the assert(opos == term->cols) near the
bottom of the function fails, on the basis that opos is the sum of
nonsense values from wcTo; if you're unlucky I suppose you might
manage to get _plausible_ nonsense through to the screen.
Now fixed, by changing that central if statement into a much more
obvious one: if we're running do_shape, then that can copy wcFrom into
wcTo, and if and only if we're _not_, then we must copy it another
way. (And while I'm here, I've turned that other way from a manual for
loop into memcpy.)
In terminal-based GUI applications, this is passed through to
term_set_trust_status, to toggle whether lines are prefixed with the
new trust sigil. In console applications, the function returns false,
indicating to the backend that it should employ some other technique
for spoofing protection.
This indicates that a line contains trusted information (originated by
PuTTY) or untrusted (from the server). Trusted lines are prefixed by a
three-column signature consisting of the trust sigil (i.e. PuTTY icon)
and a separating space.
To protect against a server using escape sequences to move the cursor
back up to a trusted line and overwrite its contents, any attempt to
write to a termline is preceded by a call to check_trust_status(),
which clears the line completely if the terminal's current trust
status is different from the previous state of that line.
In the terminal data structures, the trust sigil is represented by
0xDFFE (an otherwise unused value, because it's in the surrogate
space). For bidi purposes I've arranged to treat that value as
direction-neutral, so that it will appear on the right if a terminal
line needs it to. (Not that that's currently likely to happen, with
PuTTY not being properly localised, but it's a bit of futureproofing.)
The bidi system is also where I actually insert the trust sigil: the
_logical_ terminal data structures don't include it. term_bidi_line
was a convenient place to add it, because that function was already
transforming a logical terminal line into a physical one in a way that
also generates a logical<->physical mapping table for handling mouse
clicks and cursor positioning; so that function now adds the trust
sigil as well as running the bidi algorithm.
(A knock-on effect of _that_ is that the log<->phys position map now
has to have a value for 'no correspondence', because if the user does
click on the trust sigil, there's no logical terminal position
corresponding to that. So the map can now contain the special value
BIDI_CHAR_INDEX_NONE, and anyone looking things up in it has to be
prepared to receive that as an answer.)
Of course, this terminal-data transformation can't be kept _wholly_
within term_bidi_line, because unlike proper bidi, it actually reduces
the number of visible columns on the line. So the wrapping code
(during glyph display and also copy and paste) has to take account of
the trusted status and use it to ignore the last 3 columns of the
line. This is probably not done absolutely perfectly, but then, it
doesn't need to be - trusted lines will be filled with well-controlled
data generated from the SSH code, which won't be doing every trick in
the book with escape sequences. Only untrusted terminal lines will be
using all the terminal's capabilities, and they don't have this sigil
getting in the way.
Now, instead of each seat's prompt-handling function doing the
control-char sanitisation of prompt text, the SSH code does it. This
means we can do it differently depending on the prompt.
In particular, prompts _we_ generate (e.g. a genuine request for your
private key's passphrase) are not sanitised; but prompts coming from
the server (in keyboard-interactive mode, or its more restricted SSH-1
analogues, TIS and CryptoCard) are not only sanitised but also
line-length limited and surrounded by uncounterfeitable headers, like
I've just done to the authentication banners.
This should mean that if a malicious server tries to fake the local
passphrase prompt (perhaps because it's somehow already got a copy of
your _encrypted_ private key), you can tell the difference.
Now it can optionally check that output lines don't go beyond a
certain length (measured in terminal columns, via wcwidth, rather than
bytes or characters). In this mode, lines are prefixed with a
distinctive character (namely '|'), and if a line is too long, then it
is broken and the continuation line gets a different prefix ('>').
When StripCtrlChars is targeting a terminal, it asks the terminal to
call wcwidth on its behalf, so it can be sure to use the same idea as
the real terminal about which characters are wide (i.e. depending on
the configuration of ambiguous characters).
This mode isn't yet used anywhere.
The previous unlimited system was nicely general, but unfortunately
meant you could easily DoS a PuTTY-based terminal by sending a
printing character followed by an endless stream of identical
combining chars. (In fact, due to accidentally-quadratic linked list
management, you'd DoS it by using up all the CPU even before you got
the point of making it allocate all the RAM.)
The new limit is chosen to be 32, more or less arbitrarily. Overlong
sequences of combining characters are signalled by turning the whole
character cell into U+FFFD REPLACEMENT CHARACTER.
If the terminal is one column wide, it's not possible to print a
double-width CJK character at all - it won't fit. Replace it with
U+FFFD to indicate that impossibility.
The previous behaviour was to notice that we're in the rightmost
column of the terminal, and invoke the LATTR_WRAPPED2 special case to
wrap to the leftmost column on the next line. But in a width-1
terminal, the rightmost column _is_ the leftmost column, so this would
leave us no better off, and we would have fallen through into the next
case while in exactly the situation we'd tried to rule out.
The REP escape (ESC [ nnn b) causes the previously printed graphic
character to be repeated another nnn times. So if it's sent as the
very first thing in a terminal session, when there _is_ no previously
printed graphic character, there's nothing sensible it can do.
In fact, in that situation, it does something decidedly _not_
sensible: it takes the uninitialised value term->last_graphic_char and
sends it directly to term_display_graphic_char, with undesirable
results if it's not actually a printing character. In particular, the
value 0 is treated as a combining char (because it has zero wcwidth),
leading to a knock-on assertion failure when compressing the
scrollback lines (which uses \0 as a terminating value for sequences
of combining characters, precisely because it expects it never to show
up in an actual cc slot!).
Turns out that my assertion that term->cols == line->cols can
sometimes fail, because if the window is shrunk, scrlineptr()
deliberately _doesn't_ shrink the line (so that the columns on the
right can be recovered if the window is then resized larger again). So
clear_line() should _make_ the line the right width, instead of
asserting that it already is.
I've factored out clear_line() (wipe out everything on a terminal line
including its line attrs) and also line_cols() (determine how many
columns are on this particular line, taking into account
LATTR_WRAPPED2 which reduces it by one).
Also, newline() and freeline() were badly named. Now they're called
newtermline() and freetermline(), which include the full actual type
name they deal with, and also means that now neither of them is named
the same as a control character!
SSH authentication prompts (passwords, passphrases and keyboard-
interactive) were previously sanitised to remove escape sequences by
the simplistic sanitise_term_data() in utils.c. Now they're fed
through the new mode of StripCtrlChars instead, which means they
should permit printable Unicode (if the terminal is in UTF-8 mode)
while still disallowing escape sequences. Hopefully this will be a
usability improvement to everyone whose login prompts are in a
language not representable in plain ASCII.
Also, instead of insisting on modifying the UTF-8 decoding state
inside the Terminal structure, it now takes a separate pointer to a
small struct containing that decode state. The idea is that if a
separate module wants to decode characters the same way the real
terminal would, it can pass its own mutable state structure, but the
same main Terminal pointer.
The idea of these is that they centralise the common idiom along the
lines of
if (logical_array_len >= physical_array_size) {
physical_array_size = logical_array_len * 5 / 4 + 256;
array = sresize(array, physical_array_size, ElementType);
}
which happens at a zillion call sites throughout this code base, with
different random choices of the geometric factor and additive
constant, sometimes forgetting them completely, and generally doing a
lot of repeated work.
The new macro sgrowarray(array,size,n) has the semantics: here are the
array pointer and its physical size for you to modify, now please
ensure that the nth element exists, so I can write into it. And
sgrowarrayn(array,size,n,m) is the same except that it ensures that
the array has size at least n+m (so sgrowarray is just the special
case where m=1).
Now that this is a single centralised implementation that will be used
everywhere, I've also gone to more effort in the implementation, with
careful overflow checks that would have been painful to put at all the
previous call sites.
This commit also switches over every use of sresize(), apart from a
few where I really didn't think it would gain anything. A consequence
of that is that a lot of array-size variables have to have their types
changed to size_t, because the macros require that (they address-take
the size to pass to the underlying function).
Commit fec93d5e0 missed a piece: when we hand wcTo to
term_bidi_cache_store and it uses it to set up the mapping between
physical and logical character positions for cursor and selection
handling, it will assume wcTo has as many entries as there are columns
in the terminal. But in fact now wcTo may be shorter than that, so
term_bidi_cache_store also needs to pay attention to the nchars field.
The actual calls to win_draw_{text,cursor} in do_paint() were
duplicated in two places, and I may want to change them soon, so it's
convenient to centralise them.
Previously, any double-width character would break the bidi algorithm,
because of the quirk of data representation in which we store UCSWIDE
(0xDFFF) in the right-hand termchar overlapped by the character.
UCSWIDE has bidirectional character class L according to minibidi's
getType(), so it disrupted the algorithm.
Now we remove UCSWIDE from the input line before passing it to
do_bidi(), replacing it with an 'nchars' field in the bidi_char
structure indicating single or double width, and put the UCSWIDEs back
afterwards once do_bidi returns.
Now that all the call sites are expecting a size_t instead of an int
length field, it's no longer particularly difficult to make it
actually return the pointer,length pair in the form of a ptrlen.
It would be nice to say that simplifies call sites because those
ptrlens can all be passed straight along to other ptrlen-consuming
functions. Actually almost none of the call sites are like that _yet_,
but this makes it possible to move them in that direction in future
(as part of my general aim to migrate ptrlen-wards as much as I can).
But also it's just nicer to keep the pointer and length together in
one variable, and not have to declare them both in advance with two
extra lines of boilerplate.
This is a general cleanup which has been overdue for some time: lots
of length fields are now the machine word type rather than the (in
practice) fixed 'int'.
Taking a leaf out of the LLVM code base: this macro still includes an
assert(false) so that the message will show up in a typical build, but
it follows it up with a call to a function explicitly marked as no-
return.
So this ought to do a better job of convincing compilers that once a
code path hits this function it _really doesn't_ have to still faff
about with making up a bogus return value or filling in a variable
that 'might be used uninitialised' in the following code that won't be
reached anyway.
I've gone through the existing code looking for the assert(false) /
assert(0) idiom and replaced all the ones I found with the new macro,
which also meant I could remove a few pointless return statements and
variable initialisations that I'd already had to put in to placate
compiler front ends.
In the big boolification commit (3214563d8) I accidentally rewrote
"term->wrap == 0" as "term->wrap" instead of as "!term->wrap", so now
sending the backspace character to the terminal at the start of a line
causes the cursor to wrap round to the end of the previous line if and
only if it _shouldn't_ have done.
A long time ago, in commit 4d77b6567, I moved the generation of the
arrow-key escape sequences into a function format_arrow_key(). Mostly
the reason for that was a special purpose I had in mind at the time
which involved auto-generating the same sequences in response to
things other than a keypress, but I always thought it would be nice to
centralise a lot more of PuTTY's complicated keyboard handling in the
same way - at least the handling of the function keys and their
numerous static and dynamic config options.
In this year's general spirit of tidying up and refactoring, I think
it's finally time. So here I introduce three more centralised
functions for dealing with the numbered function keys, the small
keypad (Ins, Home, PgUp etc) and the numeric keypad. Lots of horrible
and duplicated code from the key handling functions in window.c and
gtkwin.c is now more sensibly centralised: each platform keyboard
handler concerns itself with the local format of a keyboard event and
platform-specific enumeration of key codes, and once it's decided what
the logical key press actually _is_, it hands off to the new functions
in terminal.c to generate the appropriate escape code.
Mostly this is intended to be a refactoring without functional change,
leaving the keyboard handling how it's always been. But in cases where
the Windows and GTK handlers were accidentally inconsistent, I've
fixed the inconsistency rather than carefully keeping both sides how
they were. Known consistency fixes:
- swapping the arrow keys between normal (ESC [ A) and application
(ESC O A) is now done by pressing Ctrl with them, and _not_ by
pressing Shift. That was how it was always supposed to work, and
how it's worked on GTK all along, but on Windows it's been done by
Shift as well since 2010, due to a bug at the call site of
format_arrow_key() introduced when I originally wrote that function.
- in Xterm function key mode plus application keypad mode, the /*-
keys on the numeric keypad now send ESC O {o,j,m} in place of ESC O
{Q,R,S}. That's how the Windows keyboard handler has worked all
along (it was a deliberate behaviour tweak for the Xterm-like
function key mode, because in that mode ESC O {Q,R,S} are generated
by F2-F4). But the GTK keyboard handler omitted that particular
special case and was still sending ESC O {Q,R,S} for those keys in
all application keypad modes.
- also in Xterm function key mode plus app keypad mode, we only
generates the app-keypad escape sequences if Num Lock is on; with
Num Lock off, the numeric keypad becomes arrow keys and
Home/End/etc, just as it would in non-app-keypad mode. Windows has
done this all along, but again, GTK lacked that special case.
'struct str' in terminal.c was an earlier and less good implementation
of the same concept as misc.h's strbuf, so I've replaced it with the
same strbuf we have everywhere. As a bonus, this means I can also use
put_uint{16,32} to save a bit of effort writing out the compressed
scrollback data.
On the decompression side, I've also switched to using BinarySource,
which has the advantage that now if the decoding goes wrong we can at
least be sure of not reading beyond the end of the buffer.
(The flip side of that is that now we _store_ the length of each
compressed line buffer, which costs a bit of memory. But I think it's
worth it for the safety and code consistency.)
This is another cleanup I felt a need for while I was doing
boolification. If you define a function or variable in one .c file and
declare it extern in another, then nothing will check you haven't got
the types of the two declarations mismatched - so when you're
_changing_ the type, it's a pain to make sure you've caught all the
copies of it.
It's better to put all those extern declarations in header files, so
that the declaration in the header is also in scope for the
definition. Then the compiler will complain if they don't match, which
is what I want.
My normal habit these days, in new code, is to treat int and bool as
_almost_ completely separate types. I'm still willing to use C's
implicit test for zero on an integer (e.g. 'if (!blob.len)' is fine,
no need to spell it out as blob.len != 0), but generally, if a
variable is going to be conceptually a boolean, I like to declare it
bool and assign to it using 'true' or 'false' rather than 0 or 1.
PuTTY is an exception, because it predates the C99 bool, and I've
stuck to its existing coding style even when adding new code to it.
But it's been annoying me more and more, so now that I've decided C99
bool is an acceptable thing to require from our toolchain in the first
place, here's a quite thorough trawl through the source doing
'boolification'. Many variables and function parameters are now typed
as bool rather than int; many assignments of 0 or 1 to those variables
are now spelled 'true' or 'false'.
I managed this thorough conversion with the help of a custom clang
plugin that I wrote to trawl the AST and apply heuristics to point out
where things might want changing. So I've even managed to do a decent
job on parts of the code I haven't looked at in years!
To make the plugin's work easier, I pushed platform front ends
generally in the direction of using standard 'bool' in preference to
platform-specific boolean types like Windows BOOL or GTK's gboolean;
I've left the platform booleans in places they _have_ to be for the
platform APIs to work right, but variables only used by my own code
have been converted wherever I found them.
In a few places there are int values that look very like booleans in
_most_ of the places they're used, but have a rarely-used third value,
or a distinction between different nonzero values that most users
don't care about. In these cases, I've _removed_ uses of 'true' and
'false' for the return values, to emphasise that there's something
more subtle going on than a simple boolean answer:
- the 'multisel' field in dialog.h's list box structure, for which
the GTK front end in particular recognises a difference between 1
and 2 but nearly everything else treats as boolean
- the 'urgent' parameter to plug_receive, where 1 vs 2 tells you
something about the specific location of the urgent pointer, but
most clients only care about 0 vs 'something nonzero'
- the return value of wc_match, where -1 indicates a syntax error in
the wildcard.
- the return values from SSH-1 RSA-key loading functions, which use
-1 for 'wrong passphrase' and 0 for all other failures (so any
caller which already knows it's not loading an _encrypted private_
key can treat them as boolean)
- term->esc_query, and the 'query' parameter in toggle_mode in
terminal.c, which _usually_ hold 0 for ESC[123h or 1 for ESC[?123h,
but can also hold -1 for some other intervening character that we
don't support.
In a few places there's an integer that I haven't turned into a bool
even though it really _can_ only take values 0 or 1 (and, as above,
tried to make the call sites consistent in not calling those values
true and false), on the grounds that I thought it would make it more
confusing to imply that the 0 value was in some sense 'negative' or
bad and the 1 positive or good:
- the return value of plug_accepting uses the POSIXish convention of
0=success and nonzero=error; I think if I made it bool then I'd
also want to reverse its sense, and that's a job for a separate
piece of work.
- the 'screen' parameter to lineptr() in terminal.c, where 0 and 1
represent the default and alternate screens. There's no obvious
reason why one of those should be considered 'true' or 'positive'
or 'success' - they're just indices - so I've left it as int.
ssh_scp_recv had particularly confusing semantics for its previous int
return value: its call sites used '<= 0' to check for error, but it
never actually returned a negative number, just 0 or 1. Now the
function and its call sites agree that it's a bool.
In a couple of places I've renamed variables called 'ret', because I
don't like that name any more - it's unclear whether it means the
return value (in preparation) for the _containing_ function or the
return value received from a subroutine call, and occasionally I've
accidentally used the same variable for both and introduced a bug. So
where one of those got in my way, I've renamed it to 'toret' or 'retd'
(the latter short for 'returned') in line with my usual modern
practice, but I haven't done a thorough job of finding all of them.
Finally, one amusing side effect of doing this is that I've had to
separate quite a few chained assignments. It used to be perfectly fine
to write 'a = b = c = TRUE' when a,b,c were int and TRUE was just a
the 'true' defined by stdbool.h, that idiom provokes a warning from
gcc: 'suggest parentheses around assignment used as truth value'!
I think this is the full set of things that ought logically to be
boolean.
One annoyance is that quite a few radio-button controls in config.c
address Conf fields that are now bool rather than int, which means
that the shared handler function can't just access them all with
conf_{get,set}_int. Rather than back out the rigorous separation of
int and bool in conf.c itself, I've just added a similar alternative
handler function for the bool-typed ones.
This commit includes <stdbool.h> from defs.h and deletes my
traditional definitions of TRUE and FALSE, but other than that, it's a
100% mechanical search-and-replace transforming all uses of TRUE and
FALSE into the C99-standardised lowercase spellings.
No actual types are changed in this commit; that will come next. This
is just getting the noise out of the way, so that subsequent commits
can have a higher proportion of signal.
After the recent Seat and LogContext revamps, _nearly_ all the
remaining uses of the type 'Frontend' were in terminal.c, which needs
all sorts of interactions with the GUI window the terminal lives in,
from the obvious (actually drawing text on the window, reading and
writing the clipboard) to the obscure (minimising, maximising and
moving the window in response to particular escape sequences).
All of those functions are now provided by an abstraction called
TermWin. The few remaining uses of Frontend after _that_ are internal
to a particular platform directory, so as to spread the implementation
of that particular kind of Frontend between multiple source files; so
I've renamed all of those so that they take a more specifically named
type that refers to the particular implementation rather than the
general abstraction.
So now the name 'Frontend' no longer exists in the code base at all,
and everywhere one used to be used, it's completely clear whether it
was operating in one of Frontend's three abstract roles (and if so,
which), or whether it was specific to a particular implementation.
Another type that's disappeared is 'Context', which used to be a
typedef defined to something different on each platform, describing
whatever short-lived resources were necessary to draw on the terminal
window: the front end would provide a ready-made one when calling
term_paint, and the terminal could request one with get_ctx/free_ctx
if it wanted to do proactive window updates. Now that drawing context
lives inside the TermWin itself, because there was never any need to
have two of those contexts live at the same time.
(Another minor API change is that the window-title functions - both
reading and writing - have had a missing 'const' added to their char *
parameters / return values.)
I don't expect this change to enable any particularly interesting new
functionality (in particular, I have no plans that need more than one
implementation of TermWin in the same application). But it completes
the tidying-up that began with the Seat and LogContext rework.
In the very old days, when PuTTY was new and computers were slow, I
tried to implement a feature where scrolling the window would be
implemented using a fast rectangle-copy GDI operation, rather than an
expensive character-by-character redraw of all the changed areas.
It never quite worked right, and I ended up conditioning it out on
Windows, and never even tried to implement it on GTK. It's now been
sitting around unused for so long that I think it's no longer worth
keeping in the code at all - if I tried to put it back in, it surely
wouldn't even compile, and would need rewriting from scratch anyway.
Disturbingly, it looks as if I _tried_ to re-enable it at one point,
in that there was a '#define OPTIMISE_IS_SCROLL 1' in putty.h - but
that never had any effect, because the macro name is misspelled. All
the #ifdefs are for 'OPTIMISE_SCROLL', without the 'IS'. So despite
appearances, it really _has_ been conditioned out all along!
Simon Tatham