Re: Weird loop in ftp client

[Jérémie Courrèges-Anglas]

Maxime Villard  writes:

> Is there something wrong with this patch?

Nope, there is something wrong with ftp(1) and how it recovers from the
error.  I'll commit this tomorrow unless someone disagrees.

> Le 29/11/2013 20:44, Maxime Villard a écrit :
>> What about that?
>> 
>> Index: ftp.c
>> ===
>> RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
>> retrieving revision 1.83
>> diff -u -r1.83 ftp.c
>> --- ftp.c13 Nov 2013 20:41:14 -  1.83
>> +++ ftp.c29 Nov 2013 21:17:49 -
>> @@ -1089,9 +1089,10 @@
>>  may_send_noop_char();
>>  d = 0;
>>  do {
>> -wr = write(fileno(fout), buf + d, rd);
>> -if (wr == -1 && errno == EPIPE)
>> +if ((wr = write(fileno(fout), buf + d, rd)) == 
>> -1) {
>> +d = -1;
>>  break;
>> +}
>>  d += wr;
>>  rd -= wr;
>>  } while (d < c);
>> 
>> 
>> p is set to -1 to display the proper error message
>
> of course here I meant 'd'
>
>> 
>> 


-- 
jca | PGP: 0x06A11494 / 61DB D9A0 00A4 67CF 2A90  8961 6191 8FBF 06A1 1494

Re: Weird loop in ftp client

[Maxime Villard]

Is there something wrong with this patch?


Le 29/11/2013 20:44, Maxime Villard a écrit :
> What about that?
> 
> 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 -  1.83
> +++ ftp.c 29 Nov 2013 21:17:49 -
> @@ -1089,9 +1089,10 @@
>   may_send_noop_char();
>   d = 0;
>   do {
> - wr = write(fileno(fout), buf + d, rd);
> - if (wr == -1 && errno == EPIPE)
> + if ((wr = write(fileno(fout), buf + d, rd)) == 
> -1) {
> + d = -1;
>   break;
> + }
>   d += wr;
>   rd -= wr;
>   } while (d < c);
> 
> 
> p is set to -1 to display the proper error message

of course here I meant 'd'

> 
> 

Re: Weird loop in ftp client

[sven falempin]

If the disk is full, having time to retry was nice ?

Progress 90%.
Nop try again...

Anyway,
Warning on error would be a first i guess to spot REAL problem. (Without
changing the behavior).


[Mail] + Broken Pipe, WEEK END.




On Fri, Nov 29, 2013 at 2:44 PM, Maxime Villard  wrote:

> What about that?
>
> 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 -  1.83
> +++ ftp.c   29 Nov 2013 21:17:49 -
> @@ -1089,9 +1089,10 @@
> may_send_noop_char();
> d = 0;
> do {
> -   wr = write(fileno(fout), buf + d, rd);
> -   if (wr == -1 && errno == EPIPE)
> +   if ((wr = write(fileno(fout), buf + d,
> rd)) == -1) {
> +   d = -1;
> break;
> +   }
> d += wr;
> rd -= wr;
> } while (d < c);
>
>
> p is set to -1 to display the proper error message
>
>


-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: Weird loop in ftp client

[Maxime Villard]

What about that?

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 -  1.83
+++ ftp.c   29 Nov 2013 21:17:49 -
@@ -1089,9 +1089,10 @@
may_send_noop_char();
d = 0;
do {
-   wr = write(fileno(fout), buf + d, rd);
-   if (wr == -1 && errno == EPIPE)
+   if ((wr = write(fileno(fout), buf + d, rd)) == 
-1) {
+   d = -1;
break;
+   }
d += wr;
rd -= wr;
} while (d < c);


p is set to -1 to display the proper error message

Re: Weird loop in ftp client

[Maxime Villard]

