[libdvdnav-devel] [PATCH] src/md5.c: Fix potential strict-aliasing breakage

[Jean-Baptiste Kempf]

To be honest, I would rather we take a better and more recent md5 
implementation file, with less exposed functions, like the LGPL FSF one 
that is now in vlc. (src/misc/md5.c)

Le 25/10/2014 22:11, Andrew Clayton a écrit :
> With GCC 4.8.3 on x86_64 Fedora 20 we get the following warnings from
> src/md5.c
>
>      src/md5.c: In function 'md5_finish_ctx':
>      src/md5.c:102:3: warning: dereferencing type-punned pointer will break
>        strict-aliasing rules [-Wstrict-aliasing]
>       *(md5_uint32 *) &ctx->buffer[bytes + pad] = SWAP (ctx->total[0] << 3);
>       ^
>      src/md5.c:103:3: warning: dereferencing type-punned pointer will break
>        strict-aliasing rules [-Wstrict-aliasing]
>       *(md5_uint32 *) &ctx->buffer[bytes + pad + 4] = SWAP ((ctx->total[1]
>        << 3) |
>       ^
> ctx->total is a 2 element array of type md5_uint32 and ctx->buffer is a
> character array. So the code is dereferencing and casting ctx->buffer to
> a different pointer type.
>
> This code is used by the DVDDiscID() function
>
>      /**
>       * Get a unique 128 bit disc ID.
>       * This is the MD5 sum of VIDEO_TS.IFO and the VTS_0?_0.IFO files
>       * in title order (those that exist).
>
> I got rid of this casting and dereference by using memcpy() to copy the
> total stuff into buffer.
>
> This doesn't seem to be used by libdvdread or indeed vlc or lsdvd, so to
> test this I hacked up a small test program
>
>      /*
>       * gcc -O2 -Wall -o dvddiscid dvddiscid.c -ldvdread
>       */
>
>      #include <stdio.h>
>
>      #include <dvdread/dvd_reader.h>
>
>      int main(int argc, char *argv[])
>      {
> 	    int i;
> 	    dvd_reader_t *dvd;
> 	    unsigned char id[16];
>
> 	    dvd = DVDOpen("/dev/sr0");
> 	    DVDDiscID(dvd, id);
> 	    DVDClose(dvd);
>
> 	    for (i = 0; i < 16; i++)
> 		    printf("%x", id[i]);
> 	    printf("\n");
>
> 	    return 0;
>      }
>
> Testing a DVD gives me the same result both before and after this
> change.
>
> Signed-off-by: Andrew Clayton <andrew at digital-domain.net>
> ---
>   src/md5.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/md5.c b/src/md5.c
> index 9c5f50d..607a067 100644
> --- a/src/md5.c
> +++ b/src/md5.c
> @@ -88,6 +88,7 @@ md5_finish_ctx (ctx, resbuf)
>   {
>     /* Take yet unprocessed bytes into account.  */
>     md5_uint32 bytes = ctx->buflen;
> +  md5_uint32 bits;
>     size_t pad;
>
>     /* Now count remaining bytes.  */
> @@ -99,9 +100,10 @@ md5_finish_ctx (ctx, resbuf)
>     memcpy (&ctx->buffer[bytes], fillbuf, pad);
>
>     /* Put the 64-bit file length in *bits* at the end of the buffer.  */
> -  *(md5_uint32 *) &ctx->buffer[bytes + pad] = SWAP (ctx->total[0] << 3);
> -  *(md5_uint32 *) &ctx->buffer[bytes + pad + 4] = SWAP ((ctx->total[1] << 3) |
> -                                                        (ctx->total[0] >> 29));
> +  bits = SWAP (ctx->total[0] << 3);
> +  memcpy (&ctx->buffer[bytes + pad], &bits, sizeof (bits));
> +  bits = SWAP ((ctx->total[1] << 3) | (ctx->total[0] >> 29));
> +  memcpy (&ctx->buffer[bytes + pad + 4], &bits, sizeof (bits));
>
>     /* Process last bytes.  */
>     md5_process_block (ctx->buffer, bytes + pad + 8, ctx);
>


-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device