Re: [PATCH] Bug 1961 pt2: store URI userinfo (aka login) in class URL

LGTM. +1.

On Fri, May 9, 2014 at 9:31 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 28/04/2014 2:39 p.m., Amos Jeffries wrote:
>> On 28/04/2014 3:51 a.m., Kinkie wrote:
>>> On Sun, Apr 27, 2014 at 4:52 PM, Amos Jeffries wrote:
>>>> This continues the bug 1961 redesign of URL handling.
>>>
>>> Hi,
>>>
>>> + SBuf tmp = checklist->request->url.userInfo();
>>> + // NOTE: unescape works in-place and may decrease the c-string
>>> length, never increase.
>>> + rfc1738_unescape(const_cast<char*>(tmp.c_str()));
>>>
>>> is troublesome: you are most likely trampling the userinfo data as
>>> rfc1738_unescape is acting on a shared MemBlob (shared by tmp and
>>> userInfo at least) and c_str() doesn't guarantee a COW. The best way
>>> to do this is by defining a SBuf rfc1738_unescape (const SBuf &) which
>>> copies the param if needed.
>>>
>>>
>>> + if (colonPos != 0) {
>>>
>>> shouldn't this be if (colonPos != SBuf::npos) ?
>>
>> No. The edge cases of user-info is that the colon may be the first
>> character to signal missing username, and if ':' is not found at all the
>> entire string is username.
>> So if colonPos==0 then we dont extract one, but all other values
>> regardless including npos we do.
>>
>> The edges case for password is that if colon is missing entirely we have
>> none, all other cases have a password. So the password extraction only
>> happens on colonPos!=npos.
>>
>> Special case is that 0-length username is invalid (unlike absent
>> password). The colonPos==0 case also helps omit replacing any existing
>> username with an empty string - whereas the empty password case *does*
>> replace the pasword with empty string.
>>
>
> To clarify the cases are:
>
> case u:p@...
> - gives new username 'u' and new password 'p'
>
> case u:@...
> - gives new username 'u' and clears previous password (sets new 0-length)
>
> case u@...
> - gives new username 'u' and retains previous password
>
> case :p@...
> - retains previous username and gives new password 'p'
>
> case :@...
> - retains previous username and clears previous password (sets new
> 0-length)
>
> case @...
> - invalid. retain previous username and password.
>
>>
>>>
>>> + memcpy(user, userName.rawContent(), userName.length());
>>> + user[userName.length()] = '\0';
>>>
>>> you could use
>>> SBuf::size_type upTo = min(userName.length(), sizeof(user));
>>> userName.copy(user, upto);
>>> user[upto]='\0';
>>>
>>> Same for
>>> + memcpy(loginbuf,
>>> request->url.userInfo().rawContent(), len);
>>>
>>
>> Thank you for tha pointer. Adding a slight variant, copy() does min()
>> internally and returs the size used so we can drop another line.
>>
>>>
>>>
>>> + static char result[MAX_URL*2]; // should be big enough
>>> for a single URI segment
>>> That conditional is worrisome. Is there any risk of the result not
>>> being big enough?
>>>
>>
>> Oops, small bug there should have been " < sizeof(result)-1" instead of
>> "> 0".
>>
>
> I have a better answer for this now...
>
> The URL parser used a MAX_URL length array to parse the originally
> received user-info/login field so the raw data cannot exceed MAX_URL.
> The *2 is because base64 may double the length on us.
>
> Patch attached that covers all the above.
>
> Amos

-- 
    Francesco