Now the return value is a dynamically allocated string instead of a
static one, which means that the error message can include details
taken from the specific failing connection. In particular, if someone
requests an X11 authorisation protocol we don't support, we can print
its name as part of the message, which may help users debug the
problem.
One particularly important special case of this is that if the client
connection presents _no_ authorisation - which is surely by far the
most likely thing to happen by accident, e.g. if the auth file has
gone missing, or the hostname doesn't match for some reason - then we
now give a specific message "No authorisation provided", which I think
is considerably more helpful than just lumping that very common case
in with "Unsupported authorisation protocol". Even changing the latter
to "Unsupported authorisation protocol ''" is still not very sensible.
The problem in that case is not that the user has tried an exotic auth
protocol we've never heard of - it's that they've forgotten, or
failed, to provide one at all.
The error message for "XDM-AUTHORIZATION-1 data was wrong length" is
the other modified one: it now says what the wrong length _was_.
However, all other failures of X-A-1 are still kept deliberately
vague, because saying which part of the decrypted string didn't match
is an obvious information leak.
(cherry picked from commit dff4bd4d14)
This reverts commit 0615767224
("Windows installer: remove explicit InstallScope setting"), albeit
with different comments.
The original change worked around a Windows security vulnerability
(CVE-2023-21800), but also resulted in a rather broken installer.
(cherry picked from commit cedeb75d59)
I checked exhaustively today and found that the only characters (even
in Unicode) that Windows's default argv splitter will recognise as
word separators are the space and tab characters. So I think it's a
mistake to use <ctype.h> functions to identify word separators; we
should use that fixed character pair, and then we know we're getting
the right ones only.
(cherry picked from commit 9adfa79767)
I thought I'd found all of these before, but perhaps a few managed to
slip in since I last looked. The character argument to the <ctype.h>
functions must have the value of an unsigned char or EOF; passing an
ordinary char (unless you know char is unsigned on every platform the
code will ever go near) risks mistaking '\xFF' for EOF, and causing
outright undefined behaviour on byte values in the range 80-FE. Never
do it.
(cherry picked from commit a76109c586)
Another bug turned up by writing tests. The code that spots that the
character won't fit, and wraps it to the next line setting
LATTR_WRAPPED2, was not checking that wrap mode was _enabled_ before
doing that. So if you printed a DW character in the rightmost column
while the terminal was in non-auto-wrap mode, you'd get an unwanted
wrap.
Other terminals disagree on what to do here. xterm leaves the cursor
in the same place and doesn't print any character at all.
gnome-terminal, on the other hand, backspaces by a character so that
it _can_ print the requested DW character, in the rightmost _two_
columns.
I think I don't much like either of those, so instead I'm using the
same fallback we use for displaying a DW character when the whole
terminal is only one column wide: if there is physically no room to
print the requested character, turn it into U+FFFD REPLACEMENT
CHARACTER.
(cherry picked from commit ed5bf9b3b8)
This is the first bug found as a direct result of writing that
terminal test program - I added some tests for things I expected to
work already, and some of them didn't, proving immediately that it was
a good idea!
If the terminal is one column wide, and you've printed a
character (hence, set the wrapnext flag), what should backspace do?
Surely it should behave like any other backspace with wrapnext set,
i.e. clear the wrapnext flag, returning the cursor's _logical_
position to the location of the most recently printed character. But
in fact it was anti-wrapping to the previous line, because I'd got the
cases in the wrong order in the if-else chain that forms the backspace
handler. So the handler for 'we're in column 0, wrapping time' was
coming before 'wrapnext is set, just clear it'.
Now wrapnext is checked _first_, before checking anything at all. Any
time we can just clear that, we should.
(cherry picked from commit 069f7c8b21)
Suppose an application tries to print a double-width character
starting in the rightmost column of the screen, so that we apply our
emergency fix of wrapping to the next line immediately and printing
the character in the first two columns. Suppose they then backspace
twice, taking the cursor to the RHS and then the LHS of that
character. What should happen if they backspace a third time?
Our previous behaviour was to completely ignore the unusual situation,
and do the same thing we'd do in any other backspace from column 0:
anti-wrap the cursor to the last column of the previous line, leaving
it in the empty character cell that was skipped when the DW char
couldn't be printed in it.
But I think this isn't the best response, because it breaks the
invariant that printing N columns' worth of graphic characters and
then backspacing N times should leave the cursor on the first of those
characters. If I print "a가" (for example) and then backspace three
times, I want the cursor on the a, _even_ if weird line wrapping
behaviour happened somewhere in that sequence.
(Rationale: this helps naïve terminal applications which don't even
know what the terminal width is, and aren't tracking their absolute x
position. In particular, the simplistic line-based input systems that
appear in OS kernels and our own lineedit.c will want to emit a fixed
number of backspace-space-backspace sequences to delete characters
previously entered on to the line by the user. They still need to
check the wcwidth of the characters they're emitting, so that they can
BSB twice for a DW character or 0 times for a combining one, but it
would be *hugely* more awkward for them to ask the terminal where the
cursor is so that they can take account of difficult line wraps!)
We already have the ability to _recognise_ this situation: on a line
that was wrapped in this unusual way, we set the LATTR_WRAPPED2 line
attribute flag, to prevent the empty rightmost column from injecting
an unwanted space into copy-pastes from the terminal. Now we also use
the same flag to cause the backspace control character to do something
interesting.
This was the fix that inspired me to start writing test_terminal,
because I knew it was touching a delicate area. However, in the course
of writing this fix and its tests, I encountered two (!) further bugs,
which I'll fix in followup commits!
(cherry picked from commit 9ba742ad9f)
For years I've been following the principle that before I'll add
auto-detection of an SSH server bug, I want the server maintainer to
have fixed the bug, so that the list of affected version numbers
triggering the workaround is complete, and to provide an incentive for
implementations to gradually converge on rightness.
*Finally*, I've got round to documenting that policy in public, for
the Feedback page!
(cherry picked from commit b5645f79dd)
A user just reported that it hasn't been there since 0.76. This turns
out to be because I put the wrong pathname on the 'zip' commands in
Buildscr (miscounted the number of ../ segments).
I would have noticed immediately, if Info-Zip had failed with an error
when it found I'd given it a nonexistent filename to add to the zip
file. But in fact it just prints a warning and proceeds to add all the
other files I specified. It looks as if it will only return a nonzero
exit status if _all_ the filenames you specified were nonexistent.
Therefore, I've rewritten the zip-creation commands so that they run
zip once per file. That way if any file is unreadable we _will_ get a
build error.
(Also, while I'm here, I took the opportunity to get rid of that ugly
ls|grep.)
(cherry picked from commit 9d308b39da)
cmake's configure-time #defines (at least the way I use them) are
defined to 0 or 1, rather than sometimes not defined at all, so you
have to test them with plain #if rather than #ifdef or #if defined.
I _thought_ I'd caught all of those in previous fixes, but apparently
there were a couple still lurking. Oops.
(cherry picked from commit e289265e37)
You can't call sk_write_eof() twice on the same socket, because the
second one will fail an assertion. Instead, you're supposed to know
you've already sent EOF, and not try to send it again.
The call to sk_write_eof() in raw_special (triggered by pressing ^D in
GUI PuTTY, for example) sets the flag raw->sent_socket_eof in an
attempt to prevent this. But it doesn't _check_ that flag, so a second
press of ^D can reach that assertion failure.
(cherry picked from commit 9f2e1e6e03)
In ldisc's line editing mode, pressing ^U is supposed to erase the
current unsent line rather than inserting a literal ^U into the
buffer. In fact, when using a non-Telnet backend, it erases the
line *and* inserts ^U into the buffer!
This happens because it shares a case handler with three other
disruptive control characters (^C, ^\, ^Z), which all also clear the
line-editing buffer before doing their various actions. But in
non-Telnet mode, their actions become literal insertion of themselves,
so the combined effect is to erase the line and them self-insert.
I'm not 100% convinced that was what I actually meant to do with those
characters. But it _certainly_ wasn't what I meant to do with ^U, so
that one at least I should fix right now!
(cherry picked from commit 5ade8c0047)
When I maximised a terminal window today and then used Ctrl-< to
reduce its font size (expecting that the window size would stay the
same but more characters would be squeezed in), pterm failed the
assertion in term_request_resize_completed() that checks
term->win_resize_pending == WIN_RESIZE_AWAIT_REPLY.
This happened because in this situation request_resize_internal() was
called from within window.c rather than from within the terminal code
itself. So the terminal didn't know a resize is pending at all, and
was surprised to be told that one had finished.
request_resize_internal() already has a flag parameter to tell it
whether a given resize came from the terminal or not. On the main code
path, that flag is used to decide whether to notify the terminal. But
on the early exit path when the window is maximised, we weren't
checking the flag. An easy fix.
(cherry picked from commit 95b926865a)
I noticed today that when GTK PuTTY puts up a message box such as a
host key dialog, which calls our create_message_box function with
selectable=true (so that the host key fingerprint can be conveniently
copy-pasted), a side effect is to take the X11 PRIMARY selection away
from whoever previously had it, even though the message box isn't
actually selecting anything right now.
I don't fully understand what's going on, but it apparently has
something to do with 'select on focus' behaviour, in which tabbing
into a selectable text control automatically selects its entire
contents. That makes sense for edit boxes, but not really for this
kind of thing.
Unfortunately, GTK apparently has no per-widget configuration to turn
that off. (The closest I found is not even per _application_: it lives
in GtkSettings, whose documentation says that it's general across all
GTK apps run by a user!)
So instead I work around it by moving the gtk_label_set_selectable
call to after the focus of the new window has already been sorted out.
Ugly, but it seems to work.
(cherry picked from commit c14f0e02cc)
Horizontal scroll events aren't generated by the traditional mouse
wheel, but they can be generated by trackpad gestures, though this
isn't always configured on.
The cross-platform and Windows parts of this patch is due to
Christopher Plewright; I added the GTK support.
(cherry picked from commit 819efc3c21)
This was requested by a downstream of the code, who wanted to change
the time/space tradeoff in the terminal. I currently have no plans to
change this setting for upstream PuTTY, although there is a cmake
option for it just to make testing it easy.
To avoid sprinkling ifdefs over the whole terminal code, the strategy
is to keep the separate type 'compressed_scrollback_line', and turn it
into a typedef for a 'termline *'. So compressline() becomes almost
trivial, and decompressline() even more so.
Memory management is the fiddly part. To make this work sensibly on
both sides, I've broken up each of compressline() and decompressline()
into two versions, one of which takes ownership of (and logically
speaking frees) its input, and the other doesn't. So at call sites
where a function was followed by a free, it's now calling the
'and_free' version of the function, and where the input object was
reused afterwards, it's calling the 'no_free' version. This means that
in different branches of the #if, I can make one function call the
other or vice versa, and no call site is stuck with having to do
things in a more roundabout way than necessary.
The freeing of the _return_ value from decompressline() is handled for
us, because termlines already have a 'temporary' flag which is set
when they're returned from the decompressor, and anyone receiving a
termline from lineptr() calls unlineptr() when they're finished with
it, which will _conditionally_ free it, depending on that 'temporary'
flag. So in the new mode, 'temporary' is never set at all, and all
those unlineptr() calls do nothing.
However, we also still need to free compressed lines properly when
they're actually being thrown away (scrolled off the top of the
scrollback, or cleaned up in term_free), and for that, I've made a new
special-purpose free_compressed_line() function.
(cherry picked from commit 5f2eff2fea)
A KDE user observed that if you 'dock' a GTK PuTTY window to the side
of the screen (by dragging it to the RH edge, causing it to
half-maximise over the right-hand half of the display, similarly to
Windows), and then send a terminal resize sequence, then PuTTY fails
the assertion in term_resize_request_completed() which expects that an
unacknowledged resize request was currently in flight.
When drawing_area_setup() calls term_resize_request_completed() in
response to the inst->term_resize_notification_required flag, it
resets the inst->win_resize_pending flag, but doesn't reset
inst->term_resize_notification_required. As a result, the _next_ call
to drawing_area_setup will find that flag still set, and make a
duplicate call to term_resize_request_completed, after the terminal no
longer believes it's waiting for a response to a resize request. And
in this 'docked to the right-hand side of the display' state, KDE
apparently triggers two calls to drawing_area_setup() in quick
succession, making this bug manifest.
I could fix this by clearing inst->term_resize_notification_required.
But inspecting all the other call sites, it seems clear to me that my
original intention was for inst->term_resize_notification_required to
be a flag that's only meaningful if inst->win_resize_pending is set.
So I think a better fix is to conditionalise the check in
drawing_area_setup so that we don't even check
inst->term_resize_notification_required if !inst->win_resize_pending.
(cherry picked from commit fec6719a2b)
From https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Any-event-tracking:
Any-event mode is the same as button-event mode, except that all motion
events are reported, even if no mouse button is down. It is enabled by
specifying 1003 to DECSET.
Normally the front ends only report mouse events when buttons are
pressed, so we introduce a MA_MOVE event with MBT_NOTHING set to
indicate such a mouse movement.
(cherry picked from commit 3cfbd3df0f)
Like 5f3b743eb0, specifically reassure the user that taking the
add-to-cache action will not cause the CA that signed the key to be
trusted in any wider context, in the case where there was no previous
certified key cached. (I don't know why I missed this out before.)
(cherry picked from commit 9209c7ea38)
When a host certificate was used outside its valid date range, we were
displaying the current time where we meant to show the relevant bound of
the validity range.
(cherry picked from commit 68db3d195d)
A server attempt to resize the window (for instance via DECCOLM) when
"When window is resized" was set to "Forbid resizing completely" would
cause all terminal output to be suspended, due to the resize attempt
never being acknowledged.
(There are other code paths like this, which I've fixed for
completeness, but I don't think they have any effect: the terminal
filters out resize attempts to the current size before this point, and
even if a server can get such a request through the SUPDUP protocol, the
test for that is wrong and will never fire -- this needs fixing
separately.)
(cherry picked from commit ebceb8bc94)
We still don't build or ship a PDF PuTTY manual by default, but we may
as well conveniently expose Halibut's ability to do so.
(I don't guarantee the resulting PDF is particularly pretty -- some of
our overlong code lines do go off the right margin currently.)
(cherry picked from commit f9a8213d95)
I didn't actually get these things wrong during the submission of
0.78, but I did notice that I'd forgotten to write them down.
(cherry picked from commit d39bcaedba)
Python 3's stderr was fully-buffered when non-interactive, unlike
Python 2 and more or less everything else, until 3.9 in 2020(!):
https://bugs.python.org/issue13601
(It would be less faff to sys.stderr.reconfigure(line_buffering=True)
at the start, but that was only added in 3.7, whereas the 'flush'
argument to print() dates back to 3.3, so I chose that to minimise
the risk of version dependencies getting in the way of using this as
a working example.)
(cherry picked from commit 329a4cdd79)
(Just in case anyone's entry point is this example, and concludes they
have to reverse-engineer the protocol from the script.)
(cherry picked from commit 538c8fd29c)
The char buffer 'blob' allocated in read_blob was never actually used
for anything, so there was no need to allocate it - and also no need
for the assert() just before it, which was added in commit
63a58759b5 to be extra safe against integer overflow when
computing the buffer size.
I feel a bit silly for having added that assertion in the first place
rather than removing the allocation it was protecting! It was
unnecessary even then, and I completely failed to notice :-)
(cherry picked from commit c780cffb7a)
I don't know why I never bothered to write it down before, but it's a
good idea to let a pre-release build actually *happen* between turning
them on and updating the website to claim they exist.
Also, for the first time ever, I've just sent out an announcement
email for the 0.78 *pre-releases*, soliciting testing in advance of
the actual release. So, add that to the checklist as well.
(cherry picked from commit 1a3655013d)
ssh->base_layer is NULL when the connection is still in its early
stages, before greetings are exchanged. If the user invokes the Change
Settings dialog in this situation, ssh_reconfig would call
ssh_ppl_reconfigure() on ssh->base_layer without checking if it was
NULL first.
In commit d07d7d66f6 I rewrote the code that constructs RTF paste
data so that it uses a strbuf, in place of the previous ad-hoc code
that counted up the lengths of pieces of RTF in advance in order to
realloc the buffer.
But apparently I left in an entire loop whose job was to count up one
of those lengths, failing to notice that it's now completely pointless
because its output value is never needed!
Happily a clang upgrade has just improved the 'variable set but not
used' warning to the point where it can spot that. I expect previously
the variable still counted as 'used' because each increment of it used
the previous value.
When the standalone version of a binary, with its help file included
as a resource, extracts that resource to write it to a disk, it could
have accidentally skipped a byte in the middle if the WriteFile call
in this loop had not managed to write the whole file in one go.
While writing the previous patch, I realise that walking along a
decrypted string and stopping to complain about the first mismatch you
find is an anti-pattern. If we're going to deliberately give the same
error message for various mismatches, so as not to give away which
part failed first, then we should also avoid giving away the same
information via a timing leak!
I don't think this is serious enough to warrant the full-on advisory
protocol, because XDM-AUTHORIZATION-1 is rarely used these days and
also DES-based, so there are bigger problems with it. (Plus, why on
earth is it based on encryption anyway, not a MAC?) But since I
spotted it in passing, might as well fix it.
Now the return value is a dynamically allocated string instead of a
static one, which means that the error message can include details
taken from the specific failing connection. In particular, if someone
requests an X11 authorisation protocol we don't support, we can print
its name as part of the message, which may help users debug the
problem.
One particularly important special case of this is that if the client
connection presents _no_ authorisation - which is surely by far the
most likely thing to happen by accident, e.g. if the auth file has
gone missing, or the hostname doesn't match for some reason - then we
now give a specific message "No authorisation provided", which I think
is considerably more helpful than just lumping that very common case
in with "Unsupported authorisation protocol". Even changing the latter
to "Unsupported authorisation protocol ''" is still not very sensible.
The problem in that case is not that the user has tried an exotic auth
protocol we've never heard of - it's that they've forgotten, or
failed, to provide one at all.
The error message for "XDM-AUTHORIZATION-1 data was wrong length" is
the other modified one: it now says what the wrong length _was_.
However, all other failures of X-A-1 are still kept deliberately
vague, because saying which part of the decrypted string didn't match
is an obvious information leak.
This reverts commit 0615767224
("Windows installer: remove explicit InstallScope setting"), albeit
with different comments.
The original change worked around a Windows security vulnerability
(CVE-2023-21800), but also resulted in a rather broken installer.
While doing that parametrisation I noticed three strlen calls that
could obviously be replaced with one - and then I also noticed that
there were missing parens in an expression that should have
been (n+1)/2, making it n + 1/2, i.e. just n, due to integer
arithmetic.
Happily that bug meant we were _over_-allocating rather than under,
but even so, how embarrassing. Fixed.
Created in the simplest way, by parametrising the existing code using
macros.
Nothing actually uses this yet. I hope to gradually switch
command-line parsing from 'ANSI' to Unicode strings, but this isn't
the only preparation needed, so it might yet be a while.
This will contain test code and test subprograms that don't belong in
the top-level test directory due to not being cross-platform.
Initial contents are test_screenshot.c, which was already its own
source file in the windows subdir, and test_split_into_argv.c, which
I've sawn off the bottom of windows/utils/split_into_argv.c and moved
into its own source file.
I checked exhaustively today and found that the only characters (even
in Unicode) that Windows's default argv splitter will recognise as
word separators are the space and tab characters. So I think it's a
mistake to use <ctype.h> functions to identify word separators; we
should use that fixed character pair, and then we know we're getting
the right ones only.
Just ran across this, which I think was a casualty of GNU indent in
the distant past not really knowing what to do with nested array
declarations.
It's also possible that that array should be replaced with something
mechanically generated from the current Unicode standard. I don't have
time to do that right now, but I can at least make the existing
version not amazingly ugly.
This avoids any further problems with non-NULL pointers, and also
allows me to throw out a lot of boilerplate initialisation of
pointers, integers and booleans to NULL, 0 or false respectively. (Of
course, all the initialisations to _other_ values have to stay.)
I'm not conveniently set up to actually run it during my main build,
since that happens entirely on Linux and cross-builds the Windows
binaries. But it should at least be possible to build and run it by
hand.
In live uses of Terminal, we always give it an ldisc almost
immediately after creating it. But the new test_terminal program
doesn't, which allows it to be an uninitialised pointer. Apparently I
got lucky while testing this the first few times.
I thought I'd found all of these before, but perhaps a few managed to
slip in since I last looked. The character argument to the <ctype.h>
functions must have the value of an unsigned char or EOF; passing an
ordinary char (unless you know char is unsigned on every platform the
code will ever go near) risks mistaking '\xFF' for EOF, and causing
outright undefined behaviour on byte values in the range 80-FE. Never
do it.
These seem likely to carry on being useful, so let's make sure they
pass before allowing any build to complete successfully. I've added
code to both test programs to return a sensible exit status indicating
pass/fail, and added runs of both to Buildscr.
Another bug turned up by writing tests. The code that spots that the
character won't fit, and wraps it to the next line setting
LATTR_WRAPPED2, was not checking that wrap mode was _enabled_ before
doing that. So if you printed a DW character in the rightmost column
while the terminal was in non-auto-wrap mode, you'd get an unwanted
wrap.
Other terminals disagree on what to do here. xterm leaves the cursor
in the same place and doesn't print any character at all.
gnome-terminal, on the other hand, backspaces by a character so that
it _can_ print the requested DW character, in the rightmost _two_
columns.
I think I don't much like either of those, so instead I'm using the
same fallback we use for displaying a DW character when the whole
terminal is only one column wide: if there is physically no room to
print the requested character, turn it into U+FFFD REPLACEMENT
CHARACTER.
