Re: parsing quoted-string HTTP header fields

On 05/31/2011 07:30 PM, Alex Rousskov wrote:
>
>
> On Tue, 31 May 2011, Amos Jeffries wrote:
>
>> On 31/05/11 01:40, Tsantilas Christos wrote:
>>> This is the third patch.
>>>
>>> In this patch also solved a small buffer overread which exist in the
>>> original httpHeaderParseQuotedString function. The loop:
>>>
>>> while (end <= (start+len) && *end != '\\' && *end != '\"' && *end > 0x1F
>>> && *end != 0x7F)
>>> if (*end <= 0x1F || *end == 0x7F) {
>>> ...
>>> allowed to access (and parsing affected by) the char after the end of
>>> parsed string. It did not have any bad effect for null terminated
>>> strings.
>>>
>>>
>>> On 05/27/2011 03:12 PM, Amos Jeffries wrote:
>>>> On 27/05/11 23:21, Tsantilas Christos wrote:
>>>>> Hi all,
>>>>>
>>>>> Just trying to clarify what we want to implement at the end, because I
>>>>> am confused. I am responsible for the confusion because I give two
>>>>> "(3)"
>>>>> options, and I send buggy implementations for the "(1)" and the
>>>>> "second
>>>>> (3)" option.
>>>>>
>>>>> From what I can understand, currently, we have the following options:
>>>>> 1) Just ignore any "\r" or "\n" character. This is the fastest and
>>>>> simpler approach
>>>>> 2) Require "[\r]\n " or "[\r]\n\t" as line separator and replace it
>>>>> with
>>>>> a space.
>>>>>
>>>>> From the discussion the (1) may be dangerous because strings like this
>>>>> "1\r23" will be converted to "123" which maybe it is dangerous.
>>>>>
>>>>> So I suppose we should implement the (2) option. Is it OK?
>>>>
>>>> Agreed.
>>>>
>>>> What we have been debugging in the other half of the thread was "\r\n "
>>>> or "\r\n\t".
>>>>
>>>> I think it just needs:
>>>> * the two buffer overread bugs Alex spotted removed,
>>>> * the \r made optional.
>>>>
>>>> Amos
>>>
>>
>> You don't have to escape ' in the debugs(..., \'\\r\' ) just the \\r.
>>
>> Please put "HERE" into the debugs for tracking.
>>
>>
>> Looks okay to me. Passes the new criteria.
>
> I did not notice any problems except for quoting mentioned by Amos above.
>
> Perhaps you can add a TODO comment to replace the entire LWS.

OK, I will make these changes and I will commit to trunk.

>
>
> Cheers,
>
> Alex.
>
Received on Tue May 31 2011 - 20:17:37 MDT