Fix SCP transfer from hanging indefinitely on Windows. Fixes #107, Fixes 109

[JoyceBabu]

libssh2_scp_recv was deprecated in libssh2. libssh2_scp_recv2 was
introduced with large file support on windows. Calling libssh2_scp_recv
was returning a stat object with the current timestamp as the remote file size
causing the file download to hang indefinitely.

[wez]

Ah, good find!

The appveyor build failed for this commit; can you try resolving it locally?
These commands should reproduce the failure:

cargo run --manifest-path systest/Cargo.toml --target x86_64-pc-windows-msvc
cargo run --manifest-path systest/Cargo.toml --target i686-pc-windows-msvc

[wez]

interestingly, libc::stat appears to be equivalent to _stati64 for the msvc targets, which is in turn what libssh2 chooses in the default windows flavor of the build for its libssh2_struct_stat type.

Since that is a distinct type from libc::stat, and there doesn't appear to be a libc::_stat that we can reference, I feel like it would be a good idea to break libssh2_scp_recv on windows so that we can't call it now that we know that the type is wrong.

In other words, stick #[cfg(not(windows))] as an attribute on this to effectively nerf it.

[wez]

we could just blanket remove libssh2_scp_recv from the bindings too

[wez]

I'd suggest adding this commit to this PR to resolve my earlier comments:

$ git diff
diff --git a/libssh2-sys/lib.rs b/libssh2-sys/lib.rs
index 386d4bb..91e428e 100644
--- a/libssh2-sys/lib.rs
+++ b/libssh2-sys/lib.rs
@@ -491,9 +491,6 @@ extern {
                                   -> *mut LIBSSH2_KNOWNHOSTS;

     // scp
-    pub fn libssh2_scp_recv(sess: *mut LIBSSH2_SESSION,
-                            path: *const c_char,
-                            sb: *mut libc::stat) -> *mut LIBSSH2_CHANNEL;
     pub fn libssh2_scp_recv2(sess: *mut LIBSSH2_SESSION,
                             path: *const c_char,
                             sb: *mut libc::stat) -> *mut LIBSSH2_CHANNEL;
diff --git a/systest/build.rs b/systest/build.rs
index 4a2d2b4..92eb957 100644
--- a/systest/build.rs
+++ b/systest/build.rs
@@ -18,6 +18,7 @@ fn main() {
         .skip_type(|t| t.ends_with("FUNC"))
         .skip_fn(|f| {
             f == "libssh2_userauth_password_ex" ||
+                f == "libssh2_scp_recv2" ||
                 f == "libssh2_session_init_ex"
         });
     cfg.generate("../libssh2-sys/lib.rs", "all.rs");

[JoyceBabu]

Wouldn't it be better to push the removal of libssh2_scp_recv to a major release, to prevent backward incompatibility? Or at least alias it to libssh2_scp_recv2?

Unless you are planning a major release, wouldn't it be better to just remove it from windows for the time being?

[wez]

I believe that the next release will be a major release

[wez]

@alexcrichton there's a one-liner to libssh2-sys in here that will require a release

[alexcrichton]

This PR will want to define a new function instead of replacing the old function. Additionally the function should be tested via systest to ensure that it matches the upstream library. How come this is skipped on CI?

[wez]

This PR will want to define a new function instead of replacing the old function. Additionally the function should be tested via systest to ensure that it matches the upstream library. How come this is skipped on CI?

@alexcrichton That's my fault; the old function is deprecated on all systems and dangerously broken on windows so I suggested removing it, with the thought that we'd make a breaking version number change in here.

regarding systest: it failed in the original version of this PR, but there are no usable diagnostics:

running: "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\bin\\amd64\\cl.exe" "/nologo" "/MD" "/Z7" "/I" "C:\\projects\\ssh2-rs\\target\\x86_64-pc-windows-msvc\\debug\\build\\libssh2-sys-74b8028790894537\\out\\include" "/W4" "/W3" "/Wall" "/WX" "/wd4820" "/wd4100" "/wd4996" "/wd4296" "/wd4255" "/wd4668" "/FoC:\\projects\\ssh2-rs\\target\\x86_64-pc-windows-msvc\\debug\\build\\systest-8cbd40522cd5cf36\\out\\all.o" "/c" "C:\\projects\\ssh2-rs\\target\\x86_64-pc-windows-msvc\\debug\\build\\systest-8cbd40522cd5cf36\\out\\all.c"
all.c
cargo:warning=cl : Command line warning D9025 : overriding '/W4' with '/W3'
C:\projects\ssh2-rs\target\x86_64-pc-windows-msvc\debug\build\systest-8cbd40522cd5cf36\out\all.c(1653): error C2220: warning treated as error - no 'object' file generated
C:\projects\ssh2-rs\target\x86_64-pc-windows-msvc\debug\build\systest-8cbd40522cd5cf36\out\all.c(1653): warning C4028: formal parameter 3 different from declaration
exit code: 2
--- stderr

I haven't booted my windows system up yet today, but if memory serves, that issue boiled down to:

• libc::stat is manually prepared in the windows specific portion of that crate and is equivalent to a type that is really named something like _stat64 in the windows 10 api
• libssh2 makes a typedef with its own type name for this, but aliases it to something that is actually a #define to the underlying type in the current windows 10 headers:

libssh2-sys/libssh2/include/libssh2.h
200:typedef struct _stati64 libssh2_struct_stat;

• systest generates C code that uses struct stat as the parameter and since that type is not compatible we get a failure.

[alexcrichton]

I think it's fine to keep the deprecated signature around in case anyone is using it, but if systest is failing then that means it's not bound correctly which means that it's just a segfault lurking at runtime to happen at some point in the future. It sounds like libc::stat is the wrong signature for this argument, so that needs to be fixed one way or another (I'm not sure how to best do so myself)

[wez]

I think I found a reasonable way to resolve this; I pushed it in this PR: #121

[wez]

Fixed by #121; thanks for this!