openssh unix dev - Dec 2002 - scp "Bad address" errors with strange filesystem block sizes

When copying from a remote host to a local filesystem with a strange block
size, allocbuf() in scp.c seems to calculate an incorrect buffer size,
causing the copy loop in sink() to write past the end of the buffer.

For example, with smbfs, the optimal block size is negotiated when the
client connects to the server, and is rarely a power of two. In my case
it is 64560.

This loop in sink() keeps reading into the buffer until it has read bp->count
bytes of data:

                for (count = i = 0; i < size; i += 4096) {
                        amt = 4096;
                        if (i + amt > size)
                                amt = size - i;
                        count += amt;
                        do {
                                j = read(remin, cp, amt);
                                if (j == -1 && (errno == EINTR ||
                                    errno == EAGAIN)) {
                                        continue;
                                } else if (j <= 0) {
                                        run_err("%s", j ?
strerror(errno) :
                                            "dropped connection");
                                        exit(1);
                                }
                                amt -= j;
                                cp += j;
                                statbytes += j;
                        } while (amt > 0);
                        if (count == bp->cnt) {
                                /* Keep reading so we stay sync'd up. */
                                if (wrerr == NO) {
                                        j = atomicio(write, ofd, bp->buf,
count);

The problem here is that it requests the data in chunks of 4096 bytes, but
the block size is not a multiple of 4096. One of the read() calls eventually
fails with EFAULT when "cp" is past the break.

This patch makes allocbuf() use roundup() like Berkeley rcp does, instead of
the wacky calculation it uses now. The calculated buffer sizes are probably
still suboptimal when the file system's optimal block size is not a power of
two.

Index: scp.c
==================================================================RCS file:
/x/freebsd/src/crypto/openssh/scp.c,v
retrieving revision 1.1.1.9
diff -u -r1.1.1.9 scp.c
--- scp.c	27 Jun 2002 22:31:12 -0000	1.1.1.9
+++ scp.c	3 Dec 2002 01:54:49 -0000
@@ -1030,6 +1030,9 @@
 {
 	size_t size;
 #ifdef HAVE_STRUCT_STAT_ST_BLKSIZE
+#ifndef roundup
+#define	roundup(x, y)	((((x)+((y)-1))/(y))*(y))   /* to any y */
+#endif
 	struct stat stb;
 
 	if (fstat(fd, &stb) < 0) {
@@ -1039,8 +1042,7 @@
 	if (stb.st_blksize == 0)
 		size = blksize;
 	else
-		size = blksize + (stb.st_blksize - blksize % stb.st_blksize) %
-		    stb.st_blksize;
+		size = roundup(stb.st_blksize, blksize);
 #else /* HAVE_STRUCT_STAT_ST_BLKSIZE */
 	size = blksize;
 #endif /* HAVE_STRUCT_STAT_ST_BLKSIZE */



Tim

On Tue, Dec 03, 2002 at 01:37:51PM +1100, Tim Robbins
wrote:
> +#ifndef roundup
> +#define	roundup(x, y)	((((x)+((y)-1))/(y))*(y))   /* to any y */
> +#endif
no need to define roundup here, openssh uses roundup() in
other places, too.

-m