Mailing List Archive

Passing SFTP options when using SCP
Hey all,

I might have missed this but is there any effective way of passing sftp
options when using scp? For example, increasing the number of
outstanding requests which would be the -R command line option in sftp.

For my purposes I'm mostly looking at sending different buffer (-B) and
request (-R) options. Even better if there is a way to do that
programmatically. For example, in sftp-client.c I can just change
DEFAULT_NUM_REQUESTS to whatever value I like and every sftp transfer I
initiate will use that.

Just curious. If there isn't I'll try to come up with a method and share
it when I'm done.


Chris
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Tue, 6 Dec 2022, Chris Rapier wrote:

> Just curious. If there isn't I'll try to come up with a method and share it
> when I'm done.

Mapping them all into ‘-o’-style long options would be good.
Perhaps even use -o and ssh(1) should just reject those that
are file transfer-only.

Meow,
//mirabilos
--
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

****************************************************
/?\ The UTF-8 Ribbon
? ? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen:
 ?  HTML eMail! Also, https://www.tarent.de/newsletter
? ? header encryption!
****************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Wed, 7 Dec 2022 at 10:46, Thorsten Glaser <t.glaser@tarent.de> wrote:
> On Tue, 6 Dec 2022, Chris Rapier wrote:
> > Just curious. If there isn't I'll try to come up with a method and share it
> > when I'm done.

There's not currently. Those are arguments 3 and 4 to do_init(). In
sftp.c they're:
conn = do_init(in, out, copy_buffer_len, num_requests, limit_kbps);

but in scp.c they're hard coded as:
return do_init(*reminp, *remoutp, 32768, 64, limit_kbps);

> Mapping them all into ‘-o’-style long options would be good.
> Perhaps even use -o and ssh(1) should just reject those that
> are file transfer-only.

That would probably be messy. Right now -o options are handled by
ssh's config file parser and scp/sftp have no knowledge of any of
that, they just pass -o options through to ssh. You'd probably have
to add a dependency on readconf.c, and you'd end up with two classes
of config keywords that work differently. That would likely be a
source of confusion ("Why can't I set SFTPBufferSize in
~/.ssh/config?").

scp's -B -r and -R are all taken. -s and -S (for "sftp thing") are
also taken. -b is available, as is -n/-N ("number of requests").

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Wed, 7 Dec 2022, Darren Tucker wrote:

>scp/sftp have no knowledge of any of that, they just pass -o options
>through to ssh. You'd probably have to add a dependency on readconf.c

OK, I see why that would end up being messy. Though the ability
to set the options in the config file *would* have been a nice
bonus ? (My missing knowledge was that scp and sftp call ssh as
a separate thing.)

bye,
//mirabilos
--
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

****************************************************
/?\ The UTF-8 Ribbon
? ? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen:
 ?  HTML eMail! Also, https://www.tarent.de/newsletter
? ? header encryption!
****************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Tue, 6 Dec 2022, Chris Rapier wrote:

> Hey all,
>
> I might have missed this but is there any effective way of passing sftp
> options when using scp? For example, increasing the number of outstanding
> requests which would be the -R command line option in sftp.
>
> For my purposes I'm mostly looking at sending different buffer (-B) and
> request (-R) options. Even better if there is a way to do that
> programmatically. For example, in sftp-client.c I can just change
> DEFAULT_NUM_REQUESTS to whatever value I like and every sftp transfer I
> initiate will use that.
>
> Just curious. If there isn't I'll try to come up with a method and share it
> when I'm done.

There are no options to do this ATM and at least one of the option
letters that sftp uses already has meaning for sftp. These are
rarely used, so maybe it makes sent to put them behind a single
getopt chat that accepts multiple arguments.

diff --git a/scp.c b/scp.c
index c7194c2..4e50be1 100644
--- a/scp.c
+++ b/scp.c
@@ -150,6 +150,10 @@ char *ssh_program = _PATH_SSH_PROGRAM;
pid_t do_cmd_pid = -1;
pid_t do_cmd_pid2 = -1;

+/* SFTP copy parameters */
+size_t sftp_copy_buflen;
+size_t sftp_nrequests;
+
/* Needed for sftp */
volatile sig_atomic_t interrupted = 0;

@@ -419,7 +423,7 @@ int
main(int argc, char **argv)
{
int ch, fflag, tflag, status, n;
- char **newargv, *argv0;
+ char **newargv, *argv0, *cp;
const char *errstr;
extern char *optarg;
extern int optind;
@@ -452,7 +456,7 @@ main(int argc, char **argv)

fflag = Tflag = tflag = 0;
while ((ch = getopt(argc, argv,
- "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:")) != -1) {
+ "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:X:")) != -1) {
switch (ch) {
/* User-visible flags. */
case '1':
@@ -533,6 +537,23 @@ main(int argc, char **argv)
addargs(&remote_remote_args, "-q");
showprogress = 0;
break;
+ case 'X':
+ if (strncmp(optarg, "buffer=", 7) == 0) {
+ sftp_copy_buflen = strtol(optarg + 7, &cp, 10);
+ if (sftp_copy_buflen == 0 || *cp != '\0') {
+ fatal("Invalid buffer size \"%s\"",
+ optarg);
+ }
+ } else if (strncmp(optarg, "nrequests=", 10) == 0) {
+ sftp_nrequests = strtol(optarg + 10, &cp, 10);
+ if (sftp_nrequests == 0 || *cp != '\0') {
+ fatal("Invalid number of requests "
+ "\"%s\"", optarg);
+ }
+ } else {
+ fatal("Invalid -X option");
+ }
+ break;

/* Server options. */
case 'd':
@@ -941,7 +962,8 @@ do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
reminp, remoutp, pidp) < 0)
return NULL;
}
- return do_init(*reminp, *remoutp, 32768, 64, limit_kbps);
+ return do_init(*reminp, *remoutp,
+ sftp_copy_buflen, sftp_nrequests, limit_kbps);
}

void
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Wed, 7 Dec 2022 at 14:23, Damien Miller <djm@mindrot.org> wrote:
[...]
> There are no options to do this ATM and at least one of the option
> letters that sftp uses already has meaning for sftp. These are
> rarely used, so maybe it makes sent to put them behind a single
> getopt chat that accepts multiple arguments.

Similar to ssh-keygen's -M, I like it. Since you picked a letter not
in use by sftp either, you could also put it in sftp and provide a
consistent interface in both.

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
I just tried this patch and it seems to work as expected. The only thing
I changed was to give initial values to sftp_copy_buflen and
sftp_nrequests. If they aren't initialized is there some default that
would kick into effect on the receiving side?

I'll be testing this in more detail once I have better access to my the
testbed I use. Are you likely to be using 'X' as the command line
argument if this gets rolled into a release? I just want to make sure my
arguments don't conflict if I do a release earlier than you do.

Chris

On 12/6/22 8:17 PM, Damien Miller wrote:
> On Tue, 6 Dec 2022, Chris Rapier wrote:
>
>> Hey all,
>>
>> I might have missed this but is there any effective way of passing sftp
>> options when using scp? For example, increasing the number of outstanding
>> requests which would be the -R command line option in sftp.
>>
>> For my purposes I'm mostly looking at sending different buffer (-B) and
>> request (-R) options. Even better if there is a way to do that
>> programmatically. For example, in sftp-client.c I can just change
>> DEFAULT_NUM_REQUESTS to whatever value I like and every sftp transfer I
>> initiate will use that.
>>
>> Just curious. If there isn't I'll try to come up with a method and share it
>> when I'm done.
>
> There are no options to do this ATM and at least one of the option
> letters that sftp uses already has meaning for sftp. These are
> rarely used, so maybe it makes sent to put them behind a single
> getopt chat that accepts multiple arguments.
>
> diff --git a/scp.c b/scp.c
> index c7194c2..4e50be1 100644
> --- a/scp.c
> +++ b/scp.c
> @@ -150,6 +150,10 @@ char *ssh_program = _PATH_SSH_PROGRAM;
> pid_t do_cmd_pid = -1;
> pid_t do_cmd_pid2 = -1;
>
> +/* SFTP copy parameters */
> +size_t sftp_copy_buflen;
> +size_t sftp_nrequests;
> +
> /* Needed for sftp */
> volatile sig_atomic_t interrupted = 0;
>
> @@ -419,7 +423,7 @@ int
> main(int argc, char **argv)
> {
> int ch, fflag, tflag, status, n;
> - char **newargv, *argv0;
> + char **newargv, *argv0, *cp;
> const char *errstr;
> extern char *optarg;
> extern int optind;
> @@ -452,7 +456,7 @@ main(int argc, char **argv)
>
> fflag = Tflag = tflag = 0;
> while ((ch = getopt(argc, argv,
> - "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:")) != -1) {
> + "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:X:")) != -1) {
> switch (ch) {
> /* User-visible flags. */
> case '1':
> @@ -533,6 +537,23 @@ main(int argc, char **argv)
> addargs(&remote_remote_args, "-q");
> showprogress = 0;
> break;
> + case 'X':
> + if (strncmp(optarg, "buffer=", 7) == 0) {
> + sftp_copy_buflen = strtol(optarg + 7, &cp, 10);
> + if (sftp_copy_buflen == 0 || *cp != '\0') {
> + fatal("Invalid buffer size \"%s\"",
> + optarg);
> + }
> + } else if (strncmp(optarg, "nrequests=", 10) == 0) {
> + sftp_nrequests = strtol(optarg + 10, &cp, 10);
> + if (sftp_nrequests == 0 || *cp != '\0') {
> + fatal("Invalid number of requests "
> + "\"%s\"", optarg);
> + }
> + } else {
> + fatal("Invalid -X option");
> + }
> + break;
>
> /* Server options. */
> case 'd':
> @@ -941,7 +962,8 @@ do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
> reminp, remoutp, pidp) < 0)
> return NULL;
> }
> - return do_init(*reminp, *remoutp, 32768, 64, limit_kbps);
> + return do_init(*reminp, *remoutp,
> + sftp_copy_buflen, sftp_nrequests, limit_kbps);
> }
>
> void
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Wed, 7 Dec 2022, Darren Tucker wrote:

