This is a cleanup I started to notice a need for during the BinarySink
work. It removes a lot of faffing about casting things to char * or
unsigned char * so that some API will accept them, even though lots of
such APIs really take a plain 'block of raw binary data' argument and
don't care what C thinks the signedness of that data might be - they
may well reinterpret it back and forth internally.
So I've tried to arrange for all the function call APIs that ought to
have a void * (or const void *) to have one, and those that need to do
pointer arithmetic on the parameter internally can cast it back at the
top of the function. That saves endless ad-hoc casts at the call
sites.
This removes a lot of pointless duplications of those constants.
Of course, _ideally_, I should upgrade to C99 bool throughout the code
base, replacing TRUE and FALSE with true and false and tagging
variables explicitly as bool when that's what they semantically are.
But that's a much bigger piece of work, and shouldn't block this
trivial cleanup!
This simplifies the client code both in ssh.c and in the client side
of Pageant.
I've cheated a tiny bit by preparing agent requests in a strbuf that
has space reserved at the front for the packet frame, which makes life
easier for the code that sends them off.
This affects all the functions that generate public and private key
and signature blobs of all kinds, plus ssh_ecdhkex_getpublic. Instead
of returning a bare block of memory and taking an extra 'int *length'
parameter, all these functions now write to a BinarySink, and it's the
caller's job to have prepared an appropriate one where they want the
output to go (usually a strbuf).
The main value of this change is that those blob-generation functions
were chock full of ad-hoc length-counting and data marshalling. You
have only to look at rsa2_{public,private}_blob, for example, to see
the kind of thing I was keen to get rid of!
Now instead of iterating through conf twice in separate functions,
once to count up the size of the serialised data and once to write it
out, I just go through once and dump it all in a strbuf.
(Of course, I could still do a two-pass count-then-allocate approach
easily enough in this system; nothing would stop me writing a
BinarySink implementation that didn't actually store any data and just
counted its size, and then I could choose at each call site whether I
preferred to do it that way.)
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.
This seems to be a knock-on effect of my recent reworking of the SSH
code to be based around queues and callbacks. The loop iteration
function in uxsftp.c (ssh_sftp_do_select) would keep going round its
select loop until something had happened on one of its file
descriptors, and then return to the caller in the assumption that the
resulting data might have triggered whatever condition the caller was
waiting for - and if not, then the caller checks, finds nothing
interesting has happened, and resumes looping with no harm done.
But now, when something happens on an fd, it doesn't _synchronously_
trigger the follow-up condition PSFTP was waiting for (which, at
startup time, happens to be back->sendok() starting to return TRUE).
Instead, it schedules a callback, which will schedule a callback,
which ... ends up setting that flag. But by that time, the loop
function has already returned, the caller has found nothing
interesting and resumed looping, and _now_ the interesting thing
happens but it's too late because ssh_sftp_do_select will wait until
the next file descriptor activity before it next returns.
Solution: give run_toplevel_callbacks a return value which says
whether it's actually done something, and if so, return immediately in
case that was the droid the caller was looking for. As it were.
In commit 528513dde I absentmindedly replaced a write to the local
variable 'need_size' of drawing_area_setup with a write to
inst->drawing_area_setup_needed, imagining that they had the same
effect. But actually, need_size was doing two jobs and I only replaced
one of them: it was also the variable that indicated that the logical
terminal size had changed and so we had to call term_size() to make
the terminal.c data structures resize themselves appropriately. The
loss of that call also inhibited generation of SIGWINCH.
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.
Changing the window's font size with Alt-< or Alt-> was not setting
any of the flags that make drawing_area_setup consider itself to have
been non-spuriously called, so the real window would enlarge without
the backing surface also doing so.
Since Pageant contains its own passphrase prompt system rather than
delegating it to another process, it's not trivial to use it in other
contexts. But having gone to the effort of coming up with my own
askpass system that (I think) does a better job at not revealing the
length of the password, I _want_ to use it in other contexts where a
GUI passphrase or password prompt is needed. Solution: an --askpass
option.
Mostly for debugging purposes, because I'm tired of having to use
'setsid' to force Pageant to select the GUI passphrase prompt when I'm
trying to fix bugs in gtkask.c. But I can also imagine situations in
which the ability to force a GUI prompt window might be useful to end
users, for example if the process does _technically_ have a
controlling terminal but it's not a user-visible one (say, in the back
end of some automation tool like expect(1)).
For symmetry, I also provide an option to force the tty prompt. That's
less obviously useful, because that's already the preferred prompt
type when both methods are available - so the only use for it would be
if you wanted to ensure that Pageant didn't _accidentally_ try to
launch a GUI prompt, and aborted with an error if it couldn't use a
tty prompt.
I've found Unix Pageant's GTK password prompt to be a bit flaky on
Ubuntu 18.04. Part of the reason for that seems to be (I _think_) that
GTK has changed its internal order of setting things up, so that you
can no longer call gtk_widget_show_now() and expect that when it
returns everything is ready to do a gdk_seat_grab. Another part is
that - completely mysteriously as far as I can see - a _failed_
gdk_seat_grab(GDK_SEAT_CAPABILITY_KEYBOARD) has the side effect of
calling gdk_window_hide on the window you gave it!
So I've done a considerable restructuring that means we no longer
attempt to do the keyboard grab synchronously in gtk_askpass_setup.
Instead, we make keyboard grab attempts during the run of gtk_main,
scheduling each one on a timer if the previous attempt fails.
This means I need a visual indication of 'not ready for you to type
anything yet', which I've arranged by filling in the three drawing
areas to mid-grey. At the point when the keyboard grab completes and
the window becomes receptive to input, they turn into the usual one
black and two white.
In GTK 3.10 and above, high-DPI support is arranged by each window
having a property called a 'scale factor', which translates logical
pixels as seen by most of the GTK API (widget and window sizes and
positions, coordinates in the "draw" event, etc) into the physical
pixels on the screen. This is handled more or less transparently,
except that one side effect is that your Cairo-based drawing code had
better be able to cope with that scaling without getting confused.
PuTTY's isn't, because we do all our serious drawing on a separate
Cairo surface we made ourselves, and then blit subrectangles of that
to the window during updates. This has two bad consequences. Firstly,
our surface has a size derived from what GTK told us the drawing area
size is, i.e. corresponding to GTK's _logical_ pixels, so when the
scale factor is (say) 2, our drawing takes place at half size and then
gets scaled up by the final blit in the draw event, making it look
blurry and unpleasant. Secondly, those final blits seem to end up
offset by half a pixel, so that a second blit over the same
subrectangle doesn't _quite_ completely wipe out the previously
blitted data - so there's a ghostly rectangle left behind everywhere
the cursor has been.
It's not that GTK doesn't _let_ you find out the scale factor; it's
just that it's in an out-of-the-way piece of API that you have to call
specially. So now we do: our backing surface is now created at a pixel
resolution matching the screen's real pixels, and we translate GTK's
scale factor into an ordinary cairo_scale() before we commence
drawing. So we still end up drawing the same text at the same size -
and this strategy also means that non-text elements like cursor
outlines and underlining will be scaled up with the screen DPI rather
than stubbornly staying one physical pixel thick - but now it's nice
and sharp at full screen resolution, and the subrectangle blits in the
draw event are back to affecting the exact set of pixels we expect
them to.
One silly consequence is that, immediately after removing the last
one, I've installed a handler for the GTK "configure-event" signal!
That's because the GTK 3 docs claim that that's how you get notified
that your scale factor has changed at run time (e.g. if you
reconfigure the scale factor of a whole monitor in the GNOME settings
dialog). Actually in practice I seem to find out via the "draw" event
before "configure" bothers to tell me, but now I've got a usefully
idempotent function for 'check whether the scale factor has changed
and sort it out if so', I don't see any harm in calling it from
anywhere it _might_ be useful.
I've been using that signal since the very first commit of this source
file, as a combined way to be notified when the size of the drawing
area changes (typically due to user window resizing actions) and also
when the drawing area is first created and available to be drawn on.
Unfortunately, testing on Ubuntu 18.04, I ran into an oddity, in which
the call to gtk_widget_show(inst->window) in new_session_window() has
the side effect of delivering a spurious configure_event on the
drawing area with size 1x46 pixels. This causes the terminal to resize
itself to 1 column wide, and the mistake isn't rectified until a
followup configure-event arrives after new_session_window returns to
the GTK main loop. But that means terminal output can occur between
those two configure events (the connection-sharing "Reusing a shared
connection to host.name" is a good example), and when it does, it gets
embarrassingly wrapped at one character per line down the left column.
I briefly tried to bodge around this by trying to heuristically guess
which configure events were real and which were spurious, but I have
no faith in that strategy continuing to work. I think a better
approach is to abandon configure-event completely, and move to a
system in which the two purposes I was using it for are handled by two
_different_ GTK signals, namely "size-allocate" (for knowing when we
get resized) and "realize" (for knowing when the drawing area
physically exists for us to start setting up Cairo or GDK machinery).
The result seems to have fixed the silly one-column wrapping bug, and
retained the ability to handle window resizes, on every GTK version I
have conveniently available to test on, including GTK 3 both before
and after these spurious configures started to happen.
GTK 3 PuTTY/pterm has always assumed that if it was compiled with
_support_ for talking to the raw X11 layer underneath GTK and GDK,
then it was entitled to expect that raw X11 layer to exist at all
times, i.e. that GDK_DISPLAY_XDISPLAY would return a meaningful X
display that it could do useful things with. So if you ran it over the
GDK Wayland backend, it would immediately segfault.
Modern GTK applications need to cope with multiple GDK backends at run
time. It's fine for GTK PuTTY to _contain_ the code to find and use
underlying X11 primitives like the display and the X window id, but it
should be prepared to find that it's running on Wayland (or something
else again!) so those functions don't return anything useful - in
which case it should degrade gracefully to the subset of functionality
that can be accessed through backend-independent GTK calls.
Accordingly, I've centralised the use of GDK_DISPLAY_XDISPLAY into a
support function get_x_display() in gtkmisc.c, which starts by
checking that there actually is one first. All previous direct uses of
GDK_*_XDISPLAY now go via that function, and check the result for NULL
afterwards. (To save faffing about calling that function too many
times, I'm also caching the display pointer in more places, and
passing it as an extra argument to various subfunctions, mostly in
gtkfont.c.)
Similarly, the get_windowid() function that retrieves the window id to
put in the environment of pterm's child process has to be prepared for
there not to be a window id.
This isn't a complete fix for all Wayland-related problems. The other
one I'm currently aware of is that the default font is "server:fixed",
which is a bad default now that it won't be available on all backends.
And I expect that further problems will show up with more testing. But
it's a start.
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).
Colin Watson reports that on pre-releases of Ubuntu 18.04, configure
events which don't actually involve a change of window size show up
annoyingly often. Our handling of configure events involves throwing
away the backing Cairo surface, making a fresh blank one, and
scheduling a top-level callback to get terminal.c to do a repaint and
populate the new surface; so a draw event before that callback occurs
causes the window contents to flicker off and on again, not to mention
wasting a lot of time.
The simplest solution is to spot spurious configures, and respond by
not throwing away the previous Cairo surface in the first place.
Except in GTK1 (which doesn't have the former), via a gtkcompat.h
workaround.
Up-to-date GTK3 has deprecated gdk_beep(), causing build failures due
to the default -Werror setting.
Looks as if I haven't retried the GTK1 build for a while, and recent
GTK frontend development has broken it. The selection revamp has
pointed out that GTK1 didn't have the accessor function
gtk_selection_data_get_selection(), the standard GdkAtom value
GDK_SELECTION_CLIPBOARD, or keysyms for alphabetic characters; and
also I had an initialisation of one of my own structure fields
(dp->selparams) accidentally not guarded by the same GTK-versioning
ifdef that controls whether or not it was defined.
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).
Now we don't annoyingly print the 'askappend' prompt if you ask a
PuTTY tool to write its packet log to something that's not a regular
file, such as /dev/fd/1 or /dev/tty or a named pipe.
(In the case of a named pipe, another annoyance fixed by this change
is that we also don't open it for reading in the course of the
existence test.)
Apparently I haven't tried a GTK2 build since the most recent set of
GTK-related code reorganisation. Some functions that were ifdef'ed out
in GTK3 builds were now unused even in GTK2 builds (and, because they
were also declared static, caused a -Werror build failure); and the
pointless stub version of gtkapp.c was missing a stub version of a
recently added function referred to from another module.
gtk_application_set_accels_for_action() is new in Gtk 3.12, but (e.g.)
Ubuntu 14.04 LTS still ships with Gtk 3.10.
On the other hand, the function I've used instead,
gtk_application_add_accelerator(), is deprecated from Gtk 3.14 onwards,
indicating that it will disappear in some future version, so I've left
the newer code in against that day.
It actually doesn't seem to be necessary: running 'otool -L' on the
real binary in the application bundle (Pterm-bin or PuTTY-bin) lists a
lot of paths starting with "@executable_path/../Resources/", which I
take to mean that the application is already set up to automatically
load the GTK shared libraries out of its own bundle directory, without
me having to give it the extra hint of DYLD_LIBRARY_PATH.
Moreover, I just got round to upgrading my Mac to High Sierra, and now
the version of osxlaunch _with_ DYLD_LIBRARY_PATH is causing a crash
at program load time, when the libpng in the MacOS system library
directory tries to use the libz in the application bundle and finds
that it doesn't provide an entry point it was expecting
('inflateValidate'). I could try to fix that by updating the libz
version in my OS X PuTTY build environment, but that seems to me to
set a precedent of running to keep up with any further dependencies
the system libraries happen to acquire in later releases. Better to
reset DYLD_LIBRARY_PATH so that the system libpng will load the system
libz and not get confused in the first place.
I've been having intermittent segfaults in this launcher program, and
by means of the new TEST_COMPILE_ON_LINUX facility introduced by
commit eef8cac28, I ran it under valgrind which helpfully pointed out
several pointers between linked-list nodes which I'd been relying on
OS memory allocation to happen to have zeroed for me.
By default, the program still builds on Linux to a stub that just
prints 'nothing to see here'. But if you compile with
-DTEST_COMPILE_ON_LINUX, it compiles to a program that still doesn't
do anything _actually_ useful, but goes through all the same motions
that real osxlaunch would go through, until the final execv(2) fails
because of course it's not _really_ living in an application bundle
directory of the right shape.
That allows me to run all the setup code under the debugging tools I'm
most used to, in my preferred environment. (Same rationale as having
puttyapp / ptermapp build for Linux too.)
I've filled in the results of some not-entirely-conclusive
investigation into the trackpad scrolling issue, some thoughts on
resizing, and reordered the items into what currently seems the most
sensible order to me.
This still isn't complete: I also need to add the variable collections
of things like mid-session special commands and saved session names,
and also I need to try to grey out menu items when they're not
applicable. But it's a start.
Just to avoid an endless proliferation of functions too small to see,
I've arranged an enumeration of action ids and a single
app_menu_action function on the receiving end, and in gtkapp.c, a list
macro that means I at least don't have to define the tiny callback
functions and the GActionEntry records by hand and keep them in sync.
This fixes the problem I'd previously noticed, that if you don't
configure the "Command key acts as Meta" setting, then keystrokes like
Command-Q which _ought_ to function as accelerators for the
application menu bar don't.
Turns out that this was for the totally obvious reason: the keyboard
event was still being processed by gtkwin.c's key_event() and
translated via the GTK IM into ordinary keyboard input. If instead I
return FALSE from key_event on detecting that a key event has a
non-Meta-configured Command modifier, then it will go to the next-
level key-event handler inside GTK itself which implements the menu
accelerator behaviour. Another problem ticked off the OS X checklist.
That's what I get for not testing on all platforms before I push.
Forgot that, since OS X GTK mimics X11 GTK closely enough to still use
the name "CLIPBOARD" for the unique system clipboard, I had left this
code base's internal name for it as CLIP_CLIPBOARD and not the
CLIP_SYSTEM I used on Windows.
The gtkapp.c menu now has a Copy as well as Paste option; those menu
items, as well as the corresponding ones on the context menu and Copy
All, now address sets of clipboards parametrised between OS X and
ordinary GTK in unix.h. Also I've tweaked the wording of the
context-menu items to not use the X-specific terminology "CLIPBOARD"
on OS X.
I had a segfault on OS X today at Pterm.app shutdown. I wasn't able to
reproduce it in a debugger, but the cause seemed to be that
clipboard_clear called term_deselect (this was from before the patch
series that renamed that function) when inst->term was already NULL.
This must be because a clipboard_data_instance outlived its associated
inst->term, and quite likely its associated inst as well. But we can't
free those structures when a gui_data is freed, because GTK callbacks
will still depend on them; so instead we must have each gui_data keep
a list of active cdis pointing at it, and then at destruction time,
walk along the list nulling out each one's pointer to part of itself.
I've done the general clipboard revamp, and also, since I added
Ctrl-Shift-{C,V} as a new pair of UI actions for copy and paste, I've
also fulfilled the requirement that there should be some method of
non-menu-based pasting that doesn't depend on a middle mouse button or
an Ins key.
I think the list of OS X missing features is now down to details of
the OS X GTK port _itself_, as opposed to structural issues in the
general code base.
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.
