| Message ID | DB6PR0101MB2214034DCAA45C10037497958F819@DB6PR0101MB2214.eurprd01.prod.exchangelabs.com |
|---|---|
| State | Accepted |
| Commit | aca09ed7d4832520cf10fb93faed4249726348c0 |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avutil/mem: Handle fast allocations near UINT_MAX properly | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Andreas Rheinhardt (2022-07-05 22:09:37) > av_fast_realloc and av_fast_mallocz? store the size of > the objects they allocate in an unsigned. Yet they overallocate > and currently they can allocate more than UINT_MAX bytes > in case a user has requested a size of about UINT_MAX * 16 / 17 > or more if SIZE_MAX > UINT_MAX. In this case it is impossible > to store the true size of the buffer via the unsigned*; > future requests are likely to use the (re)allocation codepath > even if the buffer is actually large enough because of > the incorrect size. > > Fix this by ensuring that the actually allocated size > always fits into an unsigned. (This entails erroring out > in case the user requested more than UINT_MAX.) I really dislike this av_fast_* naming. Given that all these functions use unsigned int for something that should really be size_t, how about we deprecate them all and replace with something that has a sane naming convention and uses proper types?
Anton Khirnov: > Quoting Andreas Rheinhardt (2022-07-05 22:09:37) >> av_fast_realloc and av_fast_mallocz? store the size of >> the objects they allocate in an unsigned. Yet they overallocate >> and currently they can allocate more than UINT_MAX bytes >> in case a user has requested a size of about UINT_MAX * 16 / 17 >> or more if SIZE_MAX > UINT_MAX. In this case it is impossible >> to store the true size of the buffer via the unsigned*; >> future requests are likely to use the (re)allocation codepath >> even if the buffer is actually large enough because of >> the incorrect size. >> >> Fix this by ensuring that the actually allocated size >> always fits into an unsigned. (This entails erroring out >> in case the user requested more than UINT_MAX.) > > I really dislike this av_fast_* naming. > > Given that all these functions use unsigned int for something that > should really be size_t, how about we deprecate them all and replace > with something that has a sane naming convention and uses proper types? > What name do you suggest? And what's your opinion of the actual patch? - Andreas
Quoting Andreas Rheinhardt (2022-07-06 15:08:05) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2022-07-05 22:09:37) > >> av_fast_realloc and av_fast_mallocz? store the size of > >> the objects they allocate in an unsigned. Yet they overallocate > >> and currently they can allocate more than UINT_MAX bytes > >> in case a user has requested a size of about UINT_MAX * 16 / 17 > >> or more if SIZE_MAX > UINT_MAX. In this case it is impossible > >> to store the true size of the buffer via the unsigned*; > >> future requests are likely to use the (re)allocation codepath > >> even if the buffer is actually large enough because of > >> the incorrect size. > >> > >> Fix this by ensuring that the actually allocated size > >> always fits into an unsigned. (This entails erroring out > >> in case the user requested more than UINT_MAX.) > > > > I really dislike this av_fast_* naming. > > > > Given that all these functions use unsigned int for something that > > should really be size_t, how about we deprecate them all and replace > > with something that has a sane naming convention and uses proper types? > > > > What name do you suggest? I suggested av_?alloc*_reuse() in a recent thread, since those function are "fast" by reusing the buffer when possible. > And what's your opinion of the actual patch? Seems straightforwardly ok.
tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt: > av_fast_realloc and av_fast_mallocz? store the size of > the objects they allocate in an unsigned. Yet they overallocate > and currently they can allocate more than UINT_MAX bytes > in case a user has requested a size of about UINT_MAX * 16 / 17 > or more if SIZE_MAX > UINT_MAX. I think you mean if max_alloc_size > UINT_MAX > In this case it is impossible > to store the true size of the buffer via the unsigned*; > future requests are likely to use the (re)allocation codepath > even if the buffer is actually large enough because of > the incorrect size. > > Fix this by ensuring that the actually allocated size > always fits into an unsigned. (This entails erroring out > in case the user requested more than UINT_MAX.) Who decided unsigned was a good idea in these functions anyway? > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavutil/mem.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavutil/mem.c b/libavutil/mem.c > index a0c9a42849..18aff5291f 100644 > --- a/libavutil/mem.c > +++ b/libavutil/mem.c > @@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int > *size, size_t min_size) > return ptr; > > max_size = atomic_load_explicit(&max_alloc_size, > memory_order_relaxed); > + /* *size is an unsigned, so the real maximum is <= UINT_MAX. */ > + max_size = FFMIN(max_size, UINT_MAX); > > if (min_size > max_size) { > *size = 0; > @@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr, > unsigned int *size, size_t min_size, i > } > > max_size = atomic_load_explicit(&max_alloc_size, > memory_order_relaxed); > + /* *size is an unsigned, so the real maximum is <= UINT_MAX. */ > + max_size = FFMIN(max_size, UINT_MAX); > > if (min_size > max_size) { > av_freep(ptr); Looks OK. This is also why I decided to do formal verification on my av_fast_recalloc() patch. I only verify part of it, so it's vulnerable to this also. This is inspiring me to rework my patch to use size_t instead of unsigned for *size /Tomas
Tomas Härdin: > tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt: >> av_fast_realloc and av_fast_mallocz? store the size of >> the objects they allocate in an unsigned. Yet they overallocate >> and currently they can allocate more than UINT_MAX bytes >> in case a user has requested a size of about UINT_MAX * 16 / 17 >> or more if SIZE_MAX > UINT_MAX. > > I think you mean if max_alloc_size > UINT_MAX > Both are correct. I should probably add a note to the commit message that this whole issue can only be encountered if one has increased the allocation limit by calling av_max_alloc() before that. >> In this case it is impossible >> to store the true size of the buffer via the unsigned*; >> future requests are likely to use the (re)allocation codepath >> even if the buffer is actually large enough because of >> the incorrect size. >> >> Fix this by ensuring that the actually allocated size >> always fits into an unsigned. (This entails erroring out >> in case the user requested more than UINT_MAX.) > > Who decided unsigned was a good idea in these functions anyway? > git log will tell you. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavutil/mem.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavutil/mem.c b/libavutil/mem.c >> index a0c9a42849..18aff5291f 100644 >> --- a/libavutil/mem.c >> +++ b/libavutil/mem.c >> @@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int >> *size, size_t min_size) >> return ptr; >> >> max_size = atomic_load_explicit(&max_alloc_size, >> memory_order_relaxed); >> + /* *size is an unsigned, so the real maximum is <= UINT_MAX. */ >> + max_size = FFMIN(max_size, UINT_MAX); >> >> if (min_size > max_size) { >> *size = 0; >> @@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr, >> unsigned int *size, size_t min_size, i >> } >> >> max_size = atomic_load_explicit(&max_alloc_size, >> memory_order_relaxed); >> + /* *size is an unsigned, so the real maximum is <= UINT_MAX. */ >> + max_size = FFMIN(max_size, UINT_MAX); >> >> if (min_size > max_size) { >> av_freep(ptr); > > Looks OK. This is also why I decided to do formal verification on my > av_fast_recalloc() patch. I only verify part of it, so it's vulnerable > to this also. > > This is inspiring me to rework my patch to use size_t instead of > unsigned for *size See also 3/8. - Andreas
Any update on this patchset? It's mostly fine as I recall /Tomas
tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt: > av_fast_realloc and av_fast_mallocz? store the size of > the objects they allocate in an unsigned. Yet they overallocate > and currently they can allocate more than UINT_MAX bytes > in case a user has requested a size of about UINT_MAX * 16 / 17 > or more if SIZE_MAX > UINT_MAX. In this case it is impossible > to store the true size of the buffer via the unsigned*; > future requests are likely to use the (re)allocation codepath > even if the buffer is actually large enough because of > the incorrect size. > > Fix this by ensuring that the actually allocated size > always fits into an unsigned. (This entails erroring out > in case the user requested more than UINT_MAX.) > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavutil/mem.c | 4 ++++ > 1 file changed, 4 insertions(+) Second bump for this and patch 3/8. This is holding up my rebasing my jpeg2000 patches and indirectly Caleb's htj2k stuff benefiting from them /Tomas
diff --git a/libavutil/mem.c b/libavutil/mem.c index a0c9a42849..18aff5291f 100644 --- a/libavutil/mem.c +++ b/libavutil/mem.c @@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size) return ptr; max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed); + /* *size is an unsigned, so the real maximum is <= UINT_MAX. */ + max_size = FFMIN(max_size, UINT_MAX); if (min_size > max_size) { *size = 0; @@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr, unsigned int *size, size_t min_size, i } max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed); + /* *size is an unsigned, so the real maximum is <= UINT_MAX. */ + max_size = FFMIN(max_size, UINT_MAX); if (min_size > max_size) { av_freep(ptr);
av_fast_realloc and av_fast_mallocz? store the size of the objects they allocate in an unsigned. Yet they overallocate and currently they can allocate more than UINT_MAX bytes in case a user has requested a size of about UINT_MAX * 16 / 17 or more if SIZE_MAX > UINT_MAX. In this case it is impossible to store the true size of the buffer via the unsigned*; future requests are likely to use the (re)allocation codepath even if the buffer is actually large enough because of the incorrect size. Fix this by ensuring that the actually allocated size always fits into an unsigned. (This entails erroring out in case the user requested more than UINT_MAX.) Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavutil/mem.c | 4 ++++ 1 file changed, 4 insertions(+)