Skip to content

Documentation suggesting incorrect flags for sftp.open() #13

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

Closed
gvialetto opened this issue Nov 16, 2017 · 3 comments
Closed

Documentation suggesting incorrect flags for sftp.open() #13

gvialetto opened this issue Nov 16, 2017 · 3 comments

Comments

@gvialetto
Copy link

gvialetto commented Nov 16, 2017

Steps to reproduce:

Simply use the code in the README file to read a file from SFTP:

sftp = session.sftp_init()
with sftp.open(<remote file to read>, 0, 0) as remote_fh, \
        open(<file to write>, 'wb') as local_fh:
    for size, data in remote_fh:
        local_fh.write(data)

Expected behaviour:

Data is copied from the remote file to the local file.

Actual behaviour:

No data is ever written to <file to write>. The size parameter is set by libssh2 to LIBSSH2_ERROR_SFTP_PROTOCOL (-31) and the calling sftp.last_error() will report LIBSSH2_FX_FAILURE (4). remote_fh.fstat() will succeed without issues (reporting correct data).

Additional info:

Version ssh2-python 0.6.0 with bundled libssh2.

This seems to happen because (at least up to libssh2 1.8.0) 0 and LIBSSH2_FXF_READ are not synonyms, like it is (in my understanding) incorrectly suggested in the documentation and the examples (sftp_read.py, for example). This may be tested using libssh2 directly by changing line 251 of the sftp.c example from:

libssh2_sftp_open(sftp_session, sftppath, LIBSSH2_FXF_READ, 0);

to

libssh2_sftp_open(sftp_session, sftppath, 0, 0);

This will make libssh2_sftp_read() fail in in the same way described in the actual behaviour.

I should be able to send a pull request to fix the documentation and examples if needed (and my understanding of the issue is correct).

@pkittenis
Copy link
Member

Hi there,

Thank you for the interest and the report!

At the moment I am not able to replicate this. Can you please show complete example code that replicates the issue?

Tested with the example sftp_read.py with no changes and size was non-negative. Also modified it to write to a local file like in the readme, as per below:

    with sftp.open(args.file, 0, 0) as fh, \
         open('test.copy', 'wb') as local_fh:
        for size, data in fh:
            local_fh.write(data)

No errors. Copied file is identical.

$ diff test.copy ~/test
$ echo $?
0

Same code is also integration tested here with 0 used as argument.

All tests with libssh2 1.8.0-1.

@gvialetto
Copy link
Author

Hi there,

I got back access to this particular server today, so I had the possibility to play around with it a little and investigate this further. It's a proprietary implementation and not OpenSSH, and I can confirm that using 0 with OpenSSH works correctly.

Which is kind of strange, since the SFTP specification I'm looking at does not mention a valid flag with the value 0 though, and libssh2 does not do any conversion.

I digged in and the reason it works in OpenSSH seem to be the way the portable flags from the specification (SSH_FXP_READ and so on) gets converted to the system specific flags by the SFTP server upon getting received (libssh2 leaves the value untouched on the client side). The flags_from_portable function returns from a value of 0 (which is the value we sent) the value of 0, which is (luckily!) the value for O_RDONLY in fcntl.h and everything ends up working smoothly :)

I suspect either the proprietary SSH implementation (I have no access to the server it's running on or the source code unfortunately) it's not so forgiving on the values of the flags (but should be returning an error and apparently it's not...) or it's running on a system where O_RDONLY != 0 (and so trying to read from a file which you did not open from reading does not work properly, and that's exactly what I'm seeing).

This seems to be a portability issue at the end of the day.

@pkittenis
Copy link
Member

Hi there,

Thanks for checking and the great investigation! That all sounds reasonable, I have only ever integration tested with OpenSSH (or compatible) servers so is quite likely that other servers do not follow the 0 == read convention.

So documentation, API and readthedocs both, as well as example code need updating to use the LIBSSH2_FXF_READ.

If you'd like to take that on I would be very happy to accept a PR for it. Thanks again for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@gvialetto @pkittenis and others