1
0
mirror of https://git.tartarus.org/simon/putty.git synced 2025-01-09 17:38:00 +00:00

Fix a deadlock in SFTP upload.

I tried to do an SFTP upload through connection sharing the other day
and found that pscp sent some data and then hung. Now I debug it, what
seems to have happened was that we were looping in sftp_recv() waiting
for an SFTP packet from the remote, but we didn't have any outstanding
SFTP requests that the remote was going to reply to. Checking further,
xfer_upload_ready() reported true, so we _could_ have sent something -
but the logic in the upload loop had a hole through which we managed
to get into 'waiting for a packet' state.

I think what must have happened is that xfer_upload_ready() reported
false so that we entered sftp_recv(), but then the event loop inside
sftp_recv() ran a toplevel callback that made xfer_upload_ready()
return true. So, the fix: sftp_recv() is our last-ditch fallback, and
we always try emptying our callback queue and rechecking upload_ready
before we resort to waiting for a remote packet.

This not only fixes the hang I observed: it also hugely improves the
upload speed. My guess is that the bug must have been preventing us
from filling our outgoing request pipeline a _lot_ - but I didn't
notice it until the one time the queue accidentally ended up empty,
rather than just sparse enough to make transfers slow.

Annoyingly, I actually considered this fix back when I was trying to
fix the proftpd issue mentioned in commit cd97b7e7e. I decided fixing
ssh_sendbuffer() was a better idea. In fact it would have been an even
better idea to do both! Oh well, better late than never.

(cherry picked from commit 3a633bed35)
This commit is contained in:
Simon Tatham 2020-02-25 21:27:34 +00:00
parent ee8baee4fa
commit aed81648cc
2 changed files with 19 additions and 0 deletions

9
pscp.c
View File

@ -812,6 +812,15 @@ int scp_send_filedata(char *data, int len)
}
while (!xfer_upload_ready(scp_sftp_xfer)) {
if (toplevel_callback_pending()) {
/* If we have pending callbacks, they might make
* xfer_upload_ready start to return true. So we should
* run them and then re-check xfer_upload_ready, before
* we go as far as waiting for an entire packet to
* arrive. */
run_toplevel_callbacks();
continue;
}
pktin = sftp_recv();
ret = xfer_upload_gotpkt(scp_sftp_xfer, pktin);
if (ret <= 0) {

10
psftp.c
View File

@ -724,6 +724,16 @@ bool sftp_put_file(char *fname, char *outfname, bool recurse, bool restart)
}
}
if (toplevel_callback_pending() && !err && !eof) {
/* If we have pending callbacks, they might make
* xfer_upload_ready start to return true. So we should
* run them and then re-check xfer_upload_ready, before
* we go as far as waiting for an entire packet to
* arrive. */
run_toplevel_callbacks();
continue;
}
if (!xfer_done(xfer)) {
pktin = sftp_recv();
ret = xfer_upload_gotpkt(xfer, pktin);