[FFmpeg-devel] [PATCH] avcodec/dpx: fix check of minimal data size for unpadded content

[Marton Balint]

On Tue, 14 Mar 2023, Jerome Martinez wrote:

> Please consider the attached patch.
>
> Before: "Overread buffer. Invalid header?" despite that all bytes are there 
> (the precheck is wrong, not the parsing after the precheck)
> After: transcoding is fine
>
> A (zeroed) sample file is available at https://trac.ffmpeg.org/ticket/10259

Will apply with a minor fix.

Regards,
Marton

>
> Jérôme
>
> On 19/10/2022 11:47, Jerome Martinez wrote:
>>  stride value is not relevant with unpadded content and the total count of
>>  pixels (width x height) must be used instead of the rounding based on
>>  width only then multiplied by height
>>
>>  unpadded_10bit value computing is moved sooner in the code in order to be
>>  able to use it during computing of minimal content size
>>
>>  Fix 'Overread buffer' error when the content is not lucky enough to have
>>  (enough) padding bytes at the end for not being rejected by the formula
>>  based on the stride value
>>
>>  Signed-off-by: Jerome Martinez <jerome at mediaarea.net>
>>  ---
>>   libavcodec/dpx.c | 34 ++++++++++++++++++----------------
>>   1 file changed, 18 insertions(+), 16 deletions(-)
>>
>>  diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
>>  index 4f50608..d4699f6 100644
>>  --- a/libavcodec/dpx.c
>>  +++ b/libavcodec/dpx.c
>>  @@ -476,14 +476,30 @@ static int decode_frame(AVCodecContext *avctx,
>>  AVFrame *p,
>>           avctx->colorspace = AVCOL_SPC_RGB;
>>       }
>>
>>  +    av_strlcpy(creator, avpkt->data + 160, 100);
>>  +    creator[100] = '\0';
>>  +    av_dict_set(&p->metadata, "Creator", creator, 0);
>>  +
>>  +    av_strlcpy(input_device, avpkt->data + 1556, 32);
>>  +    input_device[32] = '\0';
>>  +    av_dict_set(&p->metadata, "Input Device", input_device, 0);
>>  +
>>  +    // Some devices do not pad 10bit samples to whole 32bit words per row
>>  +    if (!memcmp(input_device, "Scanity", 7) ||
>>  +        !memcmp(creator, "Lasergraphics Inc.", 18)) {
>>  +        unpadded_10bit = 1;
>>  +    }
>>  +
>>       // Table 3c: Runs will always break at scan line boundaries. Packing
>>       // will always break to the next 32-bit word at scan-line boundaries.
>>       // Unfortunately, the encoder produced invalid files, so attempt
>>       // to detect it
>>  +    // Also handle special case with unpadded content
>>       need_align = FFALIGN(stride, 4);
>>  -    if (need_align*avctx->height + (int64_t)offset > avpkt->size) {
>>  +    if (need_align*avctx->height + (int64_t)offset > avpkt->size &&
>>  +        (!unpadded_10bit || (avctx->width * avctx->height * elements + 2)
>>  / 3 * 4 + (int64_t)offset > avpkt->size)) {
>>           // Alignment seems unappliable, try without
>>  -        if (stride*avctx->height + (int64_t)offset > avpkt->size) {
>>  +        if (stride*avctx->height + (int64_t)offset > avpkt->size ||
>>  unpadded_10bit) {
>>               av_log(avctx, AV_LOG_ERROR, "Overread buffer. Invalid
>>  header?\n");
>>               return AVERROR_INVALIDDATA;
>>           } else {
>>  @@ -609,20 +625,6 @@ static int decode_frame(AVCodecContext *avctx,
>>  AVFrame *p,
>>       if ((ret = ff_get_buffer(avctx, p, 0)) < 0)
>>           return ret;
>>
>>  -    av_strlcpy(creator, avpkt->data + 160, 100);
>>  -    creator[100] = '\0';
>>  -    av_dict_set(&p->metadata, "Creator", creator, 0);
>>  -
>>  -    av_strlcpy(input_device, avpkt->data + 1556, 32);
>>  -    input_device[32] = '\0';
>>  -    av_dict_set(&p->metadata, "Input Device", input_device, 0);
>>  -
>>  -    // Some devices do not pad 10bit samples to whole 32bit words per row
>>  -    if (!memcmp(input_device, "Scanity", 7) ||
>>  -        !memcmp(creator, "Lasergraphics Inc.", 18)) {
>>  -        unpadded_10bit = 1;
>>  -    }
>>  -
>>       // Move pointer to offset from start of file
>>       buf =  avpkt->data + offset;
>> 
>