Ian Jackson points out that the Linux kernel has a macro of this name
with the same purpose, and suggests that it's a good idea to use the
same name as they do, so that at least some people reading one code
base might recognise it from the other.
I never really thought very hard about what order FROMFIELD's
parameters should go in, and therefore I'm pleasantly surprised to
find that my order agrees with the kernel's, so I don't have to
permute every call site as part of making this change :-)
I don't actually know why this was ever here; it appeared in the very
first commit that invented Plug in the first place (7b0e08270) without
explanation. Perhaps Dave's original idea was that sometimes you'd
need those macros _not_ to be defined so that the same names could be
reused as the methods for a particular Plug instance? But I don't
think that ever actually happened, and the code base builds just fine
with those macros defined unconditionally just like all the other sets
of method macros we now have, so let's get rid of this piece of cruft
that was apparently unnecessary all along.
I think that means that _every_ one of my traitoids is now a struct
containing a vtable pointer as one of its fields (albeit sometimes the
only field), and never just a bare pointer.
All the main backend structures - Ssh, Telnet, Pty, Serial etc - now
describe structure types themselves rather than pointers to them. The
same goes for the codebase-wide trait types Socket and Plug, and the
supporting types SockAddr and Pinger.
All those things that were typedefed as pointers are older types; the
newer ones have the explicit * at the point of use, because that's
what I now seem to be preferring. But whichever one of those is
better, inconsistently using a mixture of the two styles is worse, so
let's make everything consistent.
A few types are still implicitly pointers, such as Bignum and some of
the GSSAPI types; generally this is either because they have to be
void *, or because they're typedefed differently on different
platforms and aren't always pointers at all. Can't be helped. But I've
got rid of the main ones, at least.
Now I've got FROMFIELD, I can rework it so that structures providing
an implementation of the Socket or Plug trait no longer have to have
the vtable pointer as the very first thing in the structure. In
particular, this means that the ProxySocket structure can now directly
implement _both_ the Socket and Plug traits, which is always
_logically_ how it's worked, but previously it had to be implemented
via two separate structs linked to each other.
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.
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!
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...
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.
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.
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.
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.
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!
On both Unix and Windows, we now redirect the local proxy command's
standard error into a third pipe; data received from that pipe is
broken up at newlines and logged in the Event Log. So if the proxy
command emits any error messages in the course of failing to connect
to something, you now have a fighting chance of finding out what went
wrong.
This feature is disabled in command-line tools like PSFTP and Plink,
on the basis that in that situation it seems more likely that the user
would expect standard-error output to go to the ordinary standard
error in the ordinary way. Only GUI PuTTY catches it and logs it like
this, because it either doesn't have a standard error at all (on
Windows) or is likely to be pointing it at some completely unhelpful
session log file (under X).
When anyone connects to a PuTTY tool's listening socket - whether it's
a user of a local->remote port forwarding, a connection-sharing
downstream or a client of Pageant - we'd like to log as much
information as we can find out about where the connection came from.
To that end, I've implemented a function sk_peer_info() in the socket
abstraction, which returns a freeform text string as best it can (or
NULL, if it can't get anything at all) describing the thing at the
other end of the connection. For TCP connections, this is done using
getpeername() to get an IP address and port in the obvious way; for
Unix-domain sockets, we attempt SO_PEERCRED (conditionalised on some
moderately hairy autoconfery) to get the pid and owner of the peer. I
haven't implemented anything for Windows named pipes, but I will if I
hear of anything useful.
It was only actually used in X11 and port forwarding, to find internal
state structures given only the Socket that ssh.c held. So now that
that lookup has been reworked to be the sensible way round,
private_ptr is no longer used for anything and can be removed.
[originally from svn r10075]
That's been a FIXME in the code for ages, because it's difficult to
get winhandl.c to stop an already-started read from a handle (since
the read is a blocking system call running in a separate thread). But
I now realise it isn't absolutely necessary to do so - you can just
buffer one lot of data from winhandl and _then_ tell it to stop.
[originally from svn r10067]
It's now kept in a separate module, where it can be reused
conveniently for other kinds of Windows HANDLE that I want to wrap in
the PuTTY Socket abstraction - for example, the named pipes that I
shortly plan to use for the Windows side of connection-sharing IPC.
[originally from svn r10066]