> On Wed, 7 Dec 2022 at 14:23, Damien Miller <djm@mindrot.org> wrote:
> [...]
> > There are no options to do this ATM and at least one of the option
> > letters that sftp uses already has meaning for sftp. These are
> > rarely used, so maybe it makes sent to put them behind a single
> > getopt chat that accepts multiple arguments.
>
> Similar to ssh-keygen's -M, I like it. Since you picked a letter not
> in use by sftp either, you could also put it in sftp and provide a
> consistent interface in both.

Here's a fleshed out version of the previous diff, including the same
option for sftp and manpage bits.

diff --git a/scp.1 b/scp.1
index cd23f97..a98df59 100644
--- a/scp.1
+++ b/scp.1
@@ -28,6 +28,7 @@
.Op Fl o Ar ssh_option
.Op Fl P Ar port
.Op Fl S Ar program
+.Op Fl X Ar sftp_option
.Ar source ... target
.Sh DESCRIPTION
.Nm
@@ -278,6 +279,19 @@ and
to print debugging messages about their progress.
This is helpful in
debugging connection, authentication, and configuration problems.
+.It Fl X Ar sftp_option
+Specify an option that controls aspects of SFTP protocol behaviour.
+The valid options are:
+.Bl -tag -width Ds
+.It Cm nrequests Ns = Ns Ar value
+Controls how many concurrent SFTP read or write requests may be in progress
+at any point in time during a download or upload.
+By default 64 requests may be active concurrently.
+.It Cm buffer Ns = Ns Ar value
+Controls the maximum buffer size for a single SFTP read/write operation used
+during download or upload.
+By default a 32KB buffer is used.
+.El
.El
.Sh EXIT STATUS
.Ex -std scp
diff --git a/scp.c b/scp.c
index c7194c2..d401718 100644
--- a/scp.c
+++ b/scp.c
@@ -150,6 +150,10 @@ char *ssh_program = _PATH_SSH_PROGRAM;
pid_t do_cmd_pid = -1;
pid_t do_cmd_pid2 = -1;

+/* SFTP copy parameters */
+size_t sftp_copy_buflen;
+size_t sftp_nrequests;
+
/* Needed for sftp */
volatile sig_atomic_t interrupted = 0;

@@ -419,7 +423,7 @@ int
main(int argc, char **argv)
{
int ch, fflag, tflag, status, n;
- char **newargv, *argv0;
+ char **newargv, *argv0, *cp;
const char *errstr;
extern char *optarg;
extern int optind;
@@ -452,7 +456,7 @@ main(int argc, char **argv)

fflag = Tflag = tflag = 0;
while ((ch = getopt(argc, argv,
- "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:")) != -1) {
+ "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:X:")) != -1) {
switch (ch) {
/* User-visible flags. */
case '1':
@@ -533,6 +537,24 @@ main(int argc, char **argv)
addargs(&remote_remote_args, "-q");
showprogress = 0;
break;
+ case 'X':
+ /* Please keep in sync with sftp.c -X */
+ if (strncmp(optarg, "buffer=", 7) == 0) {
+ sftp_copy_buflen = strtol(optarg + 7, &cp, 10);
+ if (sftp_copy_buflen == 0 || *cp != '\0') {
+ fatal("Invalid buffer size \"%s\"",
+ optarg + 7);
+ }
+ } else if (strncmp(optarg, "nrequests=", 10) == 0) {
+ sftp_nrequests = strtol(optarg + 10, &cp, 10);
+ if (sftp_nrequests == 0 || *cp != '\0') {
+ fatal("Invalid number of requests "
+ "\"%s\"", optarg + 10);
+ }
+ } else {
+ fatal("Invalid -X option");
+ }
+ break;

/* Server options. */
case 'd':
@@ -941,7 +963,8 @@ do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
reminp, remoutp, pidp) < 0)
return NULL;
}
- return do_init(*reminp, *remoutp, 32768, 64, limit_kbps);
+ return do_init(*reminp, *remoutp,
+ sftp_copy_buflen, sftp_nrequests, limit_kbps);
}

void
diff --git a/sftp-client.c b/sftp-client.c
index a3e7a5d..1907753 100644
--- a/sftp-client.c
+++ b/sftp-client.c
@@ -55,10 +55,10 @@
extern volatile sig_atomic_t interrupted;
extern int showprogress;

-/* Default size of buffer for up/download */
+/* Default size of buffer for up/download (fix sftp.1 scp.1 if changed) */
#define DEFAULT_COPY_BUFLEN 32768

-/* Default number of concurrent outstanding requests */
+/* Default number of concurrent xfer requests (fix sftp.1 scp.1 if changed) */
#define DEFAULT_NUM_REQUESTS 64

/* Minimum amount of data to read at a time */
diff --git a/sftp.1 b/sftp.1
index 3b3f2c5..3b430c5 100644
--- a/sftp.1
+++ b/sftp.1
@@ -44,6 +44,7 @@
.Op Fl R Ar num_requests
.Op Fl S Ar program
.Op Fl s Ar subsystem | sftp_server
+.Op Fl X Ar sftp_option
.Ar destination
.Sh DESCRIPTION
.Nm
@@ -320,6 +321,19 @@ does not have an sftp subsystem configured.
.It Fl v
Raise logging level.
This option is also passed to ssh.
+.It Fl X Ar sftp_option
+Specify an option that controls aspects of SFTP protocol behaviour.
+The valid options are:
+.Bl -tag -width Ds
+.It Cm nrequests Ns = Ns Ar value
+Controls how many concurrent SFTP read or write requests may be in progress
+at any point in time during a download or upload.
+By default 64 requests may be active concurrently.
+.It Cm buffer Ns = Ns Ar value
+Controls the maximum buffer size for a single SFTP read/write operation used
+during download or upload.
+By default a 32KB buffer is used.
+.El
.El
.Sh INTERACTIVE COMMANDS
Once in interactive mode,
diff --git a/sftp.c b/sftp.c
index 02547f6..01999e1 100644
--- a/sftp.c
+++ b/sftp.c
@@ -2497,6 +2497,24 @@ main(int argc, char **argv)
ssh_program = optarg;
replacearg(&args, 0, "%s", ssh_program);
break;
+ case 'X':
+ /* Please keep in sync with ssh.c -X */
+ if (strncmp(optarg, "buffer=", 7) == 0) {
+ copy_buffer_len = strtol(optarg + 7, &cp, 10);
+ if (copy_buffer_len == 0 || *cp != '\0') {
+ fatal("Invalid buffer size \"%s\"",
+ optarg + 7);
+ }
+ } else if (strncmp(optarg, "nrequests=", 10) == 0) {
+ num_requests = strtol(optarg + 10, &cp, 10);
+ if (num_requests == 0 || *cp != '\0') {
+ fatal("Invalid number of requests "
+ "\"%s\"", optarg + 10);
+ }
+ } else {
+ fatal("Invalid -X option");
+ }
+ break;
case 'h':
default:
usage();
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
Couple of points,

On 12/7/22 9:03 PM, Damien Miller wrote:

> + case 'X':
> + /* Please keep in sync with ssh.c -X */
> + if (strncmp(optarg, "buffer=", 7) == 0) {
> + copy_buffer_len = strtol(optarg + 7, &cp, 10);
> + if (copy_buffer_len == 0 || *cp != '\0') {
> + fatal("Invalid buffer size \"%s\"",
> + optarg + 7);
> + }
> + } else if (strncmp(optarg, "nrequests=", 10) == 0) {
> + num_requests = strtol(optarg + 10, &cp, 10);
> + if (num_requests == 0 || *cp != '\0') {
> + fatal("Invalid number of requests "
> + "\"%s\"", optarg + 10);

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.

Chris
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
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.

This updates the diff to use scan_scaled() and strtonum() and refuses
negative values.

diff --git a/scp.1 b/scp.1
index cd23f97..a98df59 100644
--- a/scp.1
+++ b/scp.1
@@ -28,6 +28,7 @@
.Op Fl o Ar ssh_option
.Op Fl P Ar port
.Op Fl S Ar program
+.Op Fl X Ar sftp_option
.Ar source ... target
.Sh DESCRIPTION
.Nm
@@ -278,6 +279,19 @@ and
to print debugging messages about their progress.
This is helpful in
debugging connection, authentication, and configuration problems.
+.It Fl X Ar sftp_option
+Specify an option that controls aspects of SFTP protocol behaviour.
+The valid options are:
+.Bl -tag -width Ds
+.It Cm nrequests Ns = Ns Ar value
+Controls how many concurrent SFTP read or write requests may be in progress
+at any point in time during a download or upload.
+By default 64 requests may be active concurrently.
+.It Cm buffer Ns = Ns Ar value
+Controls the maximum buffer size for a single SFTP read/write operation used
+during download or upload.
+By default a 32KB buffer is used.
+.El
.El
.Sh EXIT STATUS
.Ex -std scp
diff --git a/scp.c b/scp.c
index c7194c2..25c8d38 100644
--- a/scp.c
+++ b/scp.c
@@ -96,6 +96,7 @@
#include <time.h>
#include <unistd.h>
#include <limits.h>
+#include <util.h>
#include <vis.h>

#include "xmalloc.h"
@@ -150,6 +151,10 @@ char *ssh_program = _PATH_SSH_PROGRAM;
pid_t do_cmd_pid = -1;
pid_t do_cmd_pid2 = -1;

+/* SFTP copy parameters */
+size_t sftp_copy_buflen;
+size_t sftp_nrequests;
+
/* Needed for sftp */
volatile sig_atomic_t interrupted = 0;

@@ -418,13 +423,14 @@ void throughlocal_sftp(struct sftp_conn *, struct sftp_conn *,
int
main(int argc, char **argv)
{
- int ch, fflag, tflag, status, n;
+ int ch, fflag, tflag, status, r, n;
char **newargv, *argv0;
const char *errstr;
extern char *optarg;
extern int optind;
enum scp_mode_e mode = MODE_SFTP;
char *sftp_direct = NULL;
+ long long llv;

/* Ensure that fds 0, 1 and 2 are open or directed to /dev/null */
sanitise_stdfd();
@@ -452,7 +458,7 @@ main(int argc, char **argv)

fflag = Tflag = tflag = 0;
while ((ch = getopt(argc, argv,
- "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:")) != -1) {
+ "12346ABCTdfOpqRrstvD:F:J:M:P:S:c:i:l:o:X:")) != -1) {
switch (ch) {
/* User-visible flags. */
case '1':
@@ -533,6 +539,31 @@ main(int argc, char **argv)
addargs(&remote_remote_args, "-q");
showprogress = 0;
break;
+ 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;
+ }
+ 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);
+ if (errstr != NULL) {
+ fatal("Invalid number of requests "
+ "\"%s\": %s", optarg + 10, errstr);
+ }
+ sftp_nrequests = (size_t)llv;
+ } else {
+ fatal("Invalid -X option");
+ }
+ break;

/* Server options. */
case 'd':
@@ -941,7 +972,8 @@ do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
reminp, remoutp, pidp) < 0)
return NULL;
}
- return do_init(*reminp, *remoutp, 32768, 64, limit_kbps);
+ return do_init(*reminp, *remoutp,
+ sftp_copy_buflen, sftp_nrequests, limit_kbps);
}

