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

[Marvin Scholz]

On 17 Apr 2020, at 8:02, Thomas Guillem wrote:

> 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.
>

Agree with Thomas, it's an important warning we should not disable.

>>
>>> 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