Message ID | 20220111204610.14262-1-anton@khirnov.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,01/35] lavu/fifo: disallow overly large fifo sizes | expand |
Anton Khirnov: > The API currently allows creating FIFOs up to > - UINT_MAX: av_fifo_alloc(), av_fifo_realloc(), av_fifo_grow() > - SIZE_MAX: av_fifo_alloc_array() > However the usable limit is determined by > - rndx/wndx being uint32_t > - av_fifo_[size,space] returning int > so no FIFO should be larger than the smallest of > - INT_MAX > - UINT32_MAX > - SIZE_MAX > (which should be INT_MAX an all commonly used platforms). > Return an error on trying to allocate FIFOs larger than this limit. > --- > libavutil/fifo.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/libavutil/fifo.c b/libavutil/fifo.c > index d741bdd395..f2f046b1f3 100644 > --- a/libavutil/fifo.c > +++ b/libavutil/fifo.c > @@ -20,14 +20,23 @@ > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > */ > > +#include <stdint.h> > + > #include "avassert.h" > #include "common.h" > #include "fifo.h" > > +#define FIFO_SIZE_MAX FFMIN3((uint64_t)INT_MAX, (uint64_t)UINT32_MAX, (uint64_t)SIZE_MAX) Aren't these casts unnecessary? And actually dangerous? (They add the implicit requirement that INT_MAX and SIZE_MAX fit into an uint64_t.) > + > AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size) > { > AVFifoBuffer *f; > - void *buffer = av_realloc_array(NULL, nmemb, size); > + void *buffer; > + > + if (nmemb > FIFO_SIZE_MAX / size) > + return NULL; > + > + buffer = av_realloc_array(NULL, nmemb, size); > if (!buffer) > return NULL; > f = av_mallocz(sizeof(AVFifoBuffer)); > @@ -82,6 +91,9 @@ int av_fifo_realloc2(AVFifoBuffer *f, unsigned int new_size) > { > unsigned int old_size = f->end - f->buffer; > > + if (new_size > FIFO_SIZE_MAX) > + return AVERROR(EINVAL); > + > if (old_size < new_size) { > size_t offset_r = f->rptr - f->buffer; > size_t offset_w = f->wptr - f->buffer; >
Quoting Andreas Rheinhardt (2022-01-13 14:59:49) > Anton Khirnov: > > The API currently allows creating FIFOs up to > > - UINT_MAX: av_fifo_alloc(), av_fifo_realloc(), av_fifo_grow() > > - SIZE_MAX: av_fifo_alloc_array() > > However the usable limit is determined by > > - rndx/wndx being uint32_t > > - av_fifo_[size,space] returning int > > so no FIFO should be larger than the smallest of > > - INT_MAX > > - UINT32_MAX > > - SIZE_MAX > > (which should be INT_MAX an all commonly used platforms). > > Return an error on trying to allocate FIFOs larger than this limit. > > --- > > libavutil/fifo.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/libavutil/fifo.c b/libavutil/fifo.c > > index d741bdd395..f2f046b1f3 100644 > > --- a/libavutil/fifo.c > > +++ b/libavutil/fifo.c > > @@ -20,14 +20,23 @@ > > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > > */ > > > > +#include <stdint.h> > > + > > #include "avassert.h" > > #include "common.h" > > #include "fifo.h" > > > > +#define FIFO_SIZE_MAX FFMIN3((uint64_t)INT_MAX, (uint64_t)UINT32_MAX, (uint64_t)SIZE_MAX) > > Aren't these casts unnecessary? Possibly yes. I mainly added them so that FIFO_SIZE_MAX is always the same type, which might prevent surprises. I can drop the casts if you think they are never necessary. > And actually dangerous? (They add the implicit requirement that > INT_MAX and SIZE_MAX fit into an uint64_t.) I'll grant you _theoretically_ dangerous - I've never heard of a posix-compliant platform with sizeof(int) > 64. And in any case that macro should be gone with the old API (was going to include a patch scheduling its removal, but apparently forgot - will send it later).
Anton Khirnov: > Quoting Andreas Rheinhardt (2022-01-13 14:59:49) >> Anton Khirnov: >>> The API currently allows creating FIFOs up to >>> - UINT_MAX: av_fifo_alloc(), av_fifo_realloc(), av_fifo_grow() >>> - SIZE_MAX: av_fifo_alloc_array() >>> However the usable limit is determined by >>> - rndx/wndx being uint32_t >>> - av_fifo_[size,space] returning int >>> so no FIFO should be larger than the smallest of >>> - INT_MAX >>> - UINT32_MAX >>> - SIZE_MAX >>> (which should be INT_MAX an all commonly used platforms). >>> Return an error on trying to allocate FIFOs larger than this limit. >>> --- >>> libavutil/fifo.c | 14 +++++++++++++- >>> 1 file changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavutil/fifo.c b/libavutil/fifo.c >>> index d741bdd395..f2f046b1f3 100644 >>> --- a/libavutil/fifo.c >>> +++ b/libavutil/fifo.c >>> @@ -20,14 +20,23 @@ >>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA >>> */ >>> >>> +#include <stdint.h> >>> + >>> #include "avassert.h" >>> #include "common.h" >>> #include "fifo.h" >>> >>> +#define FIFO_SIZE_MAX FFMIN3((uint64_t)INT_MAX, (uint64_t)UINT32_MAX, (uint64_t)SIZE_MAX) >> >> Aren't these casts unnecessary? > > Possibly yes. I mainly added them so that FIFO_SIZE_MAX is always the > same type, which might prevent surprises. I can drop the casts if you > think they are never necessary. > FIFO_SIZE is the type that results after integer promotions of an expression involving an int, an uint32_t and a size_t parameter; all three constants are representable in it and FIFO_SIZE is representable in an int, an uint32_t and size_t. Given that it denotes a size I think you should cast it to size_t if you want to avoid surprises. >> And actually dangerous? (They add the implicit requirement that >> INT_MAX and SIZE_MAX fit into an uint64_t.) > > I'll grant you _theoretically_ dangerous - I've never heard of a > posix-compliant platform with sizeof(int) > 64. sizeof(int) > 8 you mean? (Or even better: UINT_MAX > UINT64_MAX.) > > And in any case that macro should be gone with the old API (was going to > include a patch scheduling its removal, but apparently forgot - will > send it later). >
diff --git a/libavutil/fifo.c b/libavutil/fifo.c index d741bdd395..f2f046b1f3 100644 --- a/libavutil/fifo.c +++ b/libavutil/fifo.c @@ -20,14 +20,23 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <stdint.h> + #include "avassert.h" #include "common.h" #include "fifo.h" +#define FIFO_SIZE_MAX FFMIN3((uint64_t)INT_MAX, (uint64_t)UINT32_MAX, (uint64_t)SIZE_MAX) + AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size) { AVFifoBuffer *f; - void *buffer = av_realloc_array(NULL, nmemb, size); + void *buffer; + + if (nmemb > FIFO_SIZE_MAX / size) + return NULL; + + buffer = av_realloc_array(NULL, nmemb, size); if (!buffer) return NULL; f = av_mallocz(sizeof(AVFifoBuffer)); @@ -82,6 +91,9 @@ int av_fifo_realloc2(AVFifoBuffer *f, unsigned int new_size) { unsigned int old_size = f->end - f->buffer; + if (new_size > FIFO_SIZE_MAX) + return AVERROR(EINVAL); + if (old_size < new_size) { size_t offset_r = f->rptr - f->buffer; size_t offset_w = f->wptr - f->buffer;