[vlc-devel] [PATCH 2/2] include: strings: Add size check to vlc_hex_encode_binary

[Marvin Scholz]

On 17 Apr 2020, at 12:49, Rémi Denis-Courmont wrote:

> Hi,
>
> This warning has served me many times both to find bugs and to clean 
> code up.
>
> If it triggers here, maybe the API should be improved...

It only triggers because the size argument is only used for the assert,
alternatively I could use an if() and abort() so that it always checks 
it
even for non-debug build. (Which might be better than to silently do a
write out of bounds in non-debug cases).

Other than this, I dont see any better way to handle a wrong size here,
as truncation would hide bugs, and adding a return value would be a bit
pointless as its usually a programmer error when the size is too small,
not something that spontaneously happens at runtime only in some cases.

>
> Le 17 avril 2020 09:02:25 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a 
> écrit :
>>
>>
>> On Fri, Apr 17, 2020, at 07:54, Steve Lhomme wrote:
>>> On 2020-04-16 20:58, Thomas Guillem wrote:
>>>>
>>>>
>>>> On Thu, Apr 16, 2020, at 16:05, Marvin Scholz wrote:
>>>>> ---
>>>>>   include/vlc_hash.h    | 16 ++++++++--------
>>>>>   include/vlc_strings.h |  5 +++--
>>>>>   src/text/strings.c    |  6 ++++--
>>>>>   3 files changed, 15 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/include/vlc_hash.h b/include/vlc_hash.h
>>>>> index 1e7757ac58..b5136677d2 100644
>>>>> --- a/include/vlc_hash.h
>>>>> +++ b/include/vlc_hash.h
>>>>> @@ -57,14 +57,14 @@
>>>>>    * \param[out]    output Output buffer to write the string to
>>>>>    */
>>>>>   #ifndef __cplusplus
>>>>> -#define vlc_hash_FinishHex(ctx, output, size)
>>  \
>>>>> -    do {
>>  \
>>>>> -        char out_tmp[_Generic((ctx),
>>  \
>>>>> -            vlc_hash_md5_t *: VLC_HASH_MD5_DIGEST_SIZE)];
>>  \
>>>>> -        _Generic((ctx),
>>  \
>>>>> -            vlc_hash_md5_t *: vlc_hash_md5_Finish)
>>  \
>>>>> -        (ctx, out_tmp, sizeof(out_tmp));
>>  \
>>>>> -        vlc_hex_encode_binary(out_tmp, sizeof(out_tmp), output);
>>  \
>>>>> +#define vlc_hash_FinishHex(ctx, output, size)
>>      \
>>>>> +    do {
>>      \
>>>>> +        char out_tmp[_Generic((ctx),
>>      \
>>>>> +            vlc_hash_md5_t *: VLC_HASH_MD5_DIGEST_SIZE)];
>>      \
>>>>> +        _Generic((ctx),
>>      \
>>>>> +            vlc_hash_md5_t *: vlc_hash_md5_Finish)
>>      \
>>>>> +        (ctx, out_tmp, sizeof(out_tmp));
>>      \
>>>>> +        vlc_hex_encode_binary(out_tmp, sizeof(out_tmp), output,
>> size);  \
>>>>>       } while (0)
>>>>>   #endif
>>>>>
>>>>> diff --git a/include/vlc_strings.h b/include/vlc_strings.h
>>>>> index 930d3347fb..5b4436141f 100644
>>>>> --- a/include/vlc_strings.h
>>>>> +++ b/include/vlc_strings.h
>>>>> @@ -125,10 +125,11 @@ VLC_API char *vlc_xml_encode(const char
>> *str)
>>>>> VLC_MALLOC;
>>>>>    * string in hexadecimal representation.
>>>>>    *
>>>>>    * \param      input    Input buffer
>>>>> - * \param      size     Input buffer size
>>>>> + * \param      in_size  Input buffer size
>>>>>    * \param[out] output   Output buffer to write the string to
>>>>> + * \param      out_size Output buffer size
>>>>>    */
>>>>> -VLC_API void vlc_hex_encode_binary(const void *input, size_t
>> size,
>>>>> char *output);
>>>>> +VLC_API void vlc_hex_encode_binary(const void *input, size_t
>> in_size,
>>>>> char *output, size_t out_size);
>>>>>
>>>>>   /**
>>>>>    * Base64 encoding.
>>>>> diff --git a/src/text/strings.c b/src/text/strings.c
>>>>> index 9676687aae..fc0babc7ce 100644
>>>>> --- a/src/text/strings.c
>>>>> +++ b/src/text/strings.c
>>>>> @@ -347,11 +347,13 @@ char *vlc_xml_encode (const char *str)
>>>>>   }
>>>>>
>>>>>   /* Hex encoding */
>>>>> -void vlc_hex_encode_binary(const void *input, size_t size, char
>> *output)
>>>>> +void vlc_hex_encode_binary(const void *input, size_t in_size,
>>>>> +                           char *output, size_t out_size)
>>>>>   {
>>>>> +    assert(out_size >= in_size * 2 + 1); // 2 chars per byte +
>> null
>>>>
>>>> Missing a (void) out_size; to clear warning on builds with NDEBUG.
>>>
>>> Can we just disable this warning ? We keep adding dummy code just to
>>> silence this warning. I don't think there are cases where it's 
>>> useful
>> to
>>> know we don't use a variable (apart when cleaning an API but then a
>> grep
>>> is still needed to verify all the calls anyway).
>>
>> Personally, this warning saved me more than one time. I would prefer 
>> to
>> keep it.
>>
>>>
>>>> Otherwise, LGTM for the set.
>>>>
>>>>>       const unsigned char *buffer = input;
>>>>>
>>>>> -    for (size_t i = 0; i < size; i++) {
>>>>> +    for (size_t i = 0; i < in_size; i++) {
>>>>>           sprintf(&output[i * 2], "%02hhx", buffer[i]);
>>>>>       }
>>>>>   }
>>>>> -- 
>>>>> 2.24.1 (Apple Git-126)
>>>>>
>>>>> _______________________________________________
>>>>> vlc-devel mailing list
>>>>> To unsubscribe or modify your subscription options:
>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez 
> excuser ma brièveté.


> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel