This is a small cleanup that removes a couple of copies of some boring
stubs, in favour of having just one copy that you can link against.
Unix Pageant can't currently use this, because it's in a precarious
state of _nearly_ having a random number generator: it links against
sshprng but not sshrand, and only uses it for the randomised keypress
acknowledgments in the GUI askpass prompt. But that means it does use
uxnoise, unlike the truly randomness-free tools.
There aren't quite as many of these as there are on Unix, but Windows
Plink and PSFTP still share some suspiciously similar-looking code.
Now they're both clients of wincliloop.c.
Unix Plink, Unix Pageant in server mode, Uppity, and the post-
connection form of PSFTP's command-line reading code all had very
similar loops in them, which run a pollwrapper and mediate between
that, timers, and toplevel callbacks. It's long past time the common
code between all of those became a reusable shared routine.
So, this commit introduces uxcliloop.c, and turns all the previous
copies of basically the same loop into a call to cli_main_loop with
various callback functions to configure the parts that differ.
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.
Spotted by Leak Sanitiser, while I was investigating the PSFTP /
proftpd issue mentioned in the previous commit (with ASan on as
usual).
The two very similar loops that read PSFTP commands from the
interactive prompt and a batch file differed in one respect: only one
of them remembered to free the command afterwards. Now I've moved the
freeing code out into a subroutine that both loops can use.
Ever since I reworked the SSH code to have multiple internal packet
queues, there's been a long-standing FIXME in ssh_sendbuffer() saying
that we ought to include the data buffered in those queues as part of
reporting how much data is buffered on standard input.
Recently a user reported that 'proftpd', or rather its 'mod_sftp'
add-on that implements an SFTP-only SSH server, exposes a bug related
to that missing piece of code. The xfer_upload system in sftp.c starts
by pushing SFTP write messages into the SSH code for as long as
sftp_sendbuffer() (which ends up at ssh_sendbuffer()) reports that not
too much data is buffered locally. In fact what happens is that all
those messages end up on the packet queues between SSH protocol
layers, so they're not counted by sftp_sendbuffer(), so we just keep
going until there's some other reason to stop.
Usually the reason we stop is because we've filled up the SFTP
channel's SSH-layer window, so we need the server to send us a
WINDOW_ADJUST before we're allowed to send any more data. So we return
to the main event loop and start waiting for reply packets. And when
the window is moderate (e.g. OpenSSH currently seems to present about
2MB), this isn't really noticeable.
But proftpd presents the maximum-size window of 2^32-1 bytes, and as a
result we just keep shovelling more and more packets into the internal
packet queues until PSFTP has grown to 4GB in size, and only then do
we even return to the event loop and start actually sending them down
the network. Moreover, this happens again at rekey time, because while
a rekey is in progress, ssh2transport stops emptying the queue of
outgoing packets sent by its higher layer - so, again, everything just
keeps buffering up somewhere that sftp_sendbuffer can't see it.
But this commit fixes it! Each PacketProtocolLayer now provides a
vtable method for asking how much data it currently has queued. Most
of them share a default implementation which just returns the newly
added total_size field from their pq_out; the exception is
ssh2transport, which also has to account for data queued in its higher
layer. And ssh_sendbuffer() adds that on to the quantity it already
knew about in other locations, to give a more realistic idea of the
currently buffered data.
The queue-node structure shared between PktIn and PktOut now has a
'formal_size' field, which is initialised appropriately by the various
packet constructors. And the PacketQueue structure has a 'total_size'
field which tracks the sum of the formal sizes of all the packets on
the queue, and is automatically updated by the push, pop and
concatenate functions.
No functional change, and nothing uses the new fields yet: this is
infrastructure that will be used in the next commit.
If the agent client code doesn't even manage to read a full response
message at all (for example, because the agent it's talking to is
Pageant running in debug mode and you just ^Ced it or it crashed,
which is what's been happening to me all afternoon), then previously,
the userauth code would loop back round to the top of the main loop
without having actually sent any request, so the client code would
deadlock waiting for a response to nothing.
I've often found it useful that you can make symlinks to Unix-domain
sockets, and then connect() on the symlink path will redirect to the
original socket.
This commit adds an option to Unix Pageant which will make it symlink
its socket path to a link location of your choice. My initial use case
is when running Pageant in debug mode during development: if you run a
new copy of it every few minutes after making a code change, then it's
annoying to have it change its socket path every time so you have to
keep pasting its setup command into your test shell. Not any more! Now
you can run 'pageant --debug --symlink fixed-location', and then your
test shell can point its SSH_AUTH_SOCK at the fixed location all the
time.
There are very likely other use cases too, but that's the one that
motivated me to add the option.
This adds an extension request to the agent protocol (named in our
private namespace, naturally) which allows you to upload a key file in
the form of a string containing an entire .ppk file. If the key is
encrypted, then Pageant stores it in such a way that it will show up
in the key list, and on the first attempt to sign something with it,
prompt for a passphrase (if it can), decrypt the key, and then answer
the request.
There are a lot of rough edges still to deal with, but this is good
enough to have successfully answered one request, so it's a start.
This is the easiest place to implement _something_ that will work as a
runtime passphrase prompt, which means I get to use it to test the
code I'm about to add to the Pageant core to make use of those
prompts. Once that's working, we can think about adding prompts for
the 'proper' usage modes.
The debug-mode passphrase prompts are implemented by simply reading
from standard input, having emitted a log message mentioning that a
prompt is impending. We put standard input into non-echoing mode, but
otherwise don't print any visible prompt (because standard output will
in general receive further log messages, which would break it anyway).
This is only just good enough for initial testing. In particular, it
won't cope if two prompts are in flight at the same time. But good
enough for initial testing is better than nothing!
This begins to head towards the goal of storing a key file encrypted
in Pageant, and decrypting it on demand via a GUI prompt the first
time a client requests a signature from it. That won't be a facility
available in all situations, so we have to be able to return failure
from the prompt.
More precisely, there are two versions of this API, one in
PageantClient and one in PageantListenerClient: the stream
implementation of PageantClient implements the former API and hands it
off to the latter. Windows Pageant has to directly implement both (but
they will end up funnelling to the same function within winpgnt.c).
NFC: for the moment, the new API functions are never called, and every
implementation of them returns failure.
I don't really know why it was still in cmdgen.c at all. There's no
reason it shouldn't live in its own source file, and keep cmdgen.c for
the actual code of the key generation program!
This reworks the cmdgen main program so that it loads the input file
into a LoadedFile right at the start, and then every time it needs to
do something with the contents, it calls one of the API functions
taking a BinarySource instead of one taking a Filename.
The usefulness of this is that now we can read from things that aren't
regular files, and can't be rewound or reopened. In particular, the
filename "-" is now taken (per the usual convention) to mean standard
input.
So now you can pipe a public or private key file into cmdgen's
standard input and have it do something useful. For example, I was
recently experimenting with the SFTP-only SSH server that comes with
'proftpd', which keeps its authorized_keys file in RFC 4716 format
instead of the OpenSSH one-liner format, and I found I wanted to do
grep 'my-key-comment' ~/.ssh/authorized_keys | puttygen -p -
to quickly get hold of my existing public key to put in that file. But
I had to go via a temporary file to make that work, because puttygen
couldn't read from standard input. Next time, it will be able to!
Like sshpubk.c before it, the loading side of import.c now works by
first slurping the whole input file into a LoadedFile structure, and
then using the BinarySource system to parse the file contents entirely
in memory.
The old API is still present and works the same as ever, but now we
also provide a secondary API that can import a foreign key file from a
BinarySource.
This is rather a superficial conversion: I've replaced all calls to
fgetline() with a local static function bsgetline() which presents
more or less the same interface for a BinarySource: that is, it still
returns a dynamically allocated string containing the line of text, so
that the followup code could change as little as possible. It would be
nice to come back in future and modernise this code to use ptrlens
throughout, saving all the unnecessary allocations.
I'm about to use it in cmdgen for a minor UI improvement. Also, I
expect it to be useful in the Pageant client code sooner or later.
While I'm here, I've also tweaked its UI a little so that it reports a
more precise error, and provided a version that can read from an
already open stdio stream.
In the previous trawl of this, I didn't bother with the statics in
main-program modules, on the grounds that my main aim was to avoid
'library' objects (shared between multiple programs) from polluting
the global namespace. But I think it's worth being more strict after
all, so this commit adds 'static' to a lot more file-scope variables
that aren't needed outside their own module.
Now it's no longer used, we can get rid of it, and better still, get
rid of every #define PUTTY_DO_GLOBALS in the many source files that
previously had them.
We now have no remaining things in header files that switch from being
a declaration to a definition depending on an awkward #define at the
point of including that header. There are still a few mutable
variables with external linkage, but at least now each one is defined
in a specific source file file appropriate to its purpose and context.
The remaining globals as of this commit were:
- 'logctx' and 'term', which never needed to be globals in the first
place, because they were never actually shared between source
files. Now 'term' is just a static in window.c, and 'logctx' is a
static in each of that and winplink.c.
- 'hinst', which still has external linkage, but is now defined
separately in each source file that sets it up (i.e. those with a
WinMain)
- osMajorVersion, osMinorVersion and osPlatformId, whose definitions
now live in winmisc.c alongside the code which sets them up.
(Actually they were defined there all along, it turns out, but
every toolchain I've built with has commoned them together with the
version defined by the GLOBAL in the header.)
- 'hwnd', which nothing was actually _using_ any more after previous
commits, so all this commit had to do was delete it.
The declarations in header files now use ordinary 'extern'. That means
I have to arrange to put definitions matching those declarations in
the appropriate modules; so I've made a macro DEFINE_WINDOWS_FUNCTION
which performs a definition matching a prior DECLARE_WINDOWS_FUNCTION
(and reusing the typedef made by the latter).
This applies not only to the batch of functions that were marked
GLOBAL in winstuff.h, but also the auxiliary sets marked
WINCAPI_GLOBAL and WINSECUR_GLOBAL in wincapi.h and winsecur.h
respectively.
This was the difficult part of cleaning up that global variable. The
main Windows PuTTY GUI is split between source files, so that _does_
actually need to refer to the main window from multiple places.
But all the places where windlg.c needed to use 'hwnd' are seat
methods, so they were already receiving a Seat pointer as a parameter.
In other words, the methods of the Windows GUI Seat were already split
between source files. So it seems only fair that they should be able
to share knowledge of the seat's data as well.
Hence, I've created a small 'WinGuiSeat' structure which both window.c
and windlg.c can see the layout of, and put the main terminal window
handle in there. Then the seat methods implemented in windlg.c, like
win_seat_verify_ssh_host_key, can use container_of to turn the Seat
pointer parameter back into the address of that structure, just as the
methods in window.c can do (even though they currently don't need to).
(Who knows: now that it _exists_, perhaps that structure can be
gradually expanded in future to turn it into a proper encapsulation of
all the Windows frontend's state, like we should have had all
along...)
I've also moved the Windows GUI LogPolicy implementation into the same
object (i.e. WinGuiSeat implements both traits at once). That allows
win_gui_logging_error to recover the same WinGuiSeat from its input
LogPolicy pointer, which means it can get from there to the Seat facet
of the same object, so that I don't need the extern variable
'win_seat' any more either.
Windows Pageant doesn't really have a 'main window' any more, ever
since I separated the roles of system-tray management and IPC receiver
into two different hidden windows managed by different threads. So it
was already silly to be storing one of them in the global 'HWND hwnd'
variable, because it's no longer obvious which it should be.
So there's now a static variable 'traywindow' within winpgnt.c which
it uses in place of the global 'hwnd'.
The GUI version of pgp_fingerprints() is now a differently named
function that takes a parent HWND as a parameter, and so does my
help-enabled wrapper around MessageBox.
It's now a static in the main source file of each application that
uses it, and isn't accessible from any other source file unless the
main one passes it by reference.
In fact, there were almost no instances of the latter: only the
config-box functions in windlg.c were using 'conf' by virtue of its
globalness, and it's easy to make those take it as a parameter.
It's silly to set it at each call site of restrict_process_acl() if
that function returns success! More sensible to have it be a flag in
the same source file as restrict_process_acl(), set as an automatic
_side effect_ of success.
I've renamed the variable itself, and the global name 'restricted_acl'
is now a query function that asks winsecur.c whether that operation
has been (successfully) performed.
Apparently it's been set on Telnet for the entire lifetime of PSCP. It
can't have caused any trouble, or we'd have noticed by now, but it
still seems silly to set it to something that PSCP clearly can't
handle!
These global variables are only ever used by load_settings, which uses
them to vary the default protocol and port number in the absence of
any specification elsewhere. So there's no real need for them to be
universally accessible via the awkward GLOBAL mechanism: they can be
statics inside settings.c, with accessor functions that can set them.
That was the last GLOBAL in putty.h, so I've removed the definition of
the macro GLOBAL itself as well. There are still some GLOBALs in the
Windows subdirectory, though.
I haven't managed to make this one _not_ be a mutable variable, but at
least it's not global across all tools any more: it lives in cmdline.c
along with the code that decides what to set it to, and cmdline.c
exports a query method to ask for its value.
This mutable global was never actually used at all, _even_ in history!
Commit 1a03fa929 introduced it, as part of the jump list support, but
even that commit never _read_ from the variable - it only assigned to
it. I have to guess that it was part of an earlier draft of the jump
lists patch and ended up orphaned in the final version.
Another ugly mutable global variable gone: now, instead of this
variable being defined in cmdline.c and written to by everyone's
main(), it's defined _alongside_ everyone's main() as a constant, and
cmdline.c just refers to it.
A bonus is that now nocmdline.c doesn't have to define it anyway for
tools that don't use cmdline.c. But mostly, it didn't need to be
mutable, so better for it not to be.
While I'm at it, I've also fiddled with the bit flags that go in it,
to define their values automatically using a list macro instead of
manually specifying each one to be a different power of 2.
This is another piece of the old 2003 attempt at async agent requests.
Nothing ever calls this function (in particular, the new working
version of async-agent doesn't need it). Remove it completely, and all
its special-window-message implementations too.
(If we _were_ still using this function, then it would surely be
possible to fold it into the more recently introduced general
toplevel-callback system, and get rid of all this single-use special
code. But we're not, so removing it completely is even easier.)
In particular, this system was the only reason why Windows Plink paid
any attention to its message queue. So now I can make it call plain
WaitForMultipleObjects instead of MsgWaitForMultipleObjects.
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.)
This was the easiest flag to remove: nothing ever checks it at all!
It was part of an abandoned early attempt to make Pageant requests
asynchronous. The flag was added in commit 135abf244 (April 2003); the
code that used it was #ifdef-ed out in commit 98d735fde (January 2004),
and removed completely in commit f864265e3 (January 2017).
We now have an actually working system for async agent requests on
Windows, via the new named-pipe IPC. And we also have a perfectly good
way to force a particular agent request to work synchronously: just
pass NULL as the callback function pointer. All of that works just
fine, without ever using this flag. So begone!
It really had no need to be a global process flag at all: it's used to
signal to uxcons.c that stderr_tty_init() ought to check whether
stderr is a tty (in order to save its termios across interactive
prompts and so forth). But the only _caller_ of stderr_tty_init is
Unix Plink, which always sets that flag! So we already have a
perfectly good system for signalling to uxcons.c that you'd like
stderr_tty_init to do something: _whether you call it or not_.
The global 'int flags' has always been an ugly feature of this code
base, and I suddenly thought that perhaps it's time to start throwing
it out, one flag at a time, until it's totally unused.
My first target is FLAG_VERBOSE. This was usually set by cmdline.c
when it saw a -v option on the program's command line, except that GUI
PuTTY itself sets it unconditionally on startup. And then various bits
of the code would check it in order to decide whether to print a given
message.
In the current system of front-end abstraction traits, there's no
_one_ place that I can move it to. But there are two: every place that
checked FLAG_VERBOSE has access to either a Seat or a LogPolicy. So
now each of those traits has a query method for 'do I want verbose
messages?'.
A good effect of this is that subsidiary Seats, like the ones used in
Uppity for the main SSH server module itself and the server end of
shell channels, now get to have their own verbosity setting instead of
inheriting the one global one. In fact I don't expect any code using
those Seats to be generating any messages at all, but if that changes
later, we'll have a way to control it. (Who knows, perhaps logging in
Uppity might become a thing.)
As part of this cleanup, I've added a new flag to cmdline_tooltype,
called TOOLTYPE_NO_VERBOSE_OPTION. The unconditionally-verbose tools
now set that, and it has the effect of making cmdline.c disallow -v
completely. So where 'putty -v' would previously have been silently
ignored ("I was already verbose"), it's now an error, reminding you
that that option doesn't actually do anything.
Finally, the 'default_logpolicy' provided by uxcons.c and wincons.c
(with identical definitions) has had to move into a new file of its
own, because now it has to ask cmdline.c for the verbosity setting as
well as asking console.c for the rest of its methods. So there's a new
file clicons.c which can only be included by programs that link
against both cmdline.c _and_ one of the *cons.c, and I've renamed the
logpolicy to reflect that.
Most of our makefiles use -Werror, and it seems silly not to do the
same for the Windows clang-cl builds.
The w32old build was not warning-clean, for a reason I can't do much
about: something in the VS2003 headers gives a lot of warnings about
mismatched #pragma pack push/pop between system header files. I don't
see anything much I can do about that except to squelch the warning
with -Wno-pragma-pack.
These were just too footling for even me to bother splitting up into
multiple commits:
- a couple of int -> size_t changes left out of the big-bang commit
0cda34c6f
- a few 'const' added to pointer-type casts that are only going to be
read from (leaving out the const provokes a warning if the pointer
was const _before_ the cast)
- a couple of 'return' statements trying to pass the void return of
one function through to another.
- another missing (void) in a declaration in putty.h (but this one
didn't cause any knock-on confusion).
- a few tweaks to macros, to arrange that they eat a semicolon after
the macro call (extra do ... while (0) wrappers, mostly, and one
case where I had to do it another way because the macro included a
variable declaration intended to remain in scope)
- reworked key_type_to_str to stop putting an unreachable 'break'
statement after every 'return'
- removed yet another type-check of a function loaded from a Windows
system DLL
- and finally, a totally spurious semicolon right after an open brace
in mainchan.c.
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.)
These are all intended to ensure that the declarations of things in
header files are in scope where the same thing is subsequently
defined, to make it harder to define it in a way that doesn't match.
(For example, the new #include in winnet.c would have caught the
just-fixed mis-definition of platform_get_x11_unix_address.)
MD5_Simple and SHA512_Simple are superseded by the general
hash_simple() in sshauxcrypt.c. The SHA-1 and SHA-256 analogues of
those functions were already removed as a side effect of reorganising
their source files for hardware acceleration, but the hashes I didn't
rewrite managed to keep their 'Simple' APIs.
mp_neg was never used in the first place. I wrote it as an obvious
companion to mp_neg_into, but since I never put either of those in the
header file, none of the rest of the code ever called them; and
internally to mpint.c, only mp_neg_into is used out of the two.
(Which makes sense, because mp_int is fundamentally an unsigned type,
so mp_neg has weird semantics - it will give you not -x but 2^k-x for
some not-entirely-specified k. mp_neg_into is actually more coherent,
because _you_ get to control k by your choice of the target mp_int.)
A recent test-compile at high warning level points out that if you
define a macro with a ... at the end of the parameter list, then every
call should at least include the comma before the variadic part. That
is, if you #define MACRO(x,y,...) then you shouldn't call MACRO(1,2)
with no comma after the 2. But that's what I had done in one of my
definitions of FUNC0 in the fiddly testcrypt system.
In a similar vein, it's a mistake to use the preprocessor 'defined'
operator when it's expanded from another macro. Adjusted the setup of
BB_OK in mpint_i.h to avoid doing that.
(Neither of these has yet caused a problem in any real compile, but
best to fix them before they do.)
Similarly to the previous commit, this function had an inconsistent
parameter list between Unix and Windows, because the Windows source
file that defines it (winnet.c) didn't include ssh.h where its
prototype lives, so the compiler never checked.
Luckily, the discrepancy was that the Windows version of the function
was declared as taking an extra parameter which it ignored, so the fix
is very easy.
In commit b4c8fd9d8 which introduced the Seat trait, I got a bit
confused about the prototype of new_prompts(). Previously it took a
'Frontend *' parameter; I edited the call sites to pass a 'Seat *'
instead, but the actual function definition takes no parameters at all
- and rightly so, because the 'Frontend *' inside the prompts_t has
been removed and _not_ replaced with a 'Seat *', so the constructor
would have nothing to do with such a thing anyway.
But I wrote the function declaration in putty.h with '()' rather than
'(void)' (too much time spent in C++), and so the compiler never
spotted the mismatch.
Now new_prompts() is consistently nullary everywhere it appears: the
prototype in the header is a proper (void) one, and the call sites
have been modified to not pointlessly give it a Seat or null pointer.
