A trawl through the code with -Wmissing-prototypes and
-Wmissing-variable-declarations turned up a lot of things that should
have been internal to a particular source file, but were accidentally
global. Keep the namespace clean by making them all static.
(Also, while I'm here, a couple of them were missing a 'const': the
ONE and ZERO arrays in sshcrcda.c, and EMPTY_WINDOW_TITLE in
terminal.c.)
In commit 09954a87c I introduced the portfwdmgr_connect_socket()
system, which opened a port forwarding given a callback to create the
Socket itself, with the aim of using it to make forwardings to Unix-
domain sockets and Windows named pipes (both initially for agent
forwarding).
But I forgot that a year and a bit ago, in commit 834396170, I already
introduced a similar low-level system for creating a PortForwarding
around an unusual kind of socket: the portfwd_raw_new() system, which
in place of a callback uses a two-phase setup protocol (you create the
socket in between the two setup calls, and can roll it back if the
socket can't be created).
There's really no need to have _both_ these systems! So now I'm
merging them, which is to say, I'm enhancing portfwd_raw_new to have
the one new feature it needs, and throwing away the newer system
completely. The new feature is to be able to control the initial state
of the 'ready' flag: portfwd_raw_new was always used for initiating
port forwardings in response to an incoming local connection, which
means you need to start off with ready=false and set it true when the
other end of the SSH connection sends back OPEN_CONFIRMATION. Now it's
being used for initiating port forwardings in response to a
CHANNEL_OPEN, we need to be able to start with ready=true.
This commit reverts 09954a87c2 and its
followup fix 12aa06ccc9, and simplifies
the agent_connect system down to a single trivial function that makes
a Socket given a Plug.
As in the previous commit, this means that agent_query() is now able
to operate in an asynchronous mode, so that if Pageant takes time to
answer a request, the GUI of the PuTTY instance making the request
won't be blocked.
Also as in the previous commit, we still fall back to the WM_COPYDATA
protocol if the new named pipe protocol isn't available.
Now that Pageant runs a named-pipe server as well as a WM_COPYDATA
server, we prefer the former (if available) for agent forwarding, for
the same reasons as on Unix: it lets us establish a simple raw-data
streaming connection instead of agentf.c's complicated message
boundary detection and buffer management, and if agent connections
ever become stateful, this technique will cope.
On Windows, another advantage of this change is that forwarded agent
requests can now be asynchronous: if the agent takes time to respond
to a request for any reason, then the rest of PuTTY's GUI and SSH
connection are not blocked, and you can carry on working while the
agent is thinking about the request.
(I didn't list that as a benefit of doing the same thing for Unix in
commit ae1148267, because on Unix, agent_query() could _already_ run
asynchronously. It's only on Windows that that's new.)
This reuses all the named-pipe IPC code I set up for connection
sharing a few years ago, to set up a named pipe with a predictable
name and speak the stream-oriented SSH agent protocol over it.
In this commit, we just set up the server, and there's no code that
speaks the client end of the new IPC yet. But my plan is that clients
should switch over to using this interface if possible, because it's
generally better: it doesn't have to be handled synchronously in the
middle of a GUI event loop (either in Pageant itself _or_ in its
client), and it's a better fit to the connection-oriented nature of
forwarded agent connections (so if any features ever appear in the
agent protocol that require state within a connection, we'll now be
able to support them).
Historically, because of the way Windows Pageant's IPC works, PuTTY's
agent forwarding has always been message-oriented. The channel
implementation in agentf.c deals with receiving a data stream from the
remote agent client and breaking it up into messages, and then it
passes each message individually to agent_query().
On Unix, this is more work than is really needed, and I've always
meant to get round to doing the more obvious thing: making an agent
forwarding channel into simply a stream-oriented proxy, passing raw
data back and forth between the SSH channel and the local AF_UNIX
socket without having to know or care about the message boundaries in
the stream.
The portfwdmgr_connect_socket() facility introduced by the previous
commit is the missing piece of infrastructure to make that possible.
Now, the agent client module provides an API that includes a callback
you can pass to portfwdmgr_connect_socket() to open a streamed agent
connection, and the agent forwarding setup function tries to use that
where possible, only falling back to the message-based agentf.c system
if it can't be done. On Windows, the new piece of agent-client API
returns failure, so we still fall back to agentf.c there.
There are two benefits to doing it this way. One is that it's just
simpler and more robust: if PuTTY isn't trying to parse the agent
connection, then it has less work to do and fewer places to introduce
bugs. The other is that it's futureproof against changes in the agent
protocol: if any kind of extension is ever introduced that requires
keeping state within a single agent connection, or that changes the
protocol itself so that agentf's message-boundary detection stops
working, then this forwarding system will still work.
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 agent sent a response whose length field describes an interval
of memory larger than the file-mapping object the message is supposed
to be stored in, we shouldn't return that message to the client as if
nothing is wrong. Treat that the same as a failure to receive any
response at all.
Those were a reasonable abbreviation when the code almost never had to
deal with little-endian numbers, but they've crept into enough places
now (e.g. the ECC formatting) that I think I'd now prefer that every
use of the integer read/write macros was clearly marked with its
endianness.
So all uses of GET_??BIT and PUT_??BIT are now qualified. The special
versions in x11fwd.c, which used variable endianness because so does
the X11 protocol, are suffixed _X11 to make that clear, and where that
pushed line lengths over 80 characters I've taken the opportunity to
name a local variable to remind me of what that extra parameter
actually does.
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.
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'!
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.
This simplifies the client code both in ssh.c and in the client side
of Pageant.
I've cheated a tiny bit by preparing agent requests in a strbuf that
has space reserved at the front for the packet frame, which makes life
easier for the code that sends them off.
The previous agent-forwarding system worked by passing each complete
query received from the input to agent_query() as soon as it was
ready. So if the remote client were to pipeline multiple requests,
then Unix PuTTY (in which agent_query() works asynchronously) would
parallelise them into many _simultaneous_ connections to the real
agent - and would not track which query went out first, so that if the
real agent happened to send its replies (to what _it_ thought were
independent clients) in the wrong order, then PuTTY would serialise
the replies on to the forwarding channel in whatever order it got
them, which wouldn't be the order the remote client was expecting.
To solve this, I've done a considerable rewrite, which keeps the
request stream in a bufchain, and only removes data from the bufchain
when it has a complete request. Then, if agent_query decides to be
asynchronous, the forwarding system waits for _that_ agent response
before even trying to extract the next request's worth of data from
the bufchain.
As an added bonus (in principle), this gives agent-forwarding channels
some actual flow control for the first time ever! If a client spams us
with an endless stream of rapid requests, and never reads its
responses, then the output side of the channel will run out of window,
which causes us to stop processing requests until we have space to
send responses again, which in turn causes us to stop granting extra
window on the input side, which serves the client right.
Now, instead of returning a boolean indicating whether the query has
completed or is still pending, agent_query() returns NULL to indicate
that the query _has_ completed, and if it hasn't, it returns a pointer
to a context structure representing the pending query, so that the
latter can be used to cancel the query if (for example) you later
decide you need to free the thing its callback was using as a context.
This should fix a potential race-condition segfault if you overload an
agent forwarding channel and then close it abruptly. (Which nobody
will be doing for sensible purposes, of course! But I ran across this
while stress-testing other aspects of agent forwarding.)
It's been commented out for ages because it never really worked, and
it's about to become further out of date when I make other changes to
the agent client code, so it's time to get rid of it before it gets in
the way.
If and when I do get round to supporting asynchronous agent requests
on Windows, it's now pretty clear to me that trying to coerce this
ghastly window-message IPC into the right shape is the wrong way, and
a better approach will be to make Pageant support a named-pipe based
alternative transport for its agent connections, and speaking the
ordinary stream-oriented agent protocol over that. Then Pageant will
be able to start adding interactive features (like confirmation
dialogs or on-demand decryption) with freedom to reply to multiple
simultaneous agent connections in whatever order it finds convenient.
A user reported in January that locking down our process ACL causes
get_user_sid's call to OpenProcessToken to fail with a permissions
error. This _shouldn't_ be important, because we'll already have found
and cached the user SID before getting that far - but unfortunately
the call to get_user_sid in winnpc.c was bypassing the cache and
trying the whole process again.
This fix changes the memory ownership semantics of get_user_sid():
it's now an error to free the value it gives you, or else the *next*
call to get_user_sid() will return a stale pointer. Hence, also
removed those frees everywhere they appear.
There's now a winsecur.[ch], which centralises helper functions using
the Windows security stuff in advapi.h (currently just get_user_sid),
and also centralises the run-time loading of those functions and
checking they're all there.
[originally from svn r10082]
The most interesting one is printer_add_enum, which I've modified to
take a char ** rather than a char * so that it can both realloc its
input buffer _and_ return NULL to indicate error.
[originally from svn r9959]
code (as introduced in r9043), so that it uses the user SID rather
than the default SID.
This does change the access-control model, in that a Pageant running
with administrator privilege will now serve keys to an unprivileged
PuTTY running as the same user who started Pageant. Owen and I think
this isn't a problem (in particular, it will still not serve keys to a
_different_ user).
More importantly, making the Pageant client and server code work the
same way means that PuTTY and Pageant can still talk to each other
when UAC is turned off, which we've had several reports of r9043
having broken.
[originally from svn r9178]
[r9043 == 05f22632eb]
should solve some of the SID-mismatch issues we've occasionally had
reported. Because it's a modification on the client side, it doesn't
affect the security of Pageant itself.
[originally from svn r9043]
behave like a pointer. In particular, the right thing to set a
HANDLE to to indicate that it's invalid is INVALID_HANDLE_VALUE, not
NULL. Crack down on sloppy use of NULL HANDLEs across all Windows
code.
(There is one oddity, which is that {Create,Open}FileMapping are
documented to return a NULL HANDLE instead of INVALID_HANDLE_VALUE
on failure. Shrug. If MS want to be inconsistent, I suppose I have
to live with it.)
[originally from svn r6833]
long last to move all the Windows-specific source files down into a
`windows' subdirectory. Only platform-specific files remain at the
top level. With any luck this will act as a hint to anyone still
contemplating sending us a Windows-centric patch...
[originally from svn r4792]
Simon Tatham
Jacob Nevins