Segfault when `File::close` returns error other than EAGAIN

[harmic]

I am using ssh2-rs together with async-ssh2 to make async sftp transfers.

I have been getting segfaults every so often. The stack trace looks like this:

# 0 0x0000563ea2aa1cb0 in _libssh2_list_next ()
# 1 0x0000563ea2aa5360 in sftp_packetlist_flush ()
# 2 0x0000563ea2aa80e5 in libssh2_sftp_close_handle ()
# 3 0x0000563ea2a9043c in _$LT$ssh2..sftp..File$u20$as$u20$core..ops..drop..Drop$GT$::drop::h889db86aab1b140f ()
# 4 0x0000563ea299197d in core::ptr::drop_in_place$LT$async_ssh2..sftp..File$GT$::h8c98be556192ed36 ()

In order to avoid blocking when dropping sftp file handles, I manually call async_ssh2::File::close, which in turn calls ssh2::File::close. That method currently looks like this:

pub fn close(&mut self) -> Result<(), Error> {
    {
        let locked = self.lock()?;
        self.rc(&locked, unsafe {
            raw::libssh2_sftp_close_handle(locked.raw)
        })?;
    }
    self.inner = None;
    Ok(())
}

Looking in the source code for libssh2, you can see that if any error other than EAGAIN occurs, it goes ahead and deallocates the resources associated with the file. If you were to call it again after this you would get a crash.

In the comments it actually says:

The handle is no longer usable after return of this function, unless the return value is LIBSSH2_ERROR_EAGAIN in which case this function should be called again.

The problem is, if any error at all occurs, self.inner is not set to None, which means later on when the struct is dropped, libssh2_sftp_close_handle gets called again - which goes on to cause the segfault.

The solution is to set self.inner to None either if no error occurs, OR if any error other than EAGAIN occurs.

I will submit a PR containing a fix proposal.

[FrankReh]

The problem is, if any error at all occurs, self.inner is not set to None,

Think a small typo in the description: should be self.inner is now set to None

And the fix in what's already been merged is to not set to None for the EAGAIN case.

Anyway, shouldn't this issue be closed?

[yodaldevoid]

Anyway, shouldn't this issue be closed?

Yes, thank you for catching that.