Passing SFTP options when using SCP

[Chris Rapier]

On 12/12/22 10:15 PM, Damien Miller wrote:
> On Mon, 12 Dec 2022, Chris Rapier wrote:
> 
>> The value tests should probably be 'val <= 0' as opposed to 'val == 0'. Also
>> you might want to have the buffer size check for a maximum value. With
>> overhead, 256K triggers an "Outbound buffer too long" error. 255K seems to
>> work but I haven't done the exact math on that one yet. Lastly, if nrequests
>> is set to a ridiculous size (on my system more than 150000) it ends up sucking
>> up a lot of memory and stalls. I think that's a buyer beware sort of thing but
>> I just wanted to let you know.
>>
>> Also, not defining the sftp_copy_buflen and sftp_nrequests means that in
>> do_sftp_connect the line
>>
>> 	return do_init(*reminp, *remoutp,
>> 	    sftp_copy_buflen, sftp_nrequests, limit_kbps);
>>
>> Ends up with 0's in there for the buflen and requests. What sort of behaviour
>> does that create on the sftp server? It seems to just pull out the stops.
> 
> No, if you look at do_init() then it uses default values of 32K buffer
> size and 64 concurrent requests.

Ah, I was missing that it was bringing in those values from sftp-client.h.

A couple of things

 > +++ b/scp.c
 > @@ -96,6 +96,7 @@
 >  #include <time.h>
 > #include <unistd.h>
 > #include <limits.h>
 > +#include <util.h>
 > #include <vis.h>

Is util.h a BSD library? My linux distro doesn't seem to have it. I did 
find it in FreeBSD but it doesn't seem necessary in linux unless it's a 
security feature.


> +		case 'X':
> +			/* Please keep in sync with sftp.c -X */
> +			if (strncmp(optarg, "buffer=", 7) == 0) {
> +				r = scan_scaled(optarg + 7, &llv);
> +				if (r == 0 && (llv <= 0 || llv > 256 * 1024)) {
> +					r = -1;
> +					errno = EINVAL;

It seems that 256 * 1024 is too large here.

"./scp -Xbuffer=262144 ~/50GB kilo:~
scp: Outbound message too long 262169"

I'm guessing control messages or something being appended is making it 
exceed SFTP_MAX_MSG_LENGTH. The size seems consistent at 25 bytes. Perhaps:

if (r == 0 && (llv <= 0 || llv > SFTP_MAX_MSG_LENGTH - 1024)) {
	r = -1;
	errno = EINVAL;
}	

That might be more informative as to why you are using that value and 
give it some additional headroom.

> +				}
> +				if (r == -1) {
> +					fatal("Invalid buffer size \"%s\": %s",
> +					     optarg + 7, strerror(errno));
> +				}
> +				sftp_copy_buflen = (size_t)llv;
> +			} else if (strncmp(optarg, "nrequests=", 10) == 0) {
> +				llv = strtonum(optarg + 10, 1, 256 * 1024,
> +				    &errstr);

In this one I would suggest making that smaller. On my tests 262144 
requests leads to an immediate stall lasting for more than 5 minutes (at 
which point I got bored and killed the process). I'd suggest something 
closer to 8k. This gives a maximum of 256MB of outstanding data with the 
default buffer size. That's enough for a 10G path at 200ms RTT.

I'd like to figure out why it stalls for so long with larger values but 
that's for another time.


Also, once again, my thanks for this and all the other help you've 
provided.

Chris