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

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