Le 23/11/2013 11:40, Otto Moerbeek a écrit :
> On Fri, Nov 22, 2013 at 10:04:02PM +0100, Maxime Villard wrote:
> 
>> Le 22/11/2013 17:48, Ted Unangst a ??crit :
>>> On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote:
 On 2013/11/22 07:25, Maxime Villard wrote:
>>>
> 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.
>>>
>>> When does write() fail and do we want to keep writing? (If I fill up
>>> the filesystem, do I really want to spin here?)
>>
>>
>> I must say that I actually fail to see why EPIPE is the only case
>> handled; it should stop looping regardless of the errno code,
>> shouldn't it?
> 
> often you'll ned to retry on EAGAIN. But indeed, the code is weird.

as far as I see, here 'fout' is either stdout, popen() or fopen(); none
of them is non-blocking, so we can't get EAGAIN

> 
>>
>>
>>>
 Shouldn't it be something more like this? Otherwise if the write() fails,
 we attempt writing one byte fewer for every retry.
>>>
>>> That looks better to me.
>>>

Re: Weird loop in ftp client

[Andres Perera]

On Sun, Nov 24, 2013 at 7:29 PM, sven falempin  wrote:
>
>
> On Sun, Nov 24, 2013 at 5:57 PM, Andres Perera  wrote:
>>
>>
>>
>>
>> On Sun, Nov 24, 2013 at 12:11 PM, sven falempin 
>> wrote:
>>>
>>> On Sat, Nov 23, 2013 at 5:47 PM, Theo de Raadt
>>> wrote:
>>>
>>> > > On 2013/11/23 14:39, sven falempin wrote:
>>> > > > Hello,
>>> > > >
>>> > > > Another point of vue :
>>> > > > Because curl is in base, what does ftp client add to the system ?
>>> > >
>>> > > ftp(1) is not without problems (bad ssl support, semi-broken proxy
>>> > > support, difficult code) but it has one major advantage: the small
>>> > > version used on ramdisks is quite small, something like 80k.
>>> > >
>>> > > cURL is not in base, but even if it was, it looks to be much too
>>> > > big for the ramdisks.
>>> >
>>> > As a note to anyone skilled... take the above issues as a challenge.
>>> > If you can improve those problems in ftp(1), we'd love it.  But it
>>> > must remain a fairly small program.
>>> >
>>>
>>> Proposed (incomplete) behavior :
>>>
>>> EBADF => fatal
>>> EFBIG, EIO, EPIPE  => warn + quit this file ( but EPIPE => wait for
>>> someone
>>> to open the pipe ? or warn + quit this file )
>>
>>
>> $ cat /dev/zero | cat >/dev/null &
>> [1] 15546 28664
>> $ kill -INT 28664
>> $ jobs
>> [1] + Broken pipe  cat /dev/zero |
>>   Interruptcat > /dev/null
>>
>> here, 15546 exits because of EPIPE during write
>>
>> it does not warn about anything
>>
>> 28664 could've been signaled in any fashion, INT is used to highlight it
>> might be interactive use
>>
>> why should a user interrupt end up as a warning? big friggen deal just
>> exit the program
>
>
> because the job is unfinished, moreover you just kill one of two programs
>
> I do not think your case is relevant.

the process getting EPIPE doesn't know that the process reading was killed

it doesn't know anything other than EPIPE

why woud it spin infinately waiting for the fd to open again, as you
suggested? or even print a warning?

