xiph / ogg

Reference implementation of the Ogg media container
BSD 3-Clause "New" or "Revised" License
345 stars 168 forks source link

gcc 7.x warns about overflowing an object's maximum size #50

Closed kcgen closed 5 years ago

kcgen commented 5 years ago
In function ‘_os_lacing_expand’,
    inlined from ‘ogg_stream_pagein.constprop’ at framing.c:814:6:
framing.c:215:8: warning: argument 2 range [18446744073709551484, 18446744073709551612] exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
     ret=_ogg_realloc(os->lacing_vals,lacing_storage*sizeof(*os->lacing_vals));
     ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In file included from framing.c:26:0:
framing.c: In function ‘ogg_stream_pagein.constprop’:
/usr/include/stdlib.h:549:14: note: in a call to allocation function ‘realloc’ declared here
 extern void *realloc (void *__ptr, size_t __size)
              ^~~~~~~

In function ‘_os_lacing_expand’,
    inlined from ‘ogg_stream_pagein.constprop’ at framing.c:814:6:
framing.c:221:8: warning: argument 2 range [18446744073709551352, 18446744073709551608] exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
     ret=_ogg_realloc(os->granule_vals,lacing_storage*
     ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                      sizeof(*os->granule_vals));
                      ~~~~~~~~~~~~~~~~~~~~~~~~~~

Feel free to add -Wall -pedantic CFLAGS to help catch these if they're not showing up. Cheers.

erikd commented 5 years ago

Does this happen without -pedantic? That flag is not very useful because it usually means pedantic with regards to a very old C spec. Its better to use (when you are using GCC) things like -std=c99 to specify a particular standard.

willson-chen commented 5 years ago

I got no warning no matter add -Wall -pedantic or not. My gcc version is 7.4.

rillian commented 5 years ago

I cannot reproduce the warning either. The guards against LONG_MAX were added in 1.3.1 to protect against overflow on systems like win64 where long is 32 bits, but pointers are 64 bits. There a > 1GB packet could fit in memory, but the storage bound could overflow.

On systems with a 64-bit long it's impossible to allocate enough data to overflow the length, even assuming you had time to submit that many ogg pages. However, technically the guard should be against LONG_MAX divided by sizeof(*os->lacing_vals) and it's worth fixing for the sake of 32-bit platforms.

willson-chen commented 5 years ago

@krcroft , could you offer more informations about how to reproduce the warning? such as your build platform, os name and so on. I had trid some times in Ubuntu 18.04 and some other Linux distributions, but got no warning.

kcgen commented 5 years ago

Sorry for the delayed reply; I should have included more detail about architecture. At the time I was compiling for OSX and Windows targets using msys2 and mingw. I will check again with using ogg 1.3.1.

kcgen commented 5 years ago

I can't reproduce this today using the current code and stable compiler revisions (tested 9.1, 8.2, and 7.4). Closing!