Re: [PATCH] base-64 encoder upgrade

On 30/04/11 04:39, Alex Rousskov wrote:
> On 04/29/2011 08:18 AM, Amos Jeffries wrote:
>
>> Are you aware of any convention about src,dst or dst,src pairing and
>> which goes first? All I can think of is the snprintf(out, out_max,
>> ...) example.
>
> I would mimic memcpy() and memmove(): dst, src.
>
> However, std::copy() uses src,dst so you cannot go wrong either way :-)!
>
>
>> I'm tempted to alter the API to be (outBuf, outBufsz, inBuf, inBufSz)
>> before we get too many more callers for the new functions.
>
> Good idea, IMO.
>
>
>> === modified file 'src/HttpHeader.cc'
>> --- src/HttpHeader.cc 2011-03-22 12:23:25 +0000
>> +++ src/HttpHeader.cc 2011-04-29 13:13:00 +0000
>> @@ -1414,7 +1414,10 @@
>> if (!*field) /* no authorization cookie */
>> return NULL;
>>
>> - return base64_decode(field);
>> + static char decodedAuthToken[8192];
>> + int decodedLen = base64_decode(decodedAuthToken, fields, sizeof(decodedAuthToken));
>> + decodedAuthToken[decodedLen] = '\0';
>> + return decodedAuthToken;
>> }
>
> s/fields/field/ ?
>

meh. yeah and a few other bits I've found tonight while build-testing.

>
> Looking at this and other caller changes, it feels like you need
> base64_decode_and_terminate() that will handle termination safely. If
> you add that, please check that 64_decode_len() and/or its documentation
> is adjusted accordingly.

This is the only decode caller which needs termination anymore. The
others just map the output to a binary packet structure. I'm pretty sure
this one can go as well with some fiddling, but that is not in scope.

>
> It feels wrong that we ignore all decoding errors, but fixing that may
> be outside your patch scope.

Which errors do you mean? the decoder skipping non-decodeable characters?

On the other end the callers of the src/HttpHeader.cc getAuth() method
handle incorrect output by validating the username:password string
decoded. In the NTLM helpers where the other decoding is done the
resulting decoded packet is validated for various binary properties
including decoded length.

>
>> === modified file 'lib/base64.c'
>> +int
>> +base64_decode_len(const char *data)
>> +{
>> + if (!data || !*data)
>> + return 0;
>> +
>> + int terminatorLen = 0;
>> + int dataLen = strlen(data);
>
> s/int dataLen/const int dataLen/
>
> The !*data check is probably not needed.
>
> BTW, I am not sure, but it is possible that base64_decode_len() can be
> implemented more efficiently and/or more elegantly using strcspn(data,
> "="), avoiding searching for \0 and then back-searching for \=. Not a
> big deal though, even if strcspn() is indeed faster.
>

Good point. I think '=' is invalid within the encoded string. But not
completely sure. The current version will ignore it.

Fixed the other comments. Patch v3 pending build tests.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.7 and 3.1.12.1