[MPlayer-dev-eng] [PATCHv2 3/8] stream ftp: readline: Always try to read complete lines

[Alexander Strasser]

Alexander Strasser wrote:
> Reimar Döffinger wrote:
> > On Sat, Nov 17, 2012 at 01:01:58AM +0100, Alexander Strasser wrote:
> > > @@ -144,8 +149,20 @@ static int readline(char *buf,int max,struct stream_priv_s *ctl)
> > >  	}
> > >        }
> > >        if (max == 1) {
> > > -	*buf = '\0';
> > > -	break;
> > > +        int found_eol = 0;
> > > +
> > > +        while (ctl->cavail > 0) { // skip remaining response line
> > > +          found_eol = *ctl->cget == '\n';
> > > +          --ctl->cavail; ++ctl->cget;
> > > +
> > > +          if (found_eol) {
> > > +            break;
> > > +          }
> > > +        }
> > > +
> > > +        if (found_eol) {
> > > +	  break;
> > > +        }
> > 
> > I find this code quite non-obvious.
> 
>   I am glad you had a look at this. I somewhat disliked the double break...
> 
> > Maybe instead
> > char *eol = memchr(ctl->cget, ctl->cavail);
> > if (eol) {
> >   eol++;
> >   ctl->cavail -= eol - ctl->cget;
> >   ctl->cget = eol;
> >   break;
> > }
> > // read more data to find end of current line
> 
>   I replaced the code now with this:
> --8<--
>   char *q = memchr(ctl->cget, '\n', ctl->cavail);
> 
>   if (q) { // found EOL: update state and return
>     ++q;
>     ctl->cavail -= q - ctl->cget;
>     ctl->cget = q;
> 
>     break;
>   }
> 
>   // receive more data to find end of current line
>   ctl->cget = ctl->cput;
> -->8--

  Excuse my changing of your eol into q, but I found
eol hard to read after eol++. Maybe calling it end
(whatever meaning the original author thought of, would
be most in-line with the code of that function).

> > I also wonder if we should do something against the
> > possibility that a hacked server could keep us in the
> > loop forever...
> 
>   Having a limit on how many bytes to look with one call of
> readline() might be a good idea. I will try to come up with
> something!

  I think I have implemented a solution for this. Do you think
it is mandatory for me to weave it into the current patch set?
Or would you be fine with incremental work from the point after
this series is committed?

  I am just asking because I also have a clean-up series sitting
in my local repository and I wonder if it would be good to have
a stable point again to base further developments on.

  Also it would be better to have the problem of the OP fixed. I
am not sure an attack from an endless sending ftp server is that
severe (our buffer is fixed at least) but maybe I did not think
of all possibilities.

> [...]

  Alexander