In fact, those functions don't even exist any more. The only way to
get data into a primitive hash state is via the new put_* system. Of
course, that means put_data() is a viable replacement for every
previous call to one of the per-hash update functions - but just
mechanically doing that would have missed the opportunity to simplify
a lot of the call sites.
This centralises a few things that multiple header files were
previously defining, and were protecting against each other's
redefinition with ifdefs - small things like structs and typedefs. Now
all those things are in a defs.h which is by definition safe to
include _first_ (out of all the codebase-local headers) and only need
to be defined once.
Lots of functions had really generic names (like 'makekey'), or names
that missed out an important concept (like 'rsakey_pubblob', which
loads a public blob from a _file_ and doesn't generate it from an
in-memory representation at all). Also, the opaque 'int order' that
distinguishes the two formats of public key blob is now a mnemonic
enumeration, and while I'm at it, rsa_ssh1_public_blob takes one of
those as an extra argument.
NFC for the moment, because the bufchain is always specially
constructed to hold exactly the same data that would have been passed
in to the function as a (pointer,length) pair. But this API change
allows get_userpass_input to express the idea that it consumed some
but not all of the data in the bufchain, which means that later on
I'll be able to point the same function at a longer-lived bufchain
containing the full stream of keyboard input and avoid dropping
keystrokes that arrive too quickly after the end of an interactive
password prompt.
NFC: this is a preliminary refactoring, intended to make my life
easier when I start changing around the APIs used to pass user
keyboard input around. The fewer functions even _have_ such an API,
the less I'll have to do at that point.
Thanks to Jiri Kaspar for sending this patch (apart from the new docs
section, which is in my own words), which implements a feature we've
had as a wishlist item ('utf8-plus-vt100') for a long time.
I was actually surprised it was possible to implement it in so few
lines of code! I'd forgotten, or possibly never noticed in the first
place, that even in UTF-8 mode PuTTY not only accepts but still
_processes_ all the ISO 2022 control sequences and shift characters,
and keeps running track of all the same state in term->cset and
term->cset_attrs that it tracks in IS0-2022-enabled modes. It's just
that in UTF-8 mode, at the very last minute when a character+attribute
pair is about to be written into the terminal's character buffer, it
deliberately ignores the contents of those variables.
So all that was needed was a new flag checked at that last moment
which causes it not quite to ignore them after all, and bingo,
utf8-plus-vt100 is supported. And it works no matter which ISO 2022
sequences you're using; whether you're using ESC ( 0 to select the
line drawing set directly into GL and ESC ( B to get back when you're
done, or whether you send a preliminary ESC ( B ESC ) 0 to get GL/GR
to be ASCII and line drawing respectively so you can use SI and SO as
one-byte mode switches thereafter, both work just as well.
This implementation strategy has a couple of consequences, which I
don't think matter very much one way or the other but I document them
just in case they turn out to be important later:
- if an application expecting this mode has already filled your
terminal window with lqqqqqqqqk, then enabling this mode in Change
Settings won't retroactively turn them into the line drawing
characters you wanted, because no memory is preserved in the screen
buffer of what the ISO 2022 state was when they were printed. So
the application still has to do a screen refresh.
- on the other hand, if you already sent the ESC ( 0 or whatever to
put the terminal _into_ line drawing mode, and then you turn on
this mode in Change Settings, you _will_ still be in line drawing
mode, because the system _does_ remember your current ISO 2022
state at all times, whether it's currently applying it to output
printing characters or not.
This is a heavily edited (by me) version of a patch originally due to
Nico Williams and Viktor Dukhovni. Their comments:
* Don't delegate credentials when rekeying unless there's a new TGT
or the old service ticket is nearly expired.
* Check for the above conditions more frequently (every two minutes
by default) and rekey when we would delegate credentials.
* Do not rekey with very short service ticket lifetimes; some GSSAPI
libraries may lose the race to use an almost expired ticket. Adjust
the timing of rekey checks to try to avoid this possibility.
My further comments:
The most interesting thing about this patch to me is that the use of
GSS key exchange causes a switch over to a completely different model
of what host keys are for. This comes from RFC 4462 section 2.1: the
basic idea is that when your session is mostly bidirectionally
authenticated by the GSSAPI exchanges happening in initial kex and
every rekey, host keys become more or less vestigial, and their
remaining purpose is to allow a rekey to happen if the requirements of
the SSH protocol demand it at an awkward moment when the GSS
credentials are not currently available (e.g. timed out and haven't
been renewed yet). As such, there's no need for host keys to be
_permanent_ or to be a reliable identifier of a particular host, and
RFC 4462 allows for the possibility that they might be purely
transient and only for this kind of emergency fallback purpose.
Therefore, once PuTTY has done a GSS key exchange, it disconnects
itself completely from the permanent host key cache functions in
storage.h, and instead switches to a _transient_ host key cache stored
in memory with the lifetime of just that SSH session. That cache is
populated with keys received from the server as a side effect of GSS
kex (via the optional SSH2_MSG_KEXGSS_HOSTKEY message), and used if
later in the session we have to fall back to a non-GSS key exchange.
However, in practice servers we've tested against do not send a host
key in that way, so we also have a fallback method of populating the
transient cache by triggering an immediate non-GSS rekey straight
after userauth (reusing the code path we also use to turn on OpenSSH
delayed encryption without the race condition).
Now its remit is widened to include not just the character-classes
list box, but also anything else related specifically to _copying_
rather than _pasting_, i.e. the terminal -> clipboard direction.
This allows me to move the Windows-specific 'write RTF to clipboard'
option into the newly named Copy panel, which gets it _out_ of the
main Selection panel which had just overflowed due to the new option
added by the previous commit.
(It looks a little asymmetric that there's no corresponding Paste
panel now! But since it would currently contain a single checkbox,
I'll wait until there's more to put in it...)
Ahem. I _spotted_ this in code review, and forgot to make the change
before pushing!
Because it's legitimate for a C implementation to define 'NULL' so
that it expands to just 0, it follows that if you use NULL in a
variadic argument list where the callee will expect to extract a
pointer, you run the risk of putting an int-sized rather than
pointer-sized argument on the list and causing the consumer to get out
of sync. So you have to add an explicit cast.
The PuTTY GUIs (Unix and Windows) maintain an in-memory event log
for display to users as they request. This uses ints for tracking
eventlog size, which is subject to memory exhaustion and (given
enough heap space) overflow attacks by servers (via, e.g., constant
rekeying).
Also a bounded log is more user-friendly. It is rare to want more
than the initial logging and the logging from a few recent rekey
events.
The Windows fix has been tested using Dr. Memory as a valgrind
substitute. No errors corresponding to the affected code showed up.
The Dr. Memory results.txt was split into a file per-error and then
grep Error $(grep -l windlg *)|cut -d: -f3-|sort |uniq -c
was used to compare. Differences arose from different usage of the GUI,
but no error could be traced to the code modified in this commit.
The Unix fix has been tested using valgrind. We don't destroy the
eventlog_stuff eventlog arrays, so we can't be entirely sure that we
don't leak more than we did before, but from code inspection it looks
like we don't (and anyways, if we leaked as much as before, just without
the integer overflow, well, that's still an improvement).
Windows, annoyingly, doesn't seem to have any bit flag in
GetFileAttributes which distinguishes a regular file (which can be
truncated destructively) from a named pipe (which can't).
Fortunately, I'm saved from needing one by the policy I came up with
in the previous commit, in which I don't bother to ask about
truncating a file if it already has zero length, because it would make
no difference anyway. Named pipes do show up as zero-length in
GetFileAttributesEx, so they get treated like zero-length regular
files by this change and it's all good.
On all platforms, you can now configure which clipboard the mouse
pastes from, which clipboard Ctrl-Ins and Shift-Ins access, and which
Ctrl-Shift-C and Ctrl-Shift-V access. In each case, the options are:
- nothing at all
- a clipboard which is implicitly written by the act of mouse
selection (the PRIMARY selection on X, CLIP_LOCAL everywhere else)
- the standard clipboard written by explicit copy/paste UI actions
(CLIPBOARD on X, the unique system clipboard elsewhere).
Also, you can control whether selecting text with the mouse _also_
writes to the explicitly accessed clipboard.
The wording of the various messages changes between platforms, but the
basic UI shape is the same everywhere.
This lays some groundwork for making PuTTY's cut and paste handling
more flexible in the area of which clipboard(s) it reads and writes,
if more than one is available on the system.
I've introduced a system of list macros which define an enumeration of
integer clipboard ids, some defined centrally in putty.h (at present
just a CLIP_NULL which never has any text in it, because that seems
like the sort of thing that will come in useful for configuring a
given copy or paste UI action to be ignored) and some defined per
platform. All the front end functions that copy and paste take a
clipboard id, and the Terminal structure is now configured at startup
to tell it which clipboard id it should paste from on a mouse click,
and which it should copy from on a selection.
However, I haven't actually added _real_ support for multiple X11
clipboards, in that the Unix front end supports a single CLIP_SYSTEM
regardless of whether it's in OS X or GTK mode. So this is currently a
NFC refactoring which does nothing but prepare the way for real
changes to come.
It's actually a function specific to the Windows front end, and has
been all along - but I've only just noticed that no other front end
either uses it or defines it.
Previously, both the Unix and Windows front ends would respond to a
paste action by retrieving data from the system clipboard, converting
it appropriately, _storing_ it in a persistent dynamic data block
inside the front end, and then calling term_do_paste(term), which in
turn would call back to the front end via get_clip() to retrieve the
current contents of that stored data block.
But, as far as I can tell, this was a completely pointless mechanism,
because after a data block was written into this storage area, it
would be immediately used for exactly one paste, and then never
accessed again until the next paste action caused it to be freed and
replaced with a new chunk of pasted data.
So why on earth was it stored persistently at all, and why that
callback mechanism from frontend to terminal back to frontend to
retrieve it for the actual paste action? I have no idea. This change
removes the entire system and replaces it with the completely obvious
alternative: the character-set-converted version of paste data is
allocated in a _local_ variable in the frontend paste functions,
passed directly to term_do_paste which now takes (buffer,length)
parameters, and freed immediately afterwards. get_clip() is gone.
These include an unused variable left over from the command-line
refactoring; an explicit referencing of the module handle for
sspicli.dll which we really do deliberately load and then don't
(directly) use; a missing pointer-type cast in the Windows handle
socket code; and two 32/64 bit integer size mismatches in the types of
functions I was importing from system API DLLs.
The last of those are a bit worrying, and suggest to me that after
going to all that trouble to add type-checking of those runtime
imports in commit 49fb598b0, I might have only checked the resulting
compiler output in a 32-bit build and not a 64-bit one. Oops!
This is another piece of long-overdue refactoring similar to the
recent commit e3796cb77. But where that one dealt with normalisation
of stuff already stored _in_ a Conf by whatever means (including, in
particular, handling a user typing 'username@host.name' into the
Hostname box of the GUI session dialog box), this one deals with
handling argv entries and putting them into the Conf.
This isn't exactly a pure no-functional-change-at-all refactoring. On
the other hand, it isn't a full-on cleanup that completely
rationalises all the user-visible behaviour as well as the code
structure. It's somewhere in between: I've preserved all the behaviour
quirks that I could imagine a reason for having intended, but taken
the opportunity to _not_ faithfully replicate anything I thought was
clearly just a bug.
So, for example, the following inconsistency is carefully preserved:
the command 'plink -load session nextword' treats 'nextword' as a host
name if the loaded session hasn't provided a hostname already, and
otherwise treats 'nextword' as the remote command to execute on the
already-specified remote host, but the same combination of arguments
to GUI PuTTY will _always_ treat 'nextword' as a hostname, overriding
a hostname (if any) in the saved session. That makes some sense to me
because of the different shapes of the overall command lines.
On the other hand, there are two behaviour changes I know of as a
result of this commit: a third argument to GUI PuTTY (after a hostname
and port) now provokes an error message instead of being silently
ignored, and in Plink, if you combine a -P option (specifying a port
number) with the historical comma-separated protocol selection prefix
on the hostname argument (which I'd completely forgotten even existed
until this piece of work), then the -P will now override the selected
protocol's default port number, whereas previously the default port
would win. For example, 'plink -P 12345 telnet,hostname' will now
connect via Telnet to port 12345 instead of to port 23.
There may be scope for removing or rethinking some of the command-
line syntax quirks in the wake of this change. If we do decide to do
anything like that, then hopefully having it all in one place will
make it easier to remove or change things consistently across the
tools.
A more or less identical piece of code to sanitise the CONF_host
string prior to session launch existed in Windows PuTTY and both
Windows and Unix Plink. It's long past time it was centralised.
While I'm here, I've added a couple of extra comments in the
centralised version, including one that - unfortunately - tries _but
fails_ to explain why a string of the form "host.name:1234" doesn't
get the suffix moved into CONF_port the way "user@host" moves the
prefix into CONF_username. Commit c1c1bc471 is the one I'm referring
to in the comment, and unfortunately it has an unexplained one-liner
log message from before I got into the habit of being usefully
verbose.
It's an incoherent concept! There should not be any such thing as an
error box that terminates the entire program but is not modal. If it's
bad enough to terminate the whole program, i.e. _all_ currently live
connections, then there's no point in permitting progress to continue
in windows other than the affected one, because all windows are
affected anyway.
So all previous uses of fatalbox() have become modalfatalbox(), except
those which looked to me as if they shouldn't have been fatal in the
first place, e.g. lingering pieces of error handling in winnet.c which
ought to have had the severity of 'give up on this particular Socket
and close it' rather than 'give up on the ENTIRE UNIVERSE'.
ATTR_REVERSE was being handled in the front ends, and was causing the
foreground and background colours to be switched. (I'm not completely
sure why I made that design decision; it might be purely historical,
but then again, it might also be because reverse video is one effect
on the fg and bg colours that must still be performed even in unusual
frontend-specific situations like display-driven monochrome mode.)
This affected both explicit reverse video enabled using SGR 7, and
also the transient reverse video arising from mouse selection. Thanks
to Markus Gans for reporting the bug in the latter, which when I
investigated it turned out to affect the former as well.
I've done this on a 'where possible' basis: in Windows paletted mode
(in case anyone is still using an old enough graphics card to need
that!) I simply haven't bothered, and will completely ignore the dim
flag.
Markus Gans points out that some applications which (not at all
unreasonably) don't trust $TERM to tell them the full capabilities of
their terminal will use the sequence "OSC 4 ; nn ; ? BEL" to ask for
the colour-palette value in position nn, and they may not particularly
care _what_ the results are but they will use them to decide whether
the right number of colour palette entries even exist.
Otherwise, moving the cursor (at least in active, filled-cell mode) on
to a true-coloured character cell causes it to vanish completely
because the cell's colours override the thing that differentiates the
cursor.
I'm not sure if any X11 monochrome visuals or Windows paletted display
modes are still around, but just in case they are, we shouldn't
attempt true colour on either kind of display.
I know some users don't like any colour _at all_, and we have a
separate option to turn off xterm-style 256-colour sequences, so it
seems remiss not to have an option to disable true colour as well.
This is a heavily rewritten version of a patch originally by Lorenz
Diener; it was tidied up somewhat by Christian Brabandt, and then
tidied up more by me. The basic idea is to add to the termchar
structure a pair of small structs encoding 24-bit RGB values, each
with a flag indicating whether it's turned on; if it is, it overrides
any other specification of fg or bg colour for that character cell.
I've added a test line to colours.txt containing a few example colours
from /usr/share/X11/rgb.txt. In fact it makes quite a good demo to run
the whole of rgb.txt through this treatment, with a command such as
perl -pe 's!^\s*(\d+)\s+(\d+)\s+(\d+).*$!\e[38;2;$1;$2;$3m$&\e[m!' rgb.txt
This causes PuTTY processes spawned from its system-tray menu to run
with the -restrict-acl option (or rather, the synonymous &R prefix
used by my auto-constructed command lines for easier parsing).
The previous behaviour of Pageant was never to pass -restrict-acl to
PuTTY, even when started with -restrict-acl itself; this is not
actually a silly thing to want to do, because Pageant might well have
more need of -restrict-acl than PuTTY (it stores longer-term and more
powerful secrets) and conversely PuTTY might have more need to _not_
restrict its ACL than Pageant (in that among the things enabled by an
unrestricted ACL are various kinds of accessibility software, which is
more useful on the more user-facing PuTTY than on Pageant).
But for those who want to lock everything down with every security
option possible (even though -restrict-acl is only an ad-hoc
precaution and cannot deliver any hard guarantees), this new option
should fill in the UI gap.
After a conversation this week with a user who tried to use it, it's
clear that Borland C can't build the up-to-date PuTTY without having
to make too many compromises of functionality (unsupported API
details, no 'long long' type), even above the issues that could be
worked round with extra porting ifdefs.
64-bit PuTTY should be loading gssapi64.dll, not gssapi32.dll. (In
contrast to the Windows system API DLLs, such as secur32.dll which is
also mentioned in the same source file; those keep the "32" in their
name whether we're in Win32 or Win64.)
Like every other toolchain I've tried, my Coverity scanning build has
its share of random objections to parts of my Windows API type-
checking system. I do wonder if that bright idea was worth the hassle
- but it would probably cost all the annoyance all over again to back
out now...
Ilya Shipitsin sent me a list of errors reported by a tool 'cppcheck',
which I hadn't seen before, together with some fixes for things
already taken off that list. This change picks out all the things from
the remaining list that I could quickly identify as actual errors,
which it turns out are all format-string goofs along the lines of
using a %d with an unsigned int, or a %u with a signed int, or (in the
cases in charset/utf8.c) an actual _size_ mismatch which could in
principle have caused trouble on a big-endian target.
Rather than loading some from spoolss.dll, which some MSDN documentation
suggests, but which doesn't work very well in practice.
(Also, remove an unused variable.)
I've also been experimenting recently with running Wix on Linux under
Mono, rather than running it on Windows in the obvious way. Wix under
Mono insists on forward slashes in pathnames, and it turns out that
Wix on Windows doesn't object to them either, so I can safely change
them over unconditionally and then my installer source will work in
both modes.
stdint.h is one of the set of standard C headers that has more to do
with the compiler than the library, and hence, clang provides its own
version of it, even when you're using it in clang-cl mode with Visual
Studio headers, and even when those headers are old enough not to have
a stdint.h of their own. So in clang builds, including stdint.h is
always the right way to get uintptr_t defined.
Patch due to Brian K. White; we now condition our own declaration of
DLL_DIRECTORY_COOKIE on whether the toolchain's headers had #defined
it already, rather than trying to guess the answer to that from
version-number macros.
(Apparently everything that defines DLL_DIRECTORY_COOKIE does it by
does seem to work in all my tests.)
When making select_result() return void (a2fb1d9), I removed a "return"
at the end of the FD_CLOSE case, causing a fallthrough into FD_ACCEPT
with hilarious (segfaulting) consequences. Re-instate the "return".
plug_receive() and plug_closing() return 0 or 1 depending on whether
they think the main connection has closed. It is not appropriate, as
handle_gotdata and handle_socket_unfreeze did, to treat them as
returning a backlog. In fact, plugs are unusual in PuTTY in not
reporting a backlog, but just calling into the socket to freeze and
unfreeze it as required.
It's redundant with back->connected(): only the SSH backend has a
receive function that can ever return 0, and whenever ssh_receive
returns 0 it has called ssh_do_close, which will cause future calls
to ssh_connected also to return 0. Similarly, all backend closing
functions ensure that future calls to their connected function will
return 0.
This too is not in the list of known DLLs on Windows 10. I don't know
of any actual viable hijacking attack based on it, which according to
my reading of MSDN (specifically, a rather vague hint in
https://msdn.microsoft.com/library/ff919712) _may_ be because we
mention the common controls assembly in our application manifest; but
better safe than sorry.
Now the entire list of remaining DLLs that PuTTY links against at load
time is a subset of the Win10 known DLLs list, so that _should_ mean
that everything we load before we've deployed our own defence
(SetDefaultDllDirectories) is defended against for us by Windows
itself.
The printing functions are split between winspool.drv and spoolss.dll
in a really weird way (who would have guessed that OpenPrinter and
ClosePrinter don't live in the same dynamic library?!), but _neither_
of those counts as a system 'known DLL', so linking against either one
of these at load time is again a potential DLL hijacking vector.
It's not on the default list of important system 'known DLLs' stored
at HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\KnownDLLs (see
https://isc.sans.edu/forums/diary/DLL+hijacking+vulnerabilities/9445/ )
which apparently makes it exempt from Windows's standard DLL hijacking
defence, i.e. if an executable links against it in the normal way then
that executable will be vulnerable to DLL hijacking from a file called
winmm.dll in the same directory as it.
The solution is to load it dynamically _after_ we've locked down our
DLL search path, which fortunately PuTTY's code base is well used to
doing already for other DLLs.
This gives me an extra safety-check against having mistyped one of the
function prototypes that we load at run time from DLLs: we verify that
the typedef we defined based on the prototype in our source code
matches the type of the real function as declared in the Windows
headers.
This was an idea I had while adding a pile of further functions using
this mechanism. It didn't catch any errors (either in the new
functions or in the existing collection), but that's no reason not to
keep it anyway now that I've thought of it!
In VS2015, this automated type-check works for most functions, but a
couple manage to break it. SetCurrentProcessExplicitAppUserModelID in
winjump.c can't be type-checked, because including <shobjidl.h> where
that function is declared would also bring in a load of other stuff
that conflicts with the painful manual COM declarations in winjump.c.
(That stuff could probably be removed now we're on an up-to-date
Visual Studio, on the other hand, but that's a separate chore.) And
gai_strerror, used in winnet.c, does _have_ an implementation in a
DLL, but the header files like to provide an inline version with a
different calling convention, which defeats this error-checking trick.
And in the older VS2003 that we still precautionarily build with,
several more type-checks have to be #ifdeffed out because the
functions they check against just aren't there at all.
If MIT Kerberos is installed, then using GetProcAddress to extract
GetUserNameExA() from secur32.dll causes Windows to implicitly load
sspicli.dll in turn - and it does it in a search-path-unclean way.
If we load it in our own way before that happens, then Windows doesn't
need to load it again and won't do so wrongly.
[SGT: tidied up commit message from original patch]
gssapi32.dll from MIT Kerberos as well as from Heimdal both load
further DLLs from their installation directories.
[SGT: I polished the original patch a bit, in particular replacing
manual memory allocation with dup_mb_to_wc. This required a Recipe
change to link miscucs.c and winucs.c into more of the tools.]
I scrolled past it just now and decided those open braces at the ends
of the lines are just too ugly to live. They originally got that way
when I put the whole source base through GNU indent, which as far as
I'm concerned is a horrible misfeature of indent!
This is apparently the other half of what we should have done when we
called SetCurrentProcessExplicitAppUserModelID at run time: it
associates to PuTTY's Start Menu shortcut the same identifier that we
give to the running PuTTY process, so that jump lists saved under the
latter will be visible to users mousing over the former.
I've also done the same thing to the desktop shortcut, just in case
that does anything useful.
In the sentdata callback function given to handle_output_new, the
'new_backlog' parameter can be negative, and if so, it represents a
Windows error code and not a backlog size at all. handle_sentdata was
not checking for this before passing it on to plug_sent.
While I'm looking at these two dialog boxes, I notice there's another
prominent difference between PuTTY's one and these: I also never got
round to adding the button to go to PuTTY's main website. Now added.
The current About boxes are too small to fit in all the buildinfo
data, in particular the source-control commit id. Apparently I forgot
to enlarge them when I enlarged the one in PuTTY proper.
(All the same information is nonetheless *present* in the box, but
there seems to be no way to scroll a static text control, so you can
only find that out by 'Select All' and copying to the clipboard.)
Anyway. Now resized to the same dimensions as the main PuTTY About
box. (Really I should centralise more definitions into a common
resource file, but there we go.)
Conflicts in the FAQ are fixed by incorporating Jacob's rewritten
post-0.68 version. (But owing to considerable git confusion I haven't
managed to get his name on to this commit anywhere.)
When we create a socket with socket() (in try_connect, sk_newlistener, and
ipv4_is_local_addr) also call SetHandleInformation to disable handle
inheritance for this socket. This fixes dup-sessions-dont-close.
This commit also updates the dumps of Plink's and PSCP's help output,
adding the -proxycmd option to both and the -shareexists option to
Plink.
(Or rather, _re_-adding the latter, since it was introduced in error
by commit 07af4ed10 due to a branch management error and hastily
removed again in 29e8c24f9. This time it really does match reality.)
When a handle socket is in THAWING state and handle_socket_unfreeze is
gradually passing the backlogged data on to the plug, the plug might
suddenly turn round and close the socket in the course of handling
plug_receive(), which means that handle_socket_unfreeze had better be
careful not to have had everything vanish out from under it when that
call returns. To solve this, I've added a 'deferred close' flag which
handle_socket_unfreeze can set around its call to plug_receive, and
handle_socket_close will detect that and not actually free the socket,
instead leaving that for handle_socket_unfreeze to do under its own
control.
When called with -V to ask for our version, return 0 rather than 1.
This is the usual behaviour observed by ssh(1) and other Unix commands.
Also use exit() rather than cleanup_exit() in pscp.c and psftp.c ; at
this point we have nothing to cleanup!
Firstly, I had asserted that data would never arrive on a handle
socket in state FREEZING, which is just an error, because FREEZING is
precisely the state of not being quite frozen _yet_ because one last
read is still expected to arrive from the winhandl.c reading subthread
which it's too late to cancel. I meant to assert that it wasn't
FROZEN.
Secondly, when the handle socket was in state FREEZING, I failed to
actually _set_ it to FROZEN.
And thirdly, when the handle socket starts thawing again (i.e. there's
now outgoing buffer space so we can start sending our backlogged
data), I forgot to ever call bufchain_consume, so that the same block
of data would get sent repeatedly.
I can only assume that nothing I've ever done has actually exercised
this code!
I think all of the cases in this switch must have originally said
(shift_state ? 'this' : 'that'), and in all but the VK_NUMPAD5 case
the two options were different, and I left VK_NUMPAD5 containing a
redundant ?: just to make it line up in a nice table with the others.
But now the others all have more options than that because I had to
support Ctrl as well as Shift modifiers, so there's no reason to have
that silly ?: lingering around (and it annoys Coverity).
Avoided referring to some functions and header files that aren't there
in the winelib world (_vsnprintf, _stricmp, SecureZeroMemory,
multimon.h), and worked around a really amazingly annoying issue in
which Winelib objects to you using the type 'fd_set' unless you
included winsock2.h before stdlib.h.
The loops that were supposed to count up the number of buttons in the
variadic argument list forgot to increment the counter.
On the other hand, these functions aren't actually _used_ anywhere in
the current code - looks as if commit 616c837cf was the last time they
were seen - but manual dialog stuff like PuTTYgen might yet find a use
for them in future.
Coverity observes that sometimes 'struct tm' can have other fields
(e.g. glibc's tm_gmtoff), so it's as well to make sure we initialise
the whole thing to zero.
Assignments that are overwritten shortly afterwards and never used,
and a completely unused variable. Also, the bogus array access in
testbn.c could have actually accessed one beyond the array limit
(though of course it's only in a test harness).
Partly to reassure the user that they got what they asked for, and
partly so that's a clue for us in the logs when we get bug reports.
This involved repurposing platform_psftp_post_option_setup() (no longer
used since e22120fe) as platform_psftp_pre_conn_setup(), and moving it
to after logging is set up.
These are benign, I think. clang warns about casting non-pointer-sized
integers to pointers, but the Windows API actually does sometimes
involve values that are either pointers or _small_ integers, so in the
two cases involved I just cast through ULONG_PTR to silence the
warning. And clang insists that the integer whose address I give to
sk_getxdmdata is still uninitialised afterwards, which is just a lie.
clang-cl generates warnings saying they're deprecated, in favour of
the same names but prefixed with an underscore. The warnings are
coming from the standard MS headers, and I'm already #defining those
names differently on Unix, so I'll honour them.
When I added some extra braces in commit 095072fa4 to suppress this
warning, I think in fact I did the wrong thing, because the
declaration syntax I was originally using is the Microsoft-recommended
one in spite of clang not liking it - I think MS would be within their
rights (should they feel like it) to add those missing braces in a
later version of the WinSock headers, which would make the current
warning-clean code stop compiling. So it's better to put the code back
as it was, and avoid the clang warning by using clang's
warning-suppression pragmas for just those declarations.
I've also done the same thing in winnet.c, for two initialisers of
IPv6 well-known addresses which had the same problem (but which I
didn't notice yesterday because a misjudged set of Windows version
macros had prevented me from compiling that file successfully at all).
We used to offer to clean up saved sessions, so we should mention that
we don't for the benefit of users of old versions, who might have been
relying on it.
This change applies to every situation when GUI PuTTY knowingly spawns
another GUI PuTTY, to wit, the System menu options 'New Session',
'Duplicate Session' and the 'Saved Sessions' submenu.
(Literally speaking, what we actually pass through to the sub-PuTTY's
command line is not the "-restrict-acl" option itself, but a special
prefix "&R", which has the same meaning but which lives in the special
pre-argv-splitting command-line namespace like the magic options used
for Duplicate Session and the old '@sessionname' prefix which the
Saved Sessions submenu still uses. Otherwise, by the time we split up
argv and recognised -restrict-acl, it would be too late to parse those
other options.)
One case in which PuTTY spawns a subprocess and this change _doesn't_
apply is when the subprocess is a proxy command which happens to be a
Plink. Recognising Plink commands in that situation would be fragile
and unreliable, and in any case if the user wants a proxy Plink to be
ACL-restricted, they are in control of its exact command line so they
can add -restrict-acl themselves.
These ones are stylistic rather than potential bugs: mostly signedness
of char pointers in cases where they clearly aren't going to cause the
wrong thing to actually happen, and one thing in winsecur.c where
clang would have preferred an extra pair of braces around some
initialisers but it's legal with or without. But since some of clang's
warnings turn out to be quite useful, it seems worth silencing these
harmless ones so as to be able to see the rest.
I was having a play with clang's MSVC compatibility mode, just to see
how much of PuTTY it could compile, and one of its warnings pointed
out this error which must have crept in when I was changing the EOF
flags in winhandl.c from booleans to three-state enums - I left the !
on the front of what was previously an if (!thing) and needed to turn
into if (thing == EOF_NO).
Several functions were passing a 'char *error' and assigning error
messages directly into 'error', where they should have been passing
'char **error' and assigning error messages into '*error' if the error
message is to be returned to the caller. This would have led to
incomplete error messages.
As documented in bug 'win-process-acl-finesse', we've had enough
assorted complaints about it breaking various non-malicious pieces of
Windows process interaction (ranging from git->plink integration to
screen readers for the vision-impaired) that I think it's more
sensible to set the process back to its default level of protection.
This precaution was never a fully effective protection anyway, due to
the race condition at process startup; the only properly effective
defence would have been to prevent malware running under the same user
ID as PuTTY in the first place, so in that sense, nothing has changed.
But people who want the arguable defence-in-depth advantage of the ACL
restriction can now turn it on with the '-restrict-acl' command-line
option, and it's up to them whether they can live with the assorted
inconveniences that come with it.
In the course of this change, I've centralised a bit more of the
restriction code into winsecur.c, to avoid repeating the error
handling in multiple places.