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

[Rémi Denis-Courmont]

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

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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200417/046b55b5/attachment.html>