[FFmpeg-devel] [PATCH v2] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

On Thu, Dec 26, 2019 at 8:35 PM Nicolas George <george at nsup.org> wrote:

> Andreas Rheinhardt (12019-12-26):
> > b) It guarantees to not allocated more than UINT_MAX - 1 elements, so
> > the caller needn't check for overflow if the desired size is increased
> > in steps of one.
>
> This is preparing trouble for later,


I don't understand. Do you think that callers will take this as a blank
cheque to not check at all?


> and as Michael pointed, it will not
> work when the number of elements is stored in a signed integer.


My intention was actually to convert the types for every user of this
function when using this function (see e.g. the -Matroska demuxer patch).


> (It was
> a mistake to use a signed integer in the first place, but that
> particular mistake is already all over the place).


I agree.


> I strongly suggest we
> try to make things properly:
>
> - Make nb_allocated and min_nb size_t as they should be.
>

Do we have arrays where there is a need to go beyond the range of ordinary
integers for the number of elements?

>
> - Do not hard-code UINT_MAX-1, make it an argument, max_nb: that way, if
>   the index is an integer, just pass INT_MAX-1, and if it is properly
>   size_t, pass SIZE_MAX-1.
>
> - Since we can't pass a pointer to int as a pointer to size_t, add a
>   small convenience wrapper av_fast_realloc_array_int() just to convert
>   the type of nb_allocated. That's ugly, but I do not see a better
>   solution. Or we can decide we must change the type of the size to
>   size_t whenever we update code to use av_fast_realloc_array().
>

I'd like not to turn the int/unsigned version of av_fast_realloc_array()
into a wrapper until there is a real case where an array is needed that
doesn't fit into an int (or an unsigned). And switching to size_t
everywhere would increase the size of certain structures which I very much
like to avoid (e.g. the size of every MatroskaIndex element would increase
by eight bytes (for 64 bit systems)).

>
> And while we are at it, I would like better:
>
> - Alter ptr the same way nb_allocated is altered, and return a real
>   error code in case of error. Otherwise, the caller has to guess that
>   only AVERROR(ENOMEM) is possible, and we have seen that these
>   assumptions and hard-coded error code have a way of overstaying their
>   welcome.
>

That is certainly possible (if "alter ptr the same way nb_allocated is
altered" means: Via a pointer to a pointer. (The other interpretation (that
the array should be automatically freed on failure and ptr set to NULL)
would have elicited a different response.))

>
> Poor choice of types have caused problems in the past. They will cause
> more problems as the memory of computers, and therefore the task we give
> them, grow. Let us not cultivate them.
>
> Regards,
>
> --
>   Nicolas George
>

Thanks for looking over this.

- Andreas