This is the first major piece of code converted to the new
unmarshalling system, and allows me to remove all the sftp_pkt_get*
functions in sftp.c that were previously duplicating standard decode
logic.
This wraps up a (pointer, length) pair into a convenient struct that
lets me return it by value from a function, and also pass it through
to other functions in one go.
Ideally quite a lot of this code base could be switched over to using
ptrlen in place of separate pointer and length variables or function
parameters. (In fact, in my personal ideal conception of C, the usual
string type would be of this form, and all the string.h functions
would operate on ptrlens instead of zero-terminated 'char *'.)
For the moment, I'm just introducing it to make some upcoming
refactoring less inconvenient. Bulk migration of existing code to
ptrlen is a project for another time.
Along with the type itself, I've provided a convenient system of
including the contents of a ptrlen in a printf; a constructor function
that wraps up a pointer and length so you can make a ptrlen on the fly
in mid-expression; a function to compare a ptrlen against an ordinary
C string (which I mostly expect to use with string literals); and a
function 'mkstr' to make a dynamically allocated C string out of one.
That last function replaces a function of the same name in sftp.c,
which I'm promoting to a whole-codebase facility and adjusting its
API.
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.
xfer_download_data could return actuallen as either 0 or -1 to
indicate EOF. Now it's always 0, and scp_recv_filedata actually checks
for that case and reports an error.
It is possible for SSH_FXP_CLOSE requests to fail. This can happen if the
server buffers writes and an error occurs flushing the data to disk while
processing the SSH_FXP_CLOSE request. If the close fails, sftp_put_file now
returns an error as well.
The id member of the sftp_request structure is of type unsigned int.
This type is also used in the sftp_reqfind callback. In
sftp_find_request we thus need to pass a pointer to unsigned int to
find234. Before this commit, sftp_find_request was passing a pointer
to unsigned long to find234, which causes the lookup to fail on
big-endian platforms where sizeof(unsigned int) != sizeof(unsigned
long), e.g. ppc64.
Formerly PuTTY's SFTP code would transmit (or buffer) a megabyte of data
before even starting to look for acknowledgements, but wouldn't allow
there to be more than a megabyte of unacknowledged data at a time. Now,
instead, it pays attention to whether the transmit path is blocked, and
transmits iff it isn't.
This should mean that SFTP goes faster over long fat pipes, and also
doesn't end up buffering so much over thin ones.
I practice, I tend to run into other performance limitations (such as
TCP or SSH-2 windows) before this enhancement looks particularly good,
but with an artificial lag of 250 ms on the loopback interface this
patch almost doubles my upload speed, so I think it's worthwhile.
Having found a lot of unfixed constness issues in recent development,
I thought perhaps it was time to get proactive, so I compiled the
whole codebase with -Wwrite-strings. That turned up a huge load of
const problems, which I've fixed in this commit: the Unix build now
goes cleanly through with -Wwrite-strings, and the Windows build is as
close as I could get it (there are some lingering issues due to
occasional Windows API functions like AcquireCredentialsHandle not
having the right constness).
Notable fallout beyond the purely mechanical changing of types:
- the stuff saved by cmdline_save_param() is now explicitly
dupstr()ed, and freed in cmdline_run_saved.
- I couldn't make both string arguments to cmdline_process_param()
const, because it intentionally writes to one of them in the case
where it's the argument to -pw (in the vain hope of being at least
slightly friendly to 'ps'), so elsewhere I had to temporarily
dupstr() something for the sake of passing it to that function
- I had to invent a silly parallel version of const_cmp() so I could
pass const string literals in to lookup functions.
- stripslashes() in pscp.c and psftp.c has the annoying strchr nature
of the GET_32BIT macros and then used as length fields. Missing bounds
checks against zero have been added, and also I've introduced a helper
function toint() which casts from unsigned to int in such a way as to
avoid C undefined behaviour, since I'm not sure I trust compilers any
more to do the obviously sensible thing.
[originally from svn r9918]
effect of handling it, but they do not free it if it isn't a packet
they recognise as part of their upload/download. Invent a return value
that specifically signals this, and consistently free pktin at every
call site if that return value comes back. Also, ensure that that
return value also always comes with something meaningful in fxp_error.
[originally from svn r9915]
places we simply enforce by assertion that it will match the request
we sent out a moment ago: in fact it can also return NULL, so it makes
more sense to report a proper error message if it doesn't return the
expected value, and while we're at it, have that error message
whatever message was helpfully left in fxp_error() by
sftp_find_request when it failed.
To do this, I've written a centralised function in psftp.c called
sftp_wait_for_reply, which is handed a request that's just been sent
out and deals with the mechanics of waiting for its reply, returning
the reply when it arrives, and aborting with a sensible error if
anything else arrives instead. The numerous sites in psftp.c which
called sftp_find_request have all been rewritten to do this instead,
and as a side effect they now look more sensible. The only other uses
of sftp_find_request were in xfer_*load_gotpkt, which had to be
tweaked in its own way.
While I'm here, also fix memory management in sftp_find_request, which
was freeing its input packet on some but not all error return paths.
[originally from svn r9894]
using one for the length field and one for the rest of the packet
contents. Since sftp_senddata() has no queuing or deferral mechanism
but instead constructs and sends an SSH2_MSG_CHANNEL_DATA message
immediately, this change has the effect of ceasing to split every SFTP
packet across two SSH messages.
[originally from svn r9603]
I think I have to consider this to be a separate but related change to
the wishlist item 'pscp-filemodes'; that was written before the Unix
port existed, and referred to the ability to configure the permissions
used for files copied from Windows to Unix - which is still not done.
[originally from svn r9260]
encountered part way through transfer. In particular, this caused
psftp to hang (waiting for FXP_READ replies which had already
arrived) if you try `get' (without -r) on a remote directory.
[originally from svn r5005]
sftp.c, and psftp.c now uses that instead of going it alone. Should
in principle be easily installed in PSCP as well, but I haven't done
it yet; also it only handles downloads, not uploads, and finally it
doesn't yet properly calculate the correct number of parallel
requests to queue. Still, it's a start, and in my own tests it
seemed to perform as expected (download speed suddenly became
roughly what you'd expect from the available bandwidth, and
decreased by roughly the expected number of round-trip times).
[originally from svn r3468]
... here's a Unix port of PSFTP. Woo. (Oddly PSCP looks to be
somewhat harder; there's more Windows code interleaved than there
was in PSFTP.)
[originally from svn r3419]
has been split into a send half and a receive half, so that callers
can set several requests in motion at a time and deal with the
responses in whatever order they arrive.
[originally from svn r3318]
malloc functions, which automatically cast to the same type they're
allocating the size of. Should prevent any future errors involving
mallocing the size of the wrong structure type, and will also make
life easier if we ever need to turn the PuTTY core code from real C
into C++-friendly C. I haven't touched the Mac frontend in this
checkin because I couldn't compile or test it.
[originally from svn r3014]
absent, and also (I think) all the frontend request functions (such
as request_resize) take a context pointer, so that multiple windows
can be handled sensibly. I wouldn't swear to this, but I _think_
that only leaves the Unicode stuff as the last stubborn holdout.
[originally from svn r2147]
meaning that PSFTP couldn't meaningfully be debugged using
Minefield. That's what I get for developing it under Unix and
forgetting to port it properly :-/
[originally from svn r1383]
scp1 if it can't. Currently not very tested - I checked it in as
soon as it completed a successful recursive copy in both directions.
Also, one known bug: you can't specify a remote wildcard, because by
the nature of SFTP we'll need to implement the wildcard engine on
the client side. I do intend to do this (and use the same wildcard
engine in PSFTP as well) but I haven't got round to it yet.
[originally from svn r1208]
Simon Tatham
Tim Kosse
Ben Harris
Jacob Nevins