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

[Reimar Döffinger]

On 19 Nov 2012, at 21:50, Alexander Strasser <eclipse7 at gmx.net> wrote:

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

Get the stuff in that works, improvements can still be done later.
There's lots of other ways a malicious server could make MPlayer mostly hang anyway, it was just a thing that suddenly became obvious with the changed.