On 10/21/20 9:26 PM, Christopher Schultz wrote:
> Denis,
>
> On 10/21/20 14:53, Denis Kovalchuk wrote:
>> Hello.
>>
>> If I am not mistaken, there is an undefined behavior in apr_pbase64_encode()
>> function:
>>
>> encoded = (char *) apr_palloc(p, apr_base64_encode_len(l));
>> l = apr_base64_encode(encoded, string, l);
>> encoded[l] = '\0'; /* make binary sequence into string */
>>
>> encoded[l] is out of bounds, because apr_base64_encode() returns the length
>> of
>> the encoded string, including '\0'.
>
> This is not out-of-bounds. It's just useless. There is no UB here.
The documentation says that apr_base64_encode returns "the length of the
encoded string", but
in fact it delivers "the length of the encoded string" + 1:
*p++ = '\0';
return (int)(p - encoded);
Hence the result of apr_base64_encode is equal to apr_base64_encode_len and thus
encoded[l] would point to the first byte after the memory reserved by
apr_palloc(p, apr_base64_encode_len(l))
I guess we either need to fix the documention of apr_base64_encode or change
the above code to
*p = '\0';
return (int)(p - encoded);
Of course
encoded[l] = '\0';
should still be removed as it is pointless.
>
>> As far as I understand, there is no need to
>> set '\0' at all, because apr_base64_encode() already appends it. Based on
>> this,
>> I suggest to get rid of the explicit '\0' setting.
>
> +1
>
> It looks like r1780034 fixed the initial patch (r1490248) to remove an
> extra byte from the char array, but failed to see that there was another
> mistake in there.
With r1490248 there were just 2 '\0' at the end. Since r1780034 I think we are
writing 1 byte after the allocated memory.
Fortunately this seems to be only in trunk.
Regards
Rüdiger
