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

[Thomas Guillem]

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