| Message ID | AS8P250MB07442EDE41C074347931302B8F522@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX | 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 |
On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote: > AVCodecParameters.extradata is supposed to be allocated with > av_malloc(); av_realloc() and its wrappers do not guarantee > the proper alignment. Therefore parse the extradata twice: > Once to check its validity and to determine the eventual size > and a second time to actually write the new extradata. Why would av_malloc alignment be needed for extradata? I see the doxy says "Must be allocated with av_malloc()" but I'm fairly sure that was meant to be "Must be allocated with av_malloc() family of functions", like its AVCodecContext counterpart. The idea is that library users don't use normal malloc as extradata will be freed with av_free(), and not because it will be accessed by SIMD code. > > (Of course, not reallocating the buffer is beneficial in itself.) > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++---------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c > index a695cba370..f5424e95b8 100644 > --- a/libavcodec/bsf/hevc_mp4toannexb.c > +++ b/libavcodec/bsf/hevc_mp4toannexb.c > @@ -38,13 +38,11 @@ typedef struct HEVCBSFContext { > } HEVCBSFContext; > > static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb, > - uint8_t **new_extradatap, > + uint8_t *new_extradata, > size_t *new_extradata_sizep) > { > int num_arrays = bytestream2_get_byte(gb); > - uint8_t *new_extradata = NULL; > size_t new_extradata_size = 0; > - int ret; > > for (int i = 0; i < num_arrays; i++) { > int type = bytestream2_get_byte(gb) & 0x3f; > @@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb, > type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) { > av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n", > type); > - ret = AVERROR_INVALIDDATA; > - goto fail; > + return AVERROR_INVALIDDATA; > } > > for (int j = 0; j < cnt; j++) { > @@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb, > if (!nalu_len || > nalu_len > bytestream2_get_bytes_left(gb) || > 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) { > - ret = AVERROR_INVALIDDATA; > - goto fail; > + return AVERROR_INVALIDDATA; > } > - ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE); > - if (ret < 0) > - goto fail; > - > - AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode > - bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len); > + if (new_extradata) { > + AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode > + bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len); > + } else > + bytestream2_skipu(gb, nalu_len); > new_extradata_size += 4 + nalu_len; > - memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); > } > } > - *new_extradatap = new_extradata; > *new_extradata_sizep = new_extradata_size; > > return 0; > -fail: > - av_freep(&new_extradata); > - return ret; > } > > static int hevc_extradata_to_annexb(AVBSFContext *ctx) > @@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx) > bytestream2_skip(&gb, 21); > length_size = (bytestream2_get_byte(&gb) & 3) + 1; > > - ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata, > - &new_extradata_size); > - if (ret < 0) > - return ret; > + while (1) { > + GetByteContext gb_bak = gb; > + ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata, > + &new_extradata_size); > + if (ret < 0) > + return ret; > + if (new_extradata || !new_extradata_size) > + break; > + new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); > + if (!new_extradata) > + return AVERROR(ENOMEM); > + memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); > + gb = gb_bak; > + } > > av_freep(&ctx->par_out->extradata); > ctx->par_out->extradata = new_extradata;
James Almer: > On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote: >> AVCodecParameters.extradata is supposed to be allocated with >> av_malloc(); av_realloc() and its wrappers do not guarantee >> the proper alignment. Therefore parse the extradata twice: >> Once to check its validity and to determine the eventual size >> and a second time to actually write the new extradata. > > Why would av_malloc alignment be needed for extradata? > I see the doxy says "Must be allocated with av_malloc()" but I'm fairly > sure that was meant to be "Must be allocated with av_malloc() family of > functions", like its AVCodecContext counterpart. The idea is that > library users don't use normal malloc as extradata will be freed with > av_free(), and not because it will be accessed by SIMD code. > I thought that potential accesses by SIMD code was the point? After all, the value of AV_INPUT_BUFFER_PADDING_SIZE (which is used for both packet data and extradata) is chosen so big to accommodate reading via SIMD. You incremented the constant for this very purpose in 6e80079a28. (Granted, I don't think we have code where extradata is being parsed by SIMD.) Apart from that, given its differing alignment, I am not sure that av_realloc() is really part of the av_malloc() family of functions. We should probably replace "av_malloc() family" by "compatible with av_free()" wherever we want to allow av_realloc(), too. Anyway, there is another advantage of this patch: >> >> (Of course, not reallocating the buffer is beneficial in itself.) >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c index a695cba370..f5424e95b8 100644 --- a/libavcodec/bsf/hevc_mp4toannexb.c +++ b/libavcodec/bsf/hevc_mp4toannexb.c @@ -38,13 +38,11 @@ typedef struct HEVCBSFContext { } HEVCBSFContext; static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb, - uint8_t **new_extradatap, + uint8_t *new_extradata, size_t *new_extradata_sizep) { int num_arrays = bytestream2_get_byte(gb); - uint8_t *new_extradata = NULL; size_t new_extradata_size = 0; - int ret; for (int i = 0; i < num_arrays; i++) { int type = bytestream2_get_byte(gb) & 0x3f; @@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb, type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) { av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n", type); - ret = AVERROR_INVALIDDATA; - goto fail; + return AVERROR_INVALIDDATA; } for (int j = 0; j < cnt; j++) { @@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb, if (!nalu_len || nalu_len > bytestream2_get_bytes_left(gb) || 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) { - ret = AVERROR_INVALIDDATA; - goto fail; + return AVERROR_INVALIDDATA; } - ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE); - if (ret < 0) - goto fail; - - AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode - bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len); + if (new_extradata) { + AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode + bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len); + } else + bytestream2_skipu(gb, nalu_len); new_extradata_size += 4 + nalu_len; - memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); } } - *new_extradatap = new_extradata; *new_extradata_sizep = new_extradata_size; return 0; -fail: - av_freep(&new_extradata); - return ret; } static int hevc_extradata_to_annexb(AVBSFContext *ctx) @@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx) bytestream2_skip(&gb, 21); length_size = (bytestream2_get_byte(&gb) & 3) + 1; - ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata, - &new_extradata_size); - if (ret < 0) - return ret; + while (1) { + GetByteContext gb_bak = gb; + ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata, + &new_extradata_size); + if (ret < 0) + return ret; + if (new_extradata || !new_extradata_size) + break; + new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!new_extradata) + return AVERROR(ENOMEM); + memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); + gb = gb_bak; + } av_freep(&ctx->par_out->extradata); ctx->par_out->extradata = new_extradata;
AVCodecParameters.extradata is supposed to be allocated with av_malloc(); av_realloc() and its wrappers do not guarantee the proper alignment. Therefore parse the extradata twice: Once to check its validity and to determine the eventual size and a second time to actually write the new extradata. (Of course, not reallocating the buffer is beneficial in itself.) Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++---------------- 1 file changed, 22 insertions(+), 22 deletions(-)