From 3a633bed3505b15bb65d4709315a8c29b6497fe8 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 25 Feb 2020 21:27:34 +0000 Subject: [PATCH] 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. --- pscp.c | 9 +++++++++ psftp.c | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/pscp.c b/pscp.c index 69e1e40e..695ace3d 100644 --- a/pscp.c +++ b/pscp.c @@ -811,6 +811,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) { diff --git a/psftp.c b/psftp.c index 8399ac07..2be47894 100644 --- a/psftp.c +++ b/psftp.c @@ -725,6 +725,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);