void
diff --git a/sftp-client.c b/sftp-client.c
index a3e7a5d..1907753 100644
--- a/sftp-client.c
+++ b/sftp-client.c
@@ -55,10 +55,10 @@
extern volatile sig_atomic_t interrupted;
extern int showprogress;

-/* Default size of buffer for up/download */
+/* Default size of buffer for up/download (fix sftp.1 scp.1 if changed) */
#define DEFAULT_COPY_BUFLEN 32768

-/* Default number of concurrent outstanding requests */
+/* Default number of concurrent xfer requests (fix sftp.1 scp.1 if changed) */
#define DEFAULT_NUM_REQUESTS 64

/* Minimum amount of data to read at a time */
diff --git a/sftp.1 b/sftp.1
index 3b3f2c5..3b430c5 100644
--- a/sftp.1
+++ b/sftp.1
@@ -44,6 +44,7 @@
.Op Fl R Ar num_requests
.Op Fl S Ar program
.Op Fl s Ar subsystem | sftp_server
+.Op Fl X Ar sftp_option
.Ar destination
.Sh DESCRIPTION
.Nm
@@ -320,6 +321,19 @@ does not have an sftp subsystem configured.
.It Fl v
Raise logging level.
This option is also passed to ssh.
+.It Fl X Ar sftp_option
+Specify an option that controls aspects of SFTP protocol behaviour.
+The valid options are:
+.Bl -tag -width Ds
+.It Cm nrequests Ns = Ns Ar value
+Controls how many concurrent SFTP read or write requests may be in progress
+at any point in time during a download or upload.
+By default 64 requests may be active concurrently.
+.It Cm buffer Ns = Ns Ar value
+Controls the maximum buffer size for a single SFTP read/write operation used
+during download or upload.
+By default a 32KB buffer is used.
+.El
.El
.Sh INTERACTIVE COMMANDS
Once in interactive mode,
diff --git a/sftp.c b/sftp.c
index 02547f6..6dc52a3 100644
--- a/sftp.c
+++ b/sftp.c
@@ -2383,7 +2383,7 @@ main(int argc, char **argv)
struct sftp_conn *conn;
size_t copy_buffer_len = 0;
size_t num_requests = 0;
- long long limit_kbps = 0;
+ long long llv, limit_kbps = 0;

