The clang-analyzer report made it look into this function and
I've went through it to remove a potential use of an
uninitialized variable and I also added some validation of input
data received from the server.
In general, lots of more code in this file need to validate the
input before assuming it is correct: there are servers out there
that have bugs or just have another idea of how to do the SFTP
protocol.
This was triggered by a clang-analyzer complaint that turned out
to be valid, and it made me dig deeper and fix some generic non-
blocking problems I disovered in the code.
While cleaning this up, I moved session-specific stuff over to a
new session.h header from the libssh2_priv.h header.
clang-analyzer pointed this out as a "Pass-by-value argument in
function call is undefined" but while I can't see exactly how
this can ever happen in reality I think a little check for safety
isn't such a bad thing here.
I'll introduce a new internal function set named
_libssh2_store_u32
_libssh2_store_u64
_libssh2_store_str
That can be used all through the library to build binary outgoing
packets. Using these instead of the current approach removes
hundreds of lines from the library while at the same time greatly
enhances readability. I've not yet fully converted everything to
use these functions.
I've converted LOTS of 'unsigned long' to 'size_t' where
data/string lengths are dealt with internally. This is The Right
Thing and it will help us make the transition to our
size_t-polished API later on as well.
I'm removing the PACKET_* error codes. They were originally
introduced as a set of separate error codes from the transport
layer, but having its own set of errors turned out to be very
awkward and they were then converted into a set of #defines that
simply maps them to the global libssh2 error codes instead. Now,
I'l take the next logical step and simply replace the PACKET_*
defines with the actual LIBSSH2_ERROR_* defines. It will increase
readability and decrease confusion.
I also separated packet stuff into its own packet.h header file.
We reserve ^libssh2_ for public symbols and we use _libssh2 as
prefix for internal ones. I fixed the intendation of all these
edits with emacs afterwards, which then changed it slightly more
than just _libssh2_error() expressions but I didn't see any
obvious problems.
This function no longer has any special purpose code for the
single entry case, as it was pointless.
The previous code would overflow the buffers with an off-by-one
in case the file name or longentry data fields received from the
server were exactly as long as the buffer provided to
libssh2_sftp_readdir_ex.
We now make sure that libssh2_sftp_readdir_ex() ALWAYS zero
terminate the buffers it fills in.
The function no longer calls the libssh2_* function again, but
properly uses the internal sftp_* instead.
When _libssh2_channel_write() is asked to send off 9 bytes, the
code needs to deal with the situation where less than 9 bytes
were sent off and prepare to send the remaining piece at a later
time.
libssh2_error() no longer allocates a string and only accepts a const
error string. I also made a lot of functions use the construct of
return libssh2_error(...) instead of having one call to
libssh2_error() and then a separate return call. In several of those
cases I then also changed the former -1 return code to a more
detailed one - something that I think will not change behaviors
anywhere but it's worth keeping an eye open for any such.
internal code was changed to use that instead of wrongly using
libssh2_channel_read_ex(). Some files now need to include
channel.h to get this proto.
channel_read() calls libssh2_error() properly on transport_read()
failures
channel_read() was adjusted to not "invent" EAGAIN return code in
case the transport_read() didn't return it
channel_close() now returns 0 or error code, as
documented. Previously it would return number of bytes read in
the last read, which was confusing (and useless).
I made this change just to easier grep for "return .*EAGAIN" cases
as they should be very rare or done wrongly. Already worked to find
a flaw, marked with "TODO FIXME THIS IS WRONG" in channel.c. I also
fixed a few cases to become more general returns now when we have
more unified return codes internally.
On many places in the code there have been laziness return -1
statements lying around that should be fixed to return sensible
error codes. Here's a take at fixing a few offenders.
Each SFTP file handle is now handled by the "mother-struct"
using the generic linked list functions. The goal is to move
all custom linked list code to use this set of functions.
I also moved the list declarations to the misc.h where they
belong and made misc.h no longer include libssh2_priv.h itself
since now libssh2_priv.h needs misc.h...
In misc.c I added a #if 0'ed _libssh2_list_insert() function
because I ended up writing one, and I believe we may need it here
too once we move over more stuff to use the _libssh2_list* family.
'sftpInit_channel' struct fields within the session struct are used. And made
sure to clear them both correctly when sftp_init() returns instead of at
shutdown time, as it must not touch them at shutdown time. This should make it
possible to properly make more than one SFTP handle.
much larger packages only cause internal problems and much larger allocations.
Also fix sftp_write() when _libssh2_channel_write() returns that a packet was
only partially sent as that is not an error.
Fixed a few error messages to more accurately point out the problem
and introduced a transport.h header.
* Fixed the blocking mode to only change behavior not the actual underlying
socket mode so we now always work with non-blocking sockets. This also
introduces a new rule of thumb in libssh2 code: we don't call the
external function calls internally. We use the internal (non-blocking)
ones!
* libssh2_channel_receive_window_adjust2 was added and
libssh2_channel_receive_window_adjust is now deprecated
* Introduced "local" header files with prototypes etc for different parts
instead of cramming everything into libssh2_priv.h. channel.h is the
first.
some tests:
* cut off "_ex" from several internal function names
* corrected some log outputs
* simplified libssh2_channel_read_ex() and made it much faster in the process
* cut out {{{ and }}} comments that were incorrect anyway
* fixed sftp_packet_ask() to return the correct packet by using memcmp() and
not strncmp()
* fixed mkdir()'s wait for packet to use the correct request_id - it
semi-worked previously because strncmp() in sftp_packet_ask() made it
match far too easily.
* took away the polling functionality from sftp_packet_ask() since it wasn't
used
Made the libssh2_sftp_open_ex() deal with servers that first responds with
STATUS OK and then sends the actual HANDLE. It seems openssh does this at
times and it screwed things up. To me it seems like a spec violation...
is private in this file only and a shorter name is nicer
Removed a "flush" of the data in sftp_packet_requirev() that now seems to have
made SFTP operations a lot more reliable. It didn't make much sense to have it
there but if someone can present a reason for one I figure we should carefully
investigate one and only do it conditionally where/when needed.
