In commit 4ecc3f3c09 I did a knee-jerk fix of a macro of the form
#define SECOND_PASS_ONLY { body; }
on the grounds that it was syntax-unsafe, so I wrapped it in the
standard do while(0):
#define SECOND_PASS_ONLY do { body; } while (0)
But in this case, that was a bogus transformation, because the body
executed 'continue' with the intention of affecting the containing
loop (outside the macro). Moreover, ten lines above the macro
definition was a comment specifically explaining why it _couldn't_ be
wrapped in do while (0) !
Since then I've come up with an alternative break-and-continue-proof
wrapper for macros that are supposed to expand to something that's
syntactically a C statement. So I've used that instead, and while I'm
at it, fixed the neighbouring EXPECTS_ARG as well.
Spotted by Coverity, and well spotted indeed! How embarrassing.
wcrtomb returns a size_t, so it's silly to immediately assign it into
an int variable. Apparently running gcc with LTO enabled points this
out as an error.
This was benign as far as I can see: the obvious risk of integer
overflow could only happen if the OS wanted to convert a single wide
character into more than 2^31 bytes, and the test of the return value
against (size_t)-1 for an error check seems to work anyway in
practice, although I suspect that's only because of implementation-
defined behaviour in gcc at the point where the size_t is narrowed to
a signed int.
(cherry picked from commit 99f5fa34ab)
Mark Wooding points out that when running with the +ut flag, we close
pty_utmp_helper_pipe during pty backend setup, which causes the
previously forked helper process to terminate. If that termination
happens quickly enough, then the code later in pty_backend_create
won't have set up the SIGCHLD handler and its pipe yet, so when we get
to the main event loop, we'll fail to notice that subprocess waiting
to be reaped, and leave it lying around as a zombie.
An easy fix is to move the handler and pipe setup to before the code
that potentially closes pty_utmp_helper_pipe, so that there isn't a
race condition any more.
(cherry picked from commit 7ffa6ed41e)
Colin reports that on betas of Ubuntu 20.04, Pango has switched to
getting its font metrics from HarfBuzz, and a side effect is
apparently that they're being returned in the full precision of
PANGO_SCALE fixed point.
Previously, Pango appears to have been returning values that were
always a whole number of pixels scaled by PANGO_SCALE. Moreover, it
looks as if it was rounding the font ascent and descent _up_ to a
whole number of pixels, rather than rounding to nearest. But our code
rounds to nearest, which means that now the same font gets allocated
fewer vertical pixels, which can be enough to cut off some ascenders
or descenders.
Pango already provides the macro PANGO_PIXELS_CEIL, so it's easy to
switch over to using it. This should arrange that any text that fits
within the font's stated ascent/descent measurements will also fit in
the character cell.
(cherry picked from commit f9a46a9581)
If gdk_event_get_scroll_deltas() return failure for a given
GdkEventScroll, it doesn't follow that that event has no usable
scrolling action in it at all. The fallback is to call
gdk_event_get_scroll_direction() instead, which is less precise but
still gives _something_ you can use. So in that situation, instead of
just returning false, we can fall through to the handling we use for
pre-GTK3 scroll events (which are always imprecise).
In particular, I've noticed recently that if you run GTK 3 PuTTY in
the virtual X display created by vnc4server, and connect to it using
xtightvncviewer, then scroll-wheel actions passed through from the VNC
client will cause scroll_event() to receive low-res GdkEventScroll
structures of exactly this kind. So scroll-wheel activity on the
terminal window wasn't causing a scroll in that environment, and with
this patch, it does.
(cherry picked from commit 0fd30113f1)
That causes the config dialog to terminate with result -1, which
wasn't handled at all by the result-receiving code. So GTK PuTTY would
continue running its main loop even though it had no windows open and
wasn't ever planning to do anything.
(cherry picked from commit 4fc5d7a5f5)
This file exports several functions defined in sshserver.h, and the
declarations weren't being type-checked against the definitions.
(cherry picked from commit 37d91aabff)
I'm not really sure why that's necessary: by my understanding of the C
standard, it shouldn't be. But my observation is that when compiling
with {Address,Leak} Sanitiser enabled, pageant --askpass can somehow
manage to exit without having actually written the passphrase to its
standard output.
(cherry picked from commit c618d6baac)
On Windows, due to a copy-paste goof, the message that should have
read "Configuring n stop bits" instead ended with "data bits".
While I'm here, I've arranged that the "1 stop bit" case of that
message is in the singular. And then I've done the same thing again on
Unix, because I noticed that message was unconditionally plural too.
(cherry picked from commit bdb7b47a5e)
The sets of poll(2) events that we check in order to return SELECT_R
and SELECT_W overlap: to be precise, they have POLLERR in common. So
if an fd signals POLLERR, then pollwrap_get_fd_rwx will respond by
saying that it has both SELECT_R and SELECT_W available on it - even
if the caller had only asked for one of those.
In other words, you can get a spurious SELECT_W notification on an fd
that you never asked for SELECT_W on in the first place. This
definitely isn't what I'd meant that API to do.
In particular, if a socket in the middle of an asynchronous connect()
signals POLLERR, then Unix Plink will call select_result for it with
SELECT_R and then SELECT_W respectively. The former will notice that
it's got an error condition and call plug_closing - and _then_ the
latter will decide that it's writable and set s->connected! The plan
was to only select it for write until it was connected, but this bug
in pollwrap was defeating that plan.
Now pollwrap_get_fd_rwx should only ever return a set of rwx flags
that's a subset of the one that the client asked for via
pollwrap_add_fd_rwx.
(cherry picked from commit 78974fce89)
When I came to actually remove the global 'flags' word, I found that I
got compile failures in two functions that should never have been
accessing it at all, because they forgot to declare _local_ variables
of the same name. Yikes!
(Of course, _now_ that's harmless, because I've just removed all the
actual semantics from the global variable. But I'm about to remove the
variable too, so these bugs would become compile failures.)
(cherry picked from commit 33715c07e3)
Those chomp operations in wincons.c and uxcons.c looked ugly, and I'm
not totally convinced they couldn't underrun the buffer by 1 byte in
weird circumstances. strbuf_chomp is neater.
(cherry picked from commit 7590d0625b)
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)
When I reworked the 'selparams' array in commit e790adec4 to contain
pointers to 'struct selparam' rather than directly containing
structures, I missed this one case where I should have removed an &.
As a result the GTK1 signal handler that deals with clicks on the
config-pane selection treeview was getting a pointer to a pointer and
treating it as a pointer to an object. Nothing good happened.
I was testing some upcoming new GTK code against all GTK versions,
which for once was interesting enough to make nontrivial use of
g_object_ref_sink, and I found that I hadn't implemented the GTK1
fallback version right. GTK1 has no ref_sink call, but it does have
ref and sink, so the right thing seems to be to just call them in
succession.
If you go to Change Settings in Unix PuTTY or pterm, and change the
'Gap between text and window edge' setting but not the width and
height, then change_settings_menuitem() correctly sets the physical
window to a new size, but drawing_area_setup() was not recreating the
backing surface / pixmap in the same way, because it hadn't spotted
that the border size might be relevant.
Now I unconditionally work out what the exact size of the backing
surface _ought_ to be, before reaching the potential early exit path,
and never take the early exit if the backing area needs resizing for
any reason at all.
(I think this probably ought to have been part of commit 528513dde.)
I'm about to rearrange this function, and the patch that actually does
work will be easier to read if mass reindentation isn't combined with
it.
The braces I've just removed were necessary when we hadn't yet
committed to requiring (most of) C99 from all our build platforms. Now
they aren't.
Up until now, it's been a variadic _function_, whose argument list
consists of 'const char *' ASCIZ strings to concatenate, terminated by
one containing a null pointer. Now, that function is dupcat_fn(), and
it's wrapped by a C99 variadic _macro_ called dupcat(), which
automatically suffixes the null-pointer terminating argument.
This has three benefits. Firstly, it's just less effort at every call
site. Secondly, it protects against the risk of accidentally leaving
off the NULL, causing arbitrary words of stack memory to be
dereferenced as char pointers. And thirdly, it protects against the
more subtle risk of writing a bare 'NULL' as the terminating argument,
instead of casting it explicitly to a pointer. That last one is
necessary because C permits the macro NULL to expand to an integer
constant such as 0, so NULL by itself may not have pointer type, and
worse, it may not be marshalled in a variadic argument list in the
same way as a pointer. (For example, on a 64-bit machine it might only
occupy 32 bits. And yet, on another 64-bit platform, it might work
just fine, so that you don't notice the mistake!)
I was inspired to do this by happening to notice one of those bare
NULL terminators, and thinking I'd better check if there were any
more. Turned out there were quite a few. Now there are none.
When I introduced the unreachable() macro in commit 0112936ef, I
searched the source code for assert(0) and assert(false), together
with their variant form assert(0 && "explanatory text"). But I didn't
search for assert(!"explanatory text"), which is the form I used to
use before finding that assert(0 && "text") seemed to be preferred in
other code bases.
So, here's a belated replacement of all the assert(!"stuff") macros
with further instances of unreachable().
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.
It's silly to use the higher-level name_lookup(), because the way in
which they differ is that name_lookup() allows DNS lookups to be
deferred to the other side of a network proxy over which your
connection is travelling - and when you're trying to find out your own
host name for use as a database key, there's no _connection_ involved
at all, and no potential proxy either.
I've only just noticed that it doesn't do anything at all!
Almost every implementation of the Socket vtable provides a flush()
method which does nothing, optionally with a comment explaining why
it's OK to do nothing. The sole exception is the wrapper Proxy_Socket,
which implements the method during its setup phase by setting a
pending_flush flag, so that when its sub-socket is later created, it
can call sk_flush on that. But since the sub-socket's sk_flush will do
nothing, even that is completely pointless!
Source control history says that sk_flush was introduced by Dave
Hinton in 2001 (commit 7b0e08270), who was going to use it for some
purpose involving the SSL Telnet support he was working on at the
time. That SSL support was never finished, and its vestigial
declarations in network.h were removed in 2015 (commit 42334b65b). So
sk_flush is just another vestige of that abandoned work, which I
should have removed in the latter commit but overlooked.
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.
Introduced by 61f3e3e29, as part of my periodic efforts to make the
GTK front end work usefully on OS X: the 'Unable to open connection to
[host]: [error]' message box is accidentally passed through two layers
of printf format-string parsing, the second of which has no argument
list. So if you pass in a host name like '%s' on the command line,
bad things will happen when that error message is constructed.
This happened because in that commit I changed a call to
fatal_message_box() into a call to connection_fatal(), without
noticing that the latter was printf-style variadic and the former
wasn't. On the plus side, that means now I can remove the explicit
dupprintf/free around the error message.
If you select an entry in the saved sessions list box, but without
double-clicking to actually load it, and then you hit OK, the config-
box code will automatically load it. That just saves one click in a
common situation.
But in order to load that session, the config-box system first has to
ask the list-box control _which_ session is selected. On Windows, this
causes an assertion failure if the user has switched away from the
Session panel to some other panel of the config box, because when the
list box isn't on screen, its Windows control object is actually
destroyed.
I think a sensible answer is that we shouldn't be doing that implicit
load behaviour in any case if the list box isn't _visible_: silently
loading and launching a session someone selected a lot of UI actions
ago wasn't really the point. So now I make that behaviour only happen
when the list box (i.e. the Session panel) _is_ visible. That should
prevent the assertion failure on Windows, but the UI effect is cross-
platform, applying even on GTK where the control objects for invisible
panels persist and so the assertion failure didn't happen. I think
it's a reasonable UI change to make globally.
In order to implement it, I've had to invent a new query function so
that config.c can tell whether a given control is visible. In order to
do that on GTK, I had to give each control a pointer to the 'selparam'
structure describing its config-box pane, so that query function could
check it against the current one - and in order to do _that_, I had to
first arrange that those 'selparam' structures have stable addresses
from the moment they're first created, which meant adding a layer of
indirection so that the array of them in the top-level dlgparam
structure is now an array of _pointers_ rather than of actual structs.
(That way, realloc half way through config box creation can't
invalidate the important pointer values.)
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.
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.)
If the user closes the Change Settings dialog box using the close
button provided by the window manager (or some analogous thing that
generates the same X11 event) instead of using the Cancel button
within the dialog itself, then after_change_settings_dialog() gets
called with retval < 0, which triggers an early return path in which
we forget to call unregister_dialog(), and as a result, assertions
fail all over the place the _next_ time you try to put up a Change
Settings dialog.
Also, the early return causes ctx.newconf to be memory-leaked. So
rather than just moving the unregister_dialog() call to above the
early return, a better fix is to remove the early return completely,
and simply treat retval<0 the same as retval==0: it doesn't matter
_how_ the user closed the config box without committing the changes,
it only matters that they did.
When I start Uppity in listening mode, it's useful to have it
acknowledge that it _has_ started up in that mode, and isn't (for
example) stuck somewhere in my local wrapper script.
Coverity complained that it was wrong to use rand() in a security
context, and although in this case it's _very_ marginal, I can't
actually disagree that the choice of which light to light up to avoid
giving information about passphrase length is a security context.
So, no more rand(); instead we instantiate a shiny Fortuna PRNG
instance, seed it in more or less the usual way, and use that as an
overkill-level method of choosing which light to light up next.
(Acknowledging that this is a slightly unusual application and less
critical than most, I don't actually put the passphrase characters
themselves into the PRNG, and I don't use a random-seed file.)
It's identical in uxnoise and winnoise, being written entirely in
terms of existing cross-platform functions. Might as well centralise
it into sshrand.c.
We have to use the file name we just failed to open to format an error
message _before_ freeing it, not after. If that use-after-free managed
not to cause a crash, we'd also leak the file descriptor 'outfd'.
Both spotted by Coverity (which is probably the first thing in years
to look seriously at any of the code designed for Ben's AFL exercise).
The recent rewriting in both the GTK and Windows keyboard handlers
left the keypad 'Enter' key in a bad state, when no override is
enabled that causes it to generate an escape sequence.
On Windows, a series of fallbacks was causing it to generate \r
regardless of configuration, whereas in Telnet mode it should default
to generating the special Telnet new-line sequence, and in response to
ESC[20h (enabling term->cr_lf_return) it should generate \r\n.
On GTK, it wasn't generating anything _at all_, and also, I can't see
any evidence that the GTK keyboard handler had ever remembered to
implement the cr_lf_return mode.
Now Keypad Enter in non-escape-sequence mode should behave just like
Return, on both platforms.
With this and the ciphers, I think we've now got the full range of
SSH-1 config options (such as they are) that correspond to varying the
KEXINIT strings in SSH-2.
Despite the name, AuthPolicy in uxserver.c was also holding the state
of the current connection, including in particular how far through the
multi-step test keyboard-interactive interaction we are. But now that
Uppity can handle multiple connections in the same run, we need to
reset that state between connections. So the tree234s of acceptable
user keys now live in an AuthPolicyShared structure, and AuthPolicy
proper is a field of server_instance.
All my instincts expect the shell subprocesses to start off in ~, so
it's confusing if they start off in some random PuTTY checkout
directory. So now we default to $HOME, and if I really do want the
latter, I can use the new config option to reselect '.'.
My helper scripts for invoking Uppity have been manually unsetting
things like XAUTHORITY and SSH_AUTH_SOCK, to avoid accidentally
passing them through from my primary login session, so that I don't
get confused about whether agent forwarding is happening, or end up
with one DISPLAY going with a different XAUTHORITY.
Now I clear these within Uppity itself, so the wrapping script won't
have to.
In some contexts (namely pterm on a pure Wayland system, and Uppity),
seat_get_x_display() will return NULL. In that situation uxpty.c was
cheerfully passing it to dupprintf regardless, which in principle is
undefined behaviour and in practice was causing it to construct the
silly environment string "DISPLAY=(null)".
Now we handle that case by unsetenv("DISPLAY") instead.
