I've added the gcc-style attribute("printf") to a lot of printf-shaped
functions in this code base that didn't have it. To make that easier,
I moved the wrapping macro into defs.h, and also enabled it if we
detect the __clang__ macro as well as __GNU__ (hence, it will be used
when building for Windows using clang-cl).
The result is that a great many format strings in the code are now
checked by the compiler, where they were previously not. This causes
build failures, which I'll fix in the next commit.
(cherry picked from commit cbfba7a0e9)
The entry for 19.0 which we included in advance of its listing on the
official page is now confirmed, and also three followup versions.
(cherry picked from commit 0a4e068ada)
A user reports that the ReadFile call in console_get_userpass_input
fails with ERROR_NOT_ENOUGH_MEMORY on Windows 7, and further reports
that this problem only happens if you tell ReadFile to read more than
31366 bytes in a single call.
That seems to be a thing that other people have found as well: I
turned up a similar workaround in Ruby's Win32 support module, except
that there it's for WriteConsole. So I'm reducing my arbitrary read
size of 64K to 16K, which is well under that limit.
This issue became noticeable in PuTTY as of the recent commit
cd6bc14f0, which reworked console_get_userpass_input to use strbufs.
Previously we were trying to read an amount proportional to the
existing size of the buffer, so as to grow the buffer exponentially to
save quadratic-time reallocation. That was OK in practice, since the
initial read size was nice and small. But in principle, the same bug
was present in that version of the code, just latent - if we'd ever
been called on to read a _really large_ amount of data, then
_eventually_ the input size parameter to ReadFile would have grown
beyond that mysterious limit!
(cherry picked from commit 7b79d22021)
Those chomp operations in wincons.c and uxcons.c looked ugly, and I'm
not totally convinced they couldn't underrun the buffer by 1 byte in
weird circumstances. strbuf_chomp is neater.
(cherry picked from commit 7590d0625b)
UBsan pointed out another memcpy from NULL (again with length 0) in
the prompts_t system. When I looked at it, I realised that firstly
prompt_ensure_result_size was an early not-so-good implementation of
sgrowarray_nm that would benefit from being replaced with a call to
the real one, and secondly, the whole system for storing prompt
results should really have been replaced with strbufs with the no-move
option, because that's doing all the same jobs better.
So, now each prompt_t holds a strbuf in place of its previous manually
managed string. prompt_ensure_result_size is gone (the console
prompt-reading functions use strbuf_append, and everything else just
adds to the strbuf in the usual marshal.c way). New functions exist to
retrieve a prompt_t's result, either by reference or copied.
(cherry picked from commit cd6bc14f04)
These are better than my previous approach of just assigning to
sb->len, because firstly they check by assertion that the new length
is within range, and secondly they preserve the invariant that the
byte stored in the buffer just after the length runs out is \0.
Switched to using the new functions everywhere a grep could turn up
opportunities.
(cherry picked from commit 5891142aee)
UBsan points out that if the input pointer is NULL, we'll pass it to
memcpy, which is technically illegal by the C standard _even_ if the
length you pass with it is zero.
(cherry picked from commit 88d5948ead)
In setting up the ECC tests for cmdgen, I noticed that OpenSSH and
PuTTYgen disagree on the bit length to put in a key fingerprint for an
ed25519 key: we think 255, they think 256.
On reflection, I think 255 is more accurate, which is why I bodged
get_fp() in the test suite to ignore that difference when checking our
key fingerprint against OpenSSH's. But having done that, it now seems
silly that if you unnecessarily specify a bit count at ed25519
generation time, cmdgen will insist that it be 256!
255 is now permitted everywhere an ed25519 bit count is input. 256 is
also still allowed for backwards compatibility but 255 is preferred by
the error message if you give any other value.
(cherry picked from commit 187cc8bfcc)
A user reports that if the ^E answerback string is configured to be
empty, then causing the answerback to be sent fails the assertion in
ldisc_send introduced in commit c269dd013.
I thought I'd caught all of the remaining cases of this in commit
4634cd47f, but apparently not.
(cherry picked from commit 43a63019f5)
Well, actually, two new test programs. agenttest.py is the actual
test; it depends on agenttestgen.py which generates a collection of
test private keys, using the newly exposed testcrypt interface to our
key generation code.
In this commit I've also factored out some Python SSH marshalling code
from cryptsuite, and moved it into a module ssh.py which the agent
tests can reuse.
(cherry picked from commit 8c7b0a787f)
This doesn't affect what files are _legal_: the spec said we tolerated
three kinds of line ending, and it still says we tolerate the same
three. But I noticed that we're actually outputting \n by preference,
whereas the spec said we prefer \r\n. I'd rather change the docs than
the code.
(cherry picked from commit cbfd7dadac)
If the user is scrolled back in the scrollback when a screen-swap
takes place, and if we're not configured to reset the scrollback
completely on the grounds that the swap is display activity, then we
should do the same thing we do for other kinds of display activity:
strive to keep the scroll position pointing at the same text. In this
case, that means adjusting term->disptop by the number of virtual
lines added to the scrollback to allow the main screen to be viewed
while the alt screen is active.
This improves the quality of behaviour in that corner case, but more
importantly, it should also fix a case of the dreaded line==NULL
assertion failure, which someone just reported against 0.73 when
exiting tmux (hence, switching away from the alt screen) while
scrolled back in a purely virtual scrollback buffer: the virtual
scrollback lines vanished, but disptop was still set to a negative
value, which made it out of range.
(cherry picked from commit 22453b46da)
A long time ago, in commit 09f86ce7e, I introduced a separate copy of
the saved cursor position (used by the ESC 7 / ESC 8 sequences) for
the main and alternate screens. The idea was to fix mishandling of an
input sequence of the form
ESC 7 (save cursor)
ESC [?47h (switch to alternate screen)
...
ESC 7 ESC 8 (save and restore cursor, while in alternate screen)
...
ESC [?47l (switch back from alternate screen)
ESC 8 (restore cursor, expecting it to match the _first_ ESC 7)
in which, before the fix, the second ESC 7 would overwrite the
position saved by the first one. So the final ESC 8 would restore the
cursor position to wherever it happened to have been saved in the
alternate screen, instead of where it was saved before switching _to_
the alternate screen.
I've recently noticed that the same bug still happens if you use the
alternative escape sequences ESC[?1047h and ESC[?1047l to switch to
the alternate screen, instead of ESC[?47h and ESC[?47l. This is
because that version of the escape sequence sets the internal flag
'keep_cur_pos' in the call to swap_screen, whose job is to arrange
that the actual cursor position doesn't change at the instant of the
switch. But the code that swaps the _saved_ cursor position in and out
is also conditioned on keep_cur_pos, so the 1047 variant of the
screen-swap sequence was bypassing that too, and behaving as if there
was just a single saved cursor position inside and outside the
alternate screen.
I don't know why I did it that way in 2006. It could have been
deliberate for some reason, or it could just have been mindless copy
and paste from the existing cursor-related swap code. But checking
with xterm now, it definitely seems to be wrong: the 1047 screen swap
preserves the _actual_ cursor position across the swap, but still has
independent _saved_ cursor positions in the two screens. So now PuTTY
does the same.
(cherry picked from commit 421a8ca5d9)
The do_select function is called with a boolean parameter indicating
whether we're supposed to start or stop paying attention to network
activity on a given socket. So if we freeze and unfreeze the socket in
mid-session because of backlog, we'll call do_select(s, false) to
freeze it, and do_select(s, true) to unfreeze it.
But the implementation of do_select in the Windows SFTP code predated
the rigorous handling of socket backlogs, so it assumed that
do_select(s, true) would only be called at initialisation time, i.e.
only once, and therefore that it was safe to use that flag as a cue to
set up the Windows event object to associate with socket activity.
Hence, every time the socket was frozen and unfrozen, we would create
a new netevent at unfreeze time, leaking the old one.
I think perhaps part of the reason why that was hard to figure out was
that the boolean parameter was called 'startup' rather than 'enable'.
To make it less confusing the next time I read this code, I've also
renamed it, and while I was at it, adjusted another related comment.
(cherry picked from commit bd5c957e5b)
As explained in the comment in the code, this makes it easier to map
addresses in the log files back to addresses in the code, if the
testsc image is built as a position-independent executable.
(cherry picked from commit 2804789be8)
It demonstrates a successful round trip from a source integer to
ciphertext and back, and also I've hardcoded the ciphertext I got from
the first attempt so that future changes to the code won't be able to
change it without me noticing.
In ssh_rsakex_decrypt, the code that decodes the buffer after it's
been through RSA decryption and had the OAEP masking undone would
never have worked if there were any padding 0 bytes between the prefix
and suffix of the OAEP preimage.
I must have not noticed before because PuTTY's RSA kex client code
always makes the biggest possible secret integer, so it never _does_
need any padding!
The code that reads an SSH1_AGENTC_ADD_RSA_IDENTITY message and parses
an RSA private key out of it now does it by calling a BinarySource
function in sshrsa.c, instead of doing inline in the Pageant message
handler. This has no functional change, except that now I can expose
that separate function in the testcrypt API, where it provides me with
a mechanism for creating a bare RSAKey structure for purposes of
testing RSA key exchange.
[SGT's note: Pavel Kryukov says he came across these while setting up
CI runs of testsc. Two of the three guarded functions are obvious
variants on ones we're already guarding; I hadn't heard of cfree
before, but apparently it's an alternate form of ordinary free() from
an obsolete standard, whose glibc man page says 'never use it'.]
When I reworked the 'selparams' array in commit e790adec4 to contain
pointers to 'struct selparam' rather than directly containing
structures, I missed this one case where I should have removed an &.
As a result the GTK1 signal handler that deals with clicks on the
config-pane selection treeview was getting a pointer to a pointer and
treating it as a pointer to an object. Nothing good happened.
I was testing some upcoming new GTK code against all GTK versions,
which for once was interesting enough to make nontrivial use of
g_object_ref_sink, and I found that I hadn't implemented the GTK1
fallback version right. GTK1 has no ref_sink call, but it does have
ref and sink, so the right thing seems to be to just call them in
succession.
If you go to Change Settings in Unix PuTTY or pterm, and change the
'Gap between text and window edge' setting but not the width and
height, then change_settings_menuitem() correctly sets the physical
window to a new size, but drawing_area_setup() was not recreating the
backing surface / pixmap in the same way, because it hadn't spotted
that the border size might be relevant.
Now I unconditionally work out what the exact size of the backing
surface _ought_ to be, before reaching the potential early exit path,
and never take the early exit if the backing area needs resizing for
any reason at all.
(I think this probably ought to have been part of commit 528513dde.)
I'm about to rearrange this function, and the patch that actually does
work will be easier to read if mass reindentation isn't combined with
it.
The braces I've just removed were necessary when we hadn't yet
committed to requiring (most of) C99 from all our build platforms. Now
they aren't.
Up until now, it's been a variadic _function_, whose argument list
consists of 'const char *' ASCIZ strings to concatenate, terminated by
one containing a null pointer. Now, that function is dupcat_fn(), and
it's wrapped by a C99 variadic _macro_ called dupcat(), which
automatically suffixes the null-pointer terminating argument.
This has three benefits. Firstly, it's just less effort at every call
site. Secondly, it protects against the risk of accidentally leaving
off the NULL, causing arbitrary words of stack memory to be
dereferenced as char pointers. And thirdly, it protects against the
more subtle risk of writing a bare 'NULL' as the terminating argument,
instead of casting it explicitly to a pointer. That last one is
necessary because C permits the macro NULL to expand to an integer
constant such as 0, so NULL by itself may not have pointer type, and
worse, it may not be marshalled in a variadic argument list in the
same way as a pointer. (For example, on a 64-bit machine it might only
occupy 32 bits. And yet, on another 64-bit platform, it might work
just fine, so that you don't notice the mistake!)
I was inspired to do this by happening to notice one of those bare
NULL terminators, and thinking I'd better check if there were any
more. Turned out there were quite a few. Now there are none.
In the variable-length address slot, the main SOCKS5 reply packet can
contain a binary IP address (4- or 16-byte for v4/v6 respectively), or
a string intended to be interpreted as a domain name.
I was trying out the Python SOCKS5 proxy 'pproxy' today, which sends a
string-typed reply if you send it a string-typed domain name to
connect to. This caused me to notice that PuTTY mishandles the latter
case, by failing to account for the prefix length byte of that string
when computing the total size of the reply packet. So we would
misinterpret the final byte of its reply packet as the initial byte of
the actual connection, causing us to fail to recognise the SSH greeting.
Thanks to Ulrich Jannet for pointing this out: in
ssh2_connection_filter_queue, when we process a disconnect message, we
carefully avoid dereferencing the input 'ppl' pointer after
ssh_remote_error returns, because it will have been freed. But
ssh1_connection_filter_queue didn't have the same safety precaution.
Thanks to Patrick Stekovic for pointing out that, unlike sensible IP
stacks, Windows requires a non-default socket option to prevent a
second application from binding to a port you were already listening
on, causing some of your incoming connections to be diverted.
This replaces the previous setsockopt that enabled SO_REUSEADDR, which
I put there a long time ago in order to fix an annoying behaviour if
you used the same listening socket twice in rapid succession (e.g. for
successive PuTTYs forwarding the same port) and the second one failed
to bind the listening port because a left-over connection from the
first one was still in TIME_WAIT and causing the port number to be
marked as used.
As far as I can see, SO_EXCLUSIVEADDRUSE and SO_REUSEADDR are mutually
exclusive - if I try to set both, either way round, then setsockopt
returns failure on the second one - so if I have to set the former
then I _can't_ set the latter. And fortunately, re-testing on Windows
10, the TIME_WAIT problem that SO_REUSEADDR was supposed to solve
doesn't seem to exist any more: I deliberately tried listening on a
port that had a TIME_WAIT connection sitting on it, and it worked for
me even without SO_REUSEADDR.
(I can't remember now whether I definitely confirmed the TIME_WAIT
problem on a previous version of Windows, or whether I just assumed it
would happen on Windows in the same way as Linux, where I definitely
do remember observing it.)
While I'm changing that setsockopt call, I've also fixed its 'on'
parameter so that it's a BOOL rather than an int, in accordance with
the docs for WinSock setsockopt.
The redesign in commit 9fccb065a arranged that all keystroke data goes
via term_keyinput_internal, which calls term_bracketed_paste_stop just
in case the keystroke had interrupted an in-progress paste.
But, embarrassingly, I forgot that _pasted_ data also goes via
term_keyinput_internal, and bracketed paste mode certainly should not
be terminated before _that_ is sent! I should have conditionalised the
call to term_bracketed_paste_stop on the 'interactive' flag parameter,
which is precisely there to tell the difference between pastes and
true keyboard input.
The message "Reusing a shared connection to this server" is sent to
the seat's output method during the call to ssh_init. In Windows
Plink, that output method wants to talk to the BinarySink stderr_bs
(or stdout_bs, but for this particular message, stderr). So we have to
have already set up stderr_bs by the time the backend init function is
called.
When I introduced the unreachable() macro in commit 0112936ef, I
searched the source code for assert(0) and assert(false), together
with their variant form assert(0 && "explanatory text"). But I didn't
search for assert(!"explanatory text"), which is the form I used to
use before finding that assert(0 && "text") seemed to be preferred in
other code bases.
So, here's a belated replacement of all the assert(!"stuff") macros
with further instances of unreachable().
The number of people has been steadily increasing who read our source
code with an editor that thinks tab stops are 4 spaces apart, as
opposed to the traditional tty-derived 8 that the PuTTY code expects.
So I've been wondering for ages about just fixing it, and switching to
a spaces-only policy throughout the code. And I recently found out
about 'git blame -w', which should make this change not too disruptive
for the purposes of source-control archaeology; so perhaps now is the
time.
While I'm at it, I've also taken the opportunity to remove all the
trailing spaces from source lines (on the basis that git dislikes
them, and is the only thing that seems to have a strong opinion one
way or the other).
Apologies to anyone downstream of this code who has complicated patch
sets to rebase past this change. I don't intend it to be needed again.
The RESIZE_EITHER resizing mode responds to a window resize by
changing the logical terminal size if the window is shown normally, or
by changing the font size to keep the terminal size the same if the
resize is a transition between normal and maximised state.
But a user pointed out that it's also possible for a window to receive
a WM_SIZE message while _remaining_ in maximised state, and that
PuTTY's resize logic didn't allow for that possibility. It occurs when
there's a change in the amount of available screen space for the
window to be maximised _in_: e.g. when the video resolution is
reconfigured, or when you reconnect to a Remote Desktop session using
a client window of a different size, or even when you toggle the
'Automatically hide the taskbar' option in the Windows taskbar settings.
In that situation, the right thing seems to be for PuTTY to continue
to go with the policy of changing the font size rather than the
logical terminal size. In other words, we prefer to change the font
size when the resize is _from_ maximised state, _to_ maximised state,
_or both_.
That's easily implemented by removing the check of the 'was_zoomed'
flag, in the case where we've received a WM_SIZE message with the
state SIZE_MAXIMIZED: once we know the transition is _to_ maximised
state, it doesn't matter whether or not it was also _from_ it. (But we
still set the was_zoomed flag to the most recent maximised status, so
that we can recognise transitions _out_ of maximised mode.)
In Python 3, open(path, "w") defaults to text files encoded using some
default encoding, which of course isn't what we want for image data.
Use open(path, "wb") instead, and adjust the write calls to use forms
that work with Python 3. bytes() and bytearray() are available as of
Python 2.6.
(This could be simplified to e.g. b"%c%c%c%c" % (r,g,b,a), and similarly
avoiding the manual .encode call; but %-formatting on bytes requires
Python 3.5, and I thought it might be better to be compatible with older
versions of Python 3.)
"key in dict" was introduced in Python 2.2, and the older
"dict.has_key(key)" form was removed in Python 3. Use the more portable
"key in dict" form instead.
Python 3 changed the built-in round() function to use round-to-even for
exact half-way cases. While this eliminates a bias away from zero and
is thus a better choice in many cases, it's not what mkicon.py expected.
Shadow the built-in with an invocation of the decimal module instead:
this produces identical results for mkicon.py's purposes on both Python
2 and 3.
In Python 2, x/y means floor division if both x and y are ints, and true
division otherwise: that is, 1/2 == 0. In Python 3, x/y always means
true division even if both x and y are ints, so 1/2 == 0.5.
To prepare for porting to Python 3, change all the places that relied on
floor division to use the more explicit // operator, and use "from
__future__ import division" to change the / operator to use Python 3's
semantics even in Python 2. Both of these features have been available
since Python 2.2.
I noticed today that when you use Unix Plink interactively, to run a
tty session on a remote host starting from a local tty, it doesn't
copy the local terminal size into the "pty-req" channel request.
It looks as if this is a bug introduced in 2ca0070f8, when I broke up
ssh.c. Before that, the monolithic Ssh init procedure set
ssh->term_width and ssh->term_height from the Conf you passed in.
Afterwards, variables of the same name existed in both ssh.c *and*
ssh2connection.c (the former so that it can buffer window-size changes
before a connection layer yet exists to pass them on to), but somehow,
*neither* source file remembered to initialise them from the Conf.
Fixed by reinstating the initialisation in ssh.c. (For the same reason
as above: you want the values in Conf to be overwritten if the window
size changes between ssh_init and ssh2_connection_new.)
