On 2013/11/22 07:25, Maxime Villard wrote:
> Hi,
> a thing I spotted some weeks ago
>
> - - - usr.bin/ftp/ftp.c l.1090 - - -
>
> d = 0;
> do {
> wr = write(fileno(fout), buf + d, rd);
> if (wr == -1 && errno == EPIPE)
> break;
> d += wr;
> rd -= wr;
> } while (d < c);
>
> If write() fails without EPIPE, d is decremented, and the function
> keeps looping. If write() succeeds after several loops, d will be
> negative, and the function will write from buf-XX.
>
> Since d is later used to display a proper error message, I'd suggest
> this patch:
>
>
> Index: ftp.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
> retrieving revision 1.83
> diff -u -r1.83 ftp.c
> --- ftp.c 13 Nov 2013 20:41:14 -0000 1.83
> +++ ftp.c 22 Nov 2013 06:23:32 -0000
> @@ -1094,7 +1094,7 @@
> break;
> d += wr;
> rd -= wr;
> - } while (d < c);
> + } while (d < c && d > 0);
> if (rd != 0)
> break;
> bytes += c;
>
>
> Any opinion?
>
Shouldn't it be something more like this? Otherwise if the write() fails,
we attempt writing one byte fewer for every retry.
Index: ftp.c
===================================================================
RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.83
diff -u -p -r1.83 ftp.c
--- ftp.c 13 Nov 2013 20:41:14 -0000 1.83
+++ ftp.c 22 Nov 2013 10:08:16 -0000
@@ -1090,10 +1090,13 @@ recvrequest(const char *cmd, const char
d = 0;
do {
wr = write(fileno(fout), buf + d, rd);
- if (wr == -1 && errno == EPIPE)
- break;
- d += wr;
- rd -= wr;
+ if (wr == -1) {
+ if (errno == EPIPE)
+ break;
+ } else {
+ d += wr;
+ rd -= wr;
+ }
} while (d < c);
if (rd != 0)
break;