/* Ensure that fds 0, 1 and 2 are open or directed to /dev/null */
sanitise_stdfd();
@@ -2400,7 +2400,7 @@ main(int argc, char **argv)
infile = stdin;

while ((ch = getopt(argc, argv,
- "1246AafhNpqrvCc:D:i:l:o:s:S:b:B:F:J:P:R:")) != -1) {
+ "1246AafhNpqrvCc:D:i:l:o:s:S:b:B:F:J:P:R:X:")) != -1) {
switch (ch) {
/* Passed through to ssh(1) */
case 'A':
@@ -2497,6 +2497,31 @@ main(int argc, char **argv)
ssh_program = optarg;
replacearg(&args, 0, "%s", ssh_program);
break;
+ case 'X':
+ /* Please keep in sync with ssh.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;
+ }
+ if (r == -1) {
+ fatal("Invalid buffer size \"%s\": %s",
+ optarg + 7, strerror(errno));
+ }
+ copy_buffer_len = (size_t)llv;
+ } else if (strncmp(optarg, "nrequests=", 10) == 0) {
+ llv = strtonum(optarg + 10, 1, 256 * 1024,
+ &errstr);
+ if (errstr != NULL) {
+ fatal("Invalid number of requests "
+ "\"%s\": %s", optarg + 10, errstr);
+ }
+ num_requests = (size_t)llv;
+ } else {
+ fatal("Invalid -X option");
+ }
+ break;
case 'h':
default:
usage();
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Tue, 13 Dec 2022, Chris Rapier wrote:

> 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.

Yes, libutil is a BSD library. It's included in libopenbsd-compat for
portable OpenSSH.

> > + 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"

so don't do that :) I'm only trying to stop the user wasting so much memory
as to cause problems for the system here.

-d

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On 12/13/22 3:15 PM, Damien Miller wrote:
> On Tue, 13 Dec 2022, Chris Rapier wrote:
>
>> 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.
>
> Yes, libutil is a BSD library. It's included in libopenbsd-compat for
> portable OpenSSH.

I'm not seeing it in V_9_1_P1. The only place it's referenced is in
bsd-openpty.c and there it's wrapped in ifdefs. It being elided out
doesn't seem to have an impact but I'm not sure of the purpose in this
context.


>> It seems that 256 * 1024 is too large here.
>>
>> "./scp -Xbuffer=262144 ~/50GB kilo:~
>> scp: Outbound message too long 262169"
>
> so don't do that :) I'm only trying to stop the user wasting so much memory
> as to cause problems for the system here.

Ha! I might modify that a bit for hpnssh but ya know, different
audiences. I know I'm going to get some 'poweruser' who is going to try
pushing that as far as they can and then sending me a bug report :)

(and the irony of that is not lost on me)

Chris
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: Passing SFTP options when using SCP [ In reply to ]
On Thu, 15 Dec 2022 at 06:10, Chris Rapier <rapier@psc.edu> wrote:
> On 12/13/22 3:15 PM, Damien Miller wrote:
[...]
> > Yes, libutil is a BSD library. It's included in libopenbsd-compat for
> > portable OpenSSH.
>
> I'm not seeing it in V_9_1_P1. The only place it's referenced is in
> bsd-openpty.c and there it's wrapped in ifdefs.

The functionality libutil provides (eg fmt_scaled/scan_scaled) is in
libopenbsd-compat if needed. There's no explicit util.h file.

We could potentially have configure create empty files for them in
openbsd-compat where the platform doesn't have the file. The header
search includes openbsd-compat, so #include <util.h> would find them
there and remove the need for the ifdefs, but I'm not sure if that
would be a net improvement or not.

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev