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

[Alexandre Janniaux]

Hi,

I agree that the warning is important.

You can probably use VLC_UNUSED() instead of (void) though.

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, Apr 17, 2020 at 08:44:38AM +0200, Marvin Scholz wrote:
>
>
> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel