The code that parses hexadecimal test arguments out of test lines
writes them into a buffer in binary form, and sets ptrs[i] to be the
starting point of each argument. The idea is that ptrs[i+1]-ptrs[i] is
the length of each argument - but for that to apply to the _final_
argument, we need to have set one final element in ptrs[], which I
forgot to do.
The variable 'toret' in ssh2_transport_get_specials would have been
returned while uninitialised in the case where neither of the if
statements in the function set it to true.
The BPP_READ macros in all four BPP implementations (including
sshverstring) had the same bug: if EOF had been seen on the network
input but there was _also_ enough data in the input queue to satisfy
the current request, they would jump straight to complaining about the
EOF rather than processing the available data first.
I spotted this while trying to pipe in test data from a disk file, but
it could easily also lead to us failing to handle the final message in
the connection, e.g. losing the error message sent by the remote in a
DISCONNECT message.
An empty display number matches any display number.
For example xauth list :1 returns auth cookies where the
display number matches and where the display number is empty.
Now, instead of getting the zlib test/helper program by manually
compiling a source file with unusual options, it gets built as
standard by the ordinary Makefile.
Now they live in their own file memory.c. The advantage of this is
that you can link them into a binary without also pulling in the rest
of misc.c with its various dependencies on other parts of the code,
such as conf.c.
It stopped being useful in commit 20a9bd564, where I removed the only
code that called it. I've only just noticed that this part of the
mechanism is still lying around.
In commit 884a7df94 I claimed that all my trait-like vtable systems
now had the generic object type being a struct rather than a bare
vtable pointer (e.g. instead of 'Socket' being a typedef for a pointer
to a const Socket_vtable, it's a typedef for a struct _containing_ a
vtable pointer).
In fact, I missed a few. This commit converts ssh_key, ssh2_cipher and
ssh1_cipher into the same form as the rest.
This should have been moved over from the main ssh_free function back
when I did the original splitting-up of ssh.c: the transport layer
schedules a timer for rekeying (and also for GSSAPI credential
checks), so when it's freed, it needs to ensure the timer doesn't get
called anyway on a stale pointer.
Two users reported this in the form of an assertion failure in
conf_get_int (when ssh2_transport_timer asks for CONF_ssh_rekey_time,
if the tree234 call inside conf_get_int is confused by the contents of
the freed memory into returning failure). In other circumstances (if
the freed memory has different contents) it manifests as a segfault,
but it's the same underlying bug either way.
Of course this wouldn't have prevented me from making that mistake
myself - it's not as if I carefully re-read the design principles
appendix before writing each code change! - but it might help explain
to _someone_ at some point...
When I added a use of PRIx32 to one of Pageant's debugging messages a
couple of days ago, I forgot that one of my build setups can't cope
with inclusion of <inttypes.h>, and somehow also forgot the
precautionary pre-push full build that would have reminded me.
Now the RSA signing function supports the two flags defined in
draft-miller-ssh-agent-02, and uses them to generate RSA signatures
based on SHA-256 and SHA-512, which look exactly like the ordinary
kind of RSA SHA-1 signature except that the decoded signature integer
has a different hash at the bottom and an ASN.1 identifying prefix to
match, and also the signature-type string prefixing the integer
changes from "ssh-rsa" to "rsa-sha2-256" or "rsa-sha2-512" as
appropriate.
We don't _accept_ signatures of these new types - that would need an
entirely different protocol extension - and we don't generate them
under any circumstances other than Pageant receiving a sign request
with one of those flags set.
Now each public-key algorithm gets to indicate what flags it supports,
and the ones it specifies support for may turn up in a call to its
sign() method.
We still don't actually support any flags yet, though.
A couple of people have mentioned to me recently that these days
OpenSSH is appending a uint32 flags word to the agent sign request,
with flags that ask for an RSA signature to be over a SHA-256 or
SHA-512 hash instead of the SHA-1 standardised in ssh-rsa.
This commit adds support for the mandatory part of this protocol: we
notice the flags word at all (previously we stopped parsing the packet
before even finding it there), and return failure to the signing
request if it has any flag set that we don't support, which currently
means if it has any flag set whatsoever.
While I'm here, I've also added an error check for an undecodable sign
request. (It seemed silly to be checking get_err(msg) _after_ trying
to read the flags word without also having checked it before.)
The event log messages generated during DH key exchange now include both the
modulus size and hash algorithm used as well as whether the DH parameters
are from one of the standardized groups or were supplied by the server
during Group Exchange.
The gdb version of container_of can do better than the C function,
because you don't have to specify the structure field name if it can
be inferred from the type of the input expression.
And $list234 can be made to automatically list the contents of each
tree element, not just a pointer to it - just the thing for looking
quickly through sktree or s->channels to find the one you're after.
Jacob ran across a server which terminated a GSS userauth attempt by
sending that message before USERAUTH_FAILURE. The result was that
PuTTY interpreted the ERRTOK as indicating a failure of authentication
(so far, fair enough), and then handled it exactly as it would have
handled USERAUTH_FAILURE itself: it pushed it back on in_pq and went
back round the main userauth loop. But the thing that loop is
expecting to find at the head of in_pq is an _actual_
USERAUTH_FAILURE, not some arbitrary thing that preceded it. So this
led to some confusion, especially when the real USERAUTH_FAILURE that
immediately followed the ERRTOK got interpreted as the response to the
_next_ auth attempt.
One of the root causes is that we had no handler for ERRTOK at all. We
now do. (Though it's trivial - we ignore the content of the message
and just wait for the followup USERAUTH_FAILURE that the GSSAPI RFC
says MUST come next. Possibly there's something nicer we could do
involving handing the error token to the GSSAPI library and letting it
print a final user-facing message? But that's beyond my GSS expertise.)
But this also exposed another problem: we shouldn't be pushing _any_
packet back on in_pq unless it's actually a USERAUTH_FAILURE. Any
other unexpected packet should have _different_ confused handling. So
now that call to pq_push_front is conditionalised on the packet type,
and only triggers for USERAUTH_FAILURE proper.
Looks as if I introduced this bug in commit 431f92ade, when I moved
mainchan out into its own source file: the previous version of
mainchan_send_eof conditionalised the setting of mc->eof_sent in the
same if statement that actually sent the EOF, but somehow, in the new
version, only one of those operations was inside the if.
The effect is that in plink -nc mode, if the server sends EOF first,
the client stops listening to standard input at its own end, so it
never knows when to send EOF back and clean things up.
It claimed they were only found in ssh.c, which is no longer true:
after I broke up ssh.c into smaller pieces, they're now found all over
the place.
Also, one of the things I did during that refactoring was to arrange
that each protocol layer's cleanup function (hopefully) reliably frees
everything the coroutine might have allocated and been in the middle
of using, which was something I knew the old code was quite bad at. So
I've mentioned that in the coroutines section too, while I'm here.
I've recently started using several C99 features in PuTTY, after
finally reaching the point where it didn't break my builds to do so,
even on Windows. So it's now outright inaccurate for the documented
design principles to claim that we're sticking to C90.
While I'm here, I've filled in a bit more detail about the assumptions
we do permit.
ssh2_channel_check_throttle should only be called on channels for
which c->chan != NULL - that is, only for channels that are not
delegated to a sharing downstream. But throttle_all_channels was
calling it for _all_ channels, so if it had the bad luck to be called
while a sharing downstream was active, ssh2_channel_check_throttle
would dereference the null c->chan for the first downstream channel it
found.
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.
sk_startup and sk_nextaddr are entirely internal to winnet.c; nearly
all of import.c and minibidi.c's internal routines should have been
static and weren't; {read,write}_utf8 are internal to charset/utf8.c
(and didn't even need separate declarations at all); do_sftp_cleanup
is internal to psftp.c, and get_listitemheight to gtkdlg.c.
While I was editing those prototypes anyway, I've also added missing
'const' to the 'char *' passphrase parameters in import,c.
For a start, they now have different names on Windows and Unix,
reflecting their different roles: on Windows they apply escaping to
any string that's going to be used as a registry key (be it a session
name, or a host name for host key storage), whereas on Unix they're
for constructing saved-session file names in particular (and also
handle the special case of filling in "Default Settings" for NULL).
Also, they now produce output by writing to a strbuf, which simplifies
a lot of the call sites. In particular, the strbuf output idiom is
passed on to enum_settings_next, which is especially nice because its
only actual caller was doing an ad-hoc realloc loop that I can now get
rid of completely.
Thirdly, on Windows they're centralised into winmisc.c instead of
living in winstore.c, because that way Pageant can use the unescape
function too. (It was spotting the duplication there that made me
think of doing this in the first place, but once I'd started, I had to
keep unravelling the thread...)
There's one of these centralised in win_strerror() in winmisc.c, and
it doesn't seem worth keeping an earlier iteration of the same idea
entirely separate in winsock_error_string.
This removal means that non-network-specific error codes received in a
network context will no longer have "Network error:" prefixed to them.
But I think that's OK, because it seems unlikely to be critically
important that such an error was received from a network function - if
anything like that comes up then it's probably some kind of systemwide
chaos.
Jumped out at me in my trawl of the whole code base:
set_explicit_app_user_model_id is declared and defined as () rather
than (void), but this isn't C++.
I came across this unexplained static variable in my boolification
trawl. It seems clearly unintentional that it has only one instance
instead of one per terminal window - the code in question closely
resembles the Windows front end, and I think this must just be a
variable that I never swept up into 'inst' in the very early days when
I was making gtkwin.c out of a cloned-and-hacked window.c in the first
place.
These days it's even a bug, now that the OS X port actually does run
multiple terminal windows in the same process: if one goes into mouse
reporting mode, I'm pretty sure this would have done confusing things
to the effects of mouse actions in the other.
Notably toint(), which ought to compile down to the identity function
in any case so you don't really want to put in a pointless call
overhead, and make_ptrlen() (and a couple of its wrappers) which is
standing in for what ought to be a struct-literal syntax.
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.
It's not actually used anywhere yet, though. This is just adding the
accessor functions, which will enforce a rigorous separation between
conf keys typed as int and as bool.
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.
The annoying int64.h is completely retired, since C99 guarantees a
64-bit integer type that you can actually treat like an ordinary
integer. Also, I've replaced the local typedefs uint32 and word32
(scattered through different parts of the crypto code) with the
standard uint32_t.
If values are boolean, it's confusing to use & and | in place of &&
and ||. In two of these three cases it was simply a typo and I've used
the other one; in the third, it was a deliberate avoidance of short-
circuit evaluation (and commented as such), but having seen how easy
it is to make the same typo by accident, I've decided it's clearer to
just move the LHS and RHS evaluations outside the expression.
In some compiler modes - notably the one that gtk-config selects when
GTK PuTTY is built for GTK 1 - it's an error to typedef the same thing
twice. 'mainchan' is defined in defs.h, so it doesn't need defining
again where the structure contents are specified.
x11_char_struct returns a pointer or NULL, so while returning FALSE
would have ended up doing the right thing after macro expansion and
integer->pointer conversion, it wasn't actually how I _meant_ to spell
a failure return.
In GTK 2, this function was a new and convenient way to override the
order in which the Tab key cycled through the sub-widgets of a
container, replacing the more verbose mechanism in GTK 1 where you had
to provide a custom implementation of the "focus" method in
GtkContainerClass.
GTK 3.24 has now deprecated gtk_container_set_focus_chain(),
apparently on the grounds that that old system is what they think you
_ought_ to be doing. So I've abandoned set_focus_chain completely, and
switched back to doing it by a custom focus method for _all_ versions
of GTK, with the only slight wrinkle being that between GTK 1 and 2
the method in question moved from GtkContainer to GtkWidget (my guess
is so that an individual non-container widget can still have multiple
separately focusable UI components).
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!
Colin Harrison points out that commit c31e3cd43 was less cautious than
it should have been: when delete_callbacks_for_context nulls out the
'next' pointer in the new tail element of the callbacks list, it
should only do so if there _is_ a new tail element. If the list has
just become empty, that won't work very well!
A function containing assert(FALSE && "Should never be called")
doesn't _really_ need to return a value, but it makes compilers
happier if it pretends to.
