Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix libssh2_sftp_read issues #75

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
3 participants
@jakob
Copy link
Contributor

commented Jan 13, 2016

I have reviewed sftp_read() and discovered 3 separate bugs that cause data corruption.

Two of them were introduced by d754fee

I have tested this code with example-sftp and various read buffer sizes. I believe this fixes the data skipping issues reported by @sbredahl in #50.

However, I don't think the bugs that I have fixed could cause memory corruption as initially reported by @Corillian, so we probably have to keep digging.

jakob added some commits Jan 13, 2016

sftp.c: stop reading when buffer is full
Since we can only store data from a single chunk in filep,
we have to stop receiving data as soon as we have to stop
receiving data as soon as the buffer is full.

This adresses the following bug report:
#50
sftp.c: Send at least one read request before reading
This commit ensures that we have sent at least one read request before we try to read data in sftp_read().

Otherwise sftp_read() would return 0 bytes (indicating EOF) if the socket is not ready for writing.
@sbredahl

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@jakob I applied your 3 patches "by hand" and they do not seem to change the situation here. I still need to keep the buffer size <= SFTP_MAX_READ_SIZE / 4 to avoid data corruption.

Using example-sftp with a 8k buffer and 10 byte remote file, my downloaded file is 0 bytes. Can you replicate this?

(I'm using ubuntu x64 client and server here).

jakob added some commits Jan 14, 2016

sftp.c: Check Read Packet File Offset
This commit adds a simple check to see if the offset of the read
request matches the expected file offset.

We could try to recover, from this condition at some point in the future.
Right now it is better to return an error instead of corrupted data.
sftp.c: Fix Loop Continuation Logic
The previous check was more complicated than necessary and could lead to data loss when the buffer was exactly filled (ie. rc32==0)
sftp.c: ensure minimum read packet size
For optimum performance we need to ensure we don't request tiny packets.
@jakob

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

@sbredahl I couldn't replicate your problem, but I have found and fixed another bug (4973df3).

I have now added a bunch of sanity checks to sftp_read() to make it more likely that errors are detected. Could you try if it works (or at least reports an error) with these new commits?

@sbredahl

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

@jakob I cannot reproduce any of the issues I had before with your latest set of patches! I think you nailed it this time - thanks a bunch!

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

Thanks a lot for your work. I also appreciate that we have gotten some testing on this patch set, as it makes me feel safer with merging it.

There are however a few nits on the style I think:

  • there are a few trailing white spaces
  • you've indented with tabs instead of spaces in a few places
  • we write source code lines that wrap before column 80
  • "if (size < buffer_size) size = buffer_size;" should be written in two lines
  • we don't use // comments as we want to comply with C89 standard (and really old compilers)

@bagder bagder self-assigned this Jan 17, 2016

@jakob

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2016

Thanks for your comments @bagder

I have now fixed the coding style in the parts that I touched.

@bagder

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

I still get this:

sftp.c:1378:13: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

And please squash/fixup your code style fixes so that this patch set is only a series of logical fixes that fix existing problems, not problems within the patch series itself.

@jakob

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2016

@bagder thanks for your comments. It took me a while to fix the commits, but I managed to do so. Please see #79 for a cleaned up version of this pull request.

@jakob jakob closed this Feb 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.