>
>>
>>
>>>
>>> ENOSPC, EDQUOT  => warn + quit this file, or warn + sleep (hope someone
>>> rm
>>> some stuff)
>>> EINTR => info + retry ? how many retry ? or for how long ?
>>> EFAULT => call the martian + fatal
>>>
>>>
>>> EAGAIN => call poll and retry ? some fs can do that right ?
>>>
>>> Network related => ignore (fout is not a socket)
>>> But this one
>>> <<
>>> [EIO]  The process is a member of a background process
>>> attempting to write to its controlling terminal,
>>> TOSTOP is set on the terminal, the process isn't
>>> ignoring the SIGTTOUT signal and the thread isn't
>>> blocking the SIGTTOUT signal, and either the
>>> process
>>> was created with
>>>
>>> vfork(2)and
>>>
>>> hasn't successfully
>>> executed one of the exec functions or the process
>>> group is orphaned.
>>>
>>> >>
>>> Yes this one would require skillz
>>> of fatal("I dont get it, can you please use your ftp client like anyone
>>> else :-)");
>>>
>>>
>>> --
>>>
>>> -
>>> () ascii ribbon campaign - against html e-mail
>>> /\
>>
>>
>
>
>
> --
> -
> () ascii ribbon campaign - against html e-mail
> /\

Re: Weird loop in ftp client

[Andres Perera]

On Sun, Nov 24, 2013 at 12:11 PM, sven falempin wrote:

> On Sat, Nov 23, 2013 at 5:47 PM, Theo de Raadt  >wrote:
>
> > > On 2013/11/23 14:39, sven falempin wrote:
> > > > Hello,
> > > >
> > > > Another point of vue :
> > > > Because curl is in base, what does ftp client add to the system ?
> > >
> > > ftp(1) is not without problems (bad ssl support, semi-broken proxy
> > > support, difficult code) but it has one major advantage: the small
> > > version used on ramdisks is quite small, something like 80k.
> > >
> > > cURL is not in base, but even if it was, it looks to be much too
> > > big for the ramdisks.
> >
> > As a note to anyone skilled... take the above issues as a challenge.
> > If you can improve those problems in ftp(1), we'd love it.  But it
> > must remain a fairly small program.
> >
>
> Proposed (incomplete) behavior :
>
> EBADF => fatal
> EFBIG, EIO, EPIPE  => warn + quit this file ( but EPIPE => wait for someone
> to open the pipe ? or warn + quit this file )
>

$ cat /dev/zero | cat >/dev/null &
[1] 15546 28664
$ kill -INT 28664
$ jobs
[1] + Broken pipe  cat /dev/zero |
  Interruptcat > /dev/null

here, 15546 exits because of EPIPE during write

it does not warn about anything

28664 could've been signaled in any fashion, INT is used to highlight it
might be interactive use

why should a user interrupt end up as a warning? big friggen deal just exit
the program


> ENOSPC, EDQUOT  => warn + quit this file, or warn + sleep (hope someone rm
> some stuff)
> EINTR => info + retry ? how many retry ? or for how long ?
> EFAULT => call the martian + fatal
>
>
> EAGAIN => call poll and retry ? some fs can do that right ?
>
> Network related => ignore (fout is not a socket)
> But this one
> <<
> [EIO]  The process is a member of a background process
> attempting to write to its controlling terminal,
> TOSTOP is set on the terminal, the process isn't
> ignoring the SIGTTOUT signal and the thread isn't
> blocking the SIGTTOUT signal, and either the
> process
> was created with
> vfork(2)<
> http://www.openbsd.org/cgi-bin/man.cgi?query=vfork&sektion=2&arch=i386&apropos=0&manpath=OpenBSD+Current
> >and
> hasn't successfully
> executed one of the exec functions or the process
> group is orphaned.
>
> >>
> Yes this one would require skillz
> of fatal("I dont get it, can you please use your ftp client like anyone
> else :-)");
>
>
> --
>
> -
> () ascii ribbon campaign - against html e-mail
> /\
>

Re: Weird loop in ftp client

[sven falempin]

On Sat, Nov 23, 2013 at 5:47 PM, Theo de Raadt wrote:

> > On 2013/11/23 14:39, sven falempin wrote:
> > > Hello,
> > >
> > > Another point of vue :
> > > Because curl is in base, what does ftp client add to the system ?
> >
> > ftp(1) is not without problems (bad ssl support, semi-broken proxy
> > support, difficult code) but it has one major advantage: the small
> > version used on ramdisks is quite small, something like 80k.
> >
> > cURL is not in base, but even if it was, it looks to be much too
> > big for the ramdisks.
>
> As a note to anyone skilled... take the above issues as a challenge.
> If you can improve those problems in ftp(1), we'd love it.  But it
> must remain a fairly small program.
>

Proposed (incomplete) behavior :

EBADF => fatal
EFBIG, EIO, EPIPE  => warn + quit this file ( but EPIPE => wait for someone
to open the pipe ? or warn + quit this file )
ENOSPC, EDQUOT  => warn + quit this file, or warn + sleep (hope someone rm
some stuff)
EINTR => info + retry ? how many retry ? or for how long ?
EFAULT => call the martian + fatal


EAGAIN => call poll and retry ? some fs can do that right ?

Network related => ignore (fout is not a socket)
But this one
<<
[EIO]  The process is a member of a background process
attempting to write to its controlling terminal,
TOSTOP is set on the terminal, the process isn't
ignoring the SIGTTOUT signal and the thread isn't
blocking the SIGTTOUT signal, and either the process
was created with
vfork(2)and
hasn't successfully
executed one of the exec functions or the process
group is orphaned.

>>
Yes this one would require skillz
of fatal("I dont get it, can you please use your ftp client like anyone
else :-)");


-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: Weird loop in ftp client

[sven falempin]

On Sat, Nov 23, 2013 at 5:29 PM, Stuart Henderson  wrote:

> On 2013/11/23 14:39, sven falempin wrote:
> > Hello,
> >
> > Another point of vue :
> > Because curl is in base, what does ftp client add to the system ?
>
> ftp(1) is not without problems (bad ssl support, semi-broken proxy
> support, difficult code) but it has one major advantage: the small
> version used on ramdisks is quite small, something like 80k.
>
> cURL is not in base, but even if it was, it looks to be much too
> big for the ramdisks.
>
>


my bad, i was really thinking it was in there

-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: Weird loop in ftp client

[Theo de Raadt]

> On 2013/11/23 14:39, sven falempin wrote:
> > Hello,
> > 
> > Another point of vue :
> > Because curl is in base, what does ftp client add to the system ?
> 
> ftp(1) is not without problems (bad ssl support, semi-broken proxy
> support, difficult code) but it has one major advantage: the small
> version used on ramdisks is quite small, something like 80k.
> 
> cURL is not in base, but even if it was, it looks to be much too
> big for the ramdisks.

As a note to anyone skilled... take the above issues as a challenge.
If you can improve those problems in ftp(1), we'd love it.  But it
must remain a fairly small program.

Re: Weird loop in ftp client

[Stuart Henderson]

On 2013/11/23 14:39, sven falempin wrote:
> Hello,
> 
> Another point of vue :
> Because curl is in base, what does ftp client add to the system ?

ftp(1) is not without problems (bad ssl support, semi-broken proxy
support, difficult code) but it has one major advantage: the small
version used on ramdisks is quite small, something like 80k.

cURL is not in base, but even if it was, it looks to be much too
big for the ramdisks.

Re: Weird loop in ftp client

[Alexander Hall]

On 11/23/13 20:39, sven falempin wrote:

Hello,

Another point of vue :
Because curl is in base, what does ftp client add to the system ?


1. It's not.
2. Interactivity.

/Alexander

Re: Weird loop in ftp client

[Alexey E. Suslikov]

sven falempin  gmail.com> writes:

> 
> Hello,
> 
> Another point of vue :
> Because curl is in base, what does ftp client add to the system ?

pkg_delete curl and you'll understand.

Re: Weird loop in ftp client

[sven falempin]

Hello,

Another point of vue :
Because curl is in base, what does ftp client add to the system ?

Cheers :-)




On Sat, Nov 23, 2013 at 5:40 AM, Otto Moerbeek  wrote:

> On Fri, Nov 22, 2013 at 10:04:02PM +0100, Maxime Villard wrote:
>
> > Le 22/11/2013 17:48, Ted Unangst a ??crit :
> > > On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote:
> > >> On 2013/11/22 07:25, Maxime Villard wrote:
> > >
> > >>> 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.
> > >
> > > When does write() fail and do we want to keep writing? (If I fill up
> > > the filesystem, do I really want to spin here?)
> >
> >
> > I must say that I actually fail to see why EPIPE is the only case
> > handled; it should stop looping regardless of the errno code,
> > shouldn't it?
>
> often you'll ned to retry on EAGAIN. But indeed, the code is weird.
>
> >
> >
> > >
> > >> Shouldn't it be something more like this? Otherwise if the write()
> fails,
> > >> we attempt writing one byte fewer for every retry.
> > >
> > > That looks better to me.
> > >
>
>


-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: Weird loop in ftp client

[Otto Moerbeek]

On Fri, Nov 22, 2013 at 10:04:02PM +0100, Maxime Villard wrote:

> Le 22/11/2013 17:48, Ted Unangst a ??crit :
> > On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote:
> >> On 2013/11/22 07:25, Maxime Villard wrote:
> > 
> >>> 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.
> > 
> > When does write() fail and do we want to keep writing? (If I fill up
> > the filesystem, do I really want to spin here?)
> 
> 
> I must say that I actually fail to see why EPIPE is the only case
> handled; it should stop looping regardless of the errno code,
> shouldn't it?

often you'll ned to retry on EAGAIN. But indeed, the code is weird.

> 
> 
> > 
> >> Shouldn't it be something more like this? Otherwise if the write() fails,
> >> we attempt writing one byte fewer for every retry.
> > 
> > That looks better to me.
> > 

Re: Weird loop in ftp client

[Maxime Villard]

Le 22/11/2013 17:48, Ted Unangst a écrit :
> On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote:
>> On 2013/11/22 07:25, Maxime Villard wrote:
> 
>>> 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.
> 
> When does write() fail and do we want to keep writing? (If I fill up
> the filesystem, do I really want to spin here?)


I must say that I actually fail to see why EPIPE is the only case
handled; it should stop looping regardless of the errno code,
shouldn't it?


> 
>> Shouldn't it be something more like this? Otherwise if the write() fails,
>> we attempt writing one byte fewer for every retry.
> 
> That looks better to me.
> 

Re: Weird loop in ftp client

[Maxime Villard]

Le 22/11/2013 11:09, Stuart Henderson a écrit :
> 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.c13 Nov 2013 20:41:14 -  1.83
>> +++ ftp.c22 Nov 2013 06:23:32 -
>> @@ -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 -  1.83
> +++ ftp.c 22 Nov 2013 10:08:16 -
> @@ -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;
> 

Yes but, if the write() always fails, there's an infinite loop here.

Re: Weird loop in ftp client

[Ted Unangst]

On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote:
> On 2013/11/22 07:25, Maxime Villard wrote:

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

When does write() fail and do we want to keep writing? (If I fill up
the filesystem, do I really want to spin here?)

> Shouldn't it be something more like this? Otherwise if the write() fails,
> we attempt writing one byte fewer for every retry.

That looks better to me.

Re: Weird loop in ftp client

[Damien Miller]

On Fri, 22 Nov 2013, Stuart Henderson wrote:

>   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);

That still loops endlessly for errors other than EPIPE.

Re: Weird loop in ftp client

[Stuart Henderson]

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 -  1.83
> +++ ftp.c 22 Nov 2013 06:23:32 -
> @@ -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 -  1.83
+++ ftp.c   22 Nov 2013 10:08:16 -
@@ -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;

Weird loop in ftp client

[Maxime Villard]

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 -  1.83
+++ ftp.c   22 Nov 2013 06:23:32 -
@@ -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?