Re: [FFmpeg-devel] [PATCH 1/7] avcodec/sunrast: Use ptrdiff_t for stride

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> On 9/30/2022 2:02 PM, Andreas Rheinhardt wrote:
>> Fixes segfaults with negative linesizes; in particular,
>> this affected the sunraster-(1|8|24)bit-(raw|rle) and
>> sunraster-8bit_gray-raw FATE tests.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavcodec/sunrast.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/sunrast.c b/libavcodec/sunrast.c
>> index 77feef06e1..45b29e4d72 100644
>> --- a/libavcodec/sunrast.c
>> +++ b/libavcodec/sunrast.c
>> @@ -31,7 +31,8 @@ static int sunrast_decode_frame(AVCodecContext
>> *avctx, AVFrame *p,
>>   {
>>       const uint8_t *buf       = avpkt->data;
>>       const uint8_t *buf_end   = avpkt->data + avpkt->size;
>> -    unsigned int w, h, depth, type, maptype, maplength, stride, x, y,
>> len, alen;
>> +    unsigned int w, h, depth, type, maptype, maplength, x, y, len, alen;
>> +    ptrdiff_t stride;
>>       uint8_t *ptr, *ptr2 = NULL;
>>       const uint8_t *bufstart = buf;
>>       int ret;
>> @@ -141,7 +142,7 @@ static int sunrast_decode_frame(AVCodecContext
>> *avctx, AVFrame *p,
>>         if (type == RT_BYTE_ENCODED) {
>>           int value, run;
>> -        uint8_t *end = ptr + h * stride;
>> +        uint8_t *end = ptr + (ptrdiff_t)h * stride;
> 
> Is the cast needed if stride is already ptrdiff_t?
> 

Yes. If ptrdiff_t is the signed counterpart of h (an unsigned; i.e. on
32bit systems), h * stride would otherwise be evaluated as an unsigned.
And in this case, it would be a huge positive value, so that the pointer
arithmetic would be undefined. It would nevertheless work in practice
with a flat address space, because the value would be correct modulo
2^32 (modulo SIZE_MAX + 1).

- Andreas

PS: This "it would nevertheless work in practice is not uncommon". E.g.
when using types other than uint8_t to access the frames, we often use
something like "uint16_t *ptr = (uint16_t*)frame->data[0] +
(frame->linesize[0] / sizeof(*ptr)) * h". frame->linesize[0] /
sizeof(*ptr) will (usually) be performed in size_t, with negative
linesizes promoted to huge values. frame->linesize[0] / sizeof(*ptr)
will then be in the lower half of size_t (because the division uses a
logical shift). But given that pointer arithmetic is performed on units
of the pointed-to type, the offset frame->linesize[0] / sizeof(*ptr)
will be multiplied by sizeof(*ptr) and this happens to work correctly
modulo SIZE_MAX + 1 on ordinary systems.
We should nevertheless replace this idiom by "(uint16_t*)(frame->data[0]
+ frame->linesize[0] * h)".
Btw: At lots of places we presuppose that linesize is always divisible
by the size of the type we use to access the data. Yet we only require
linesize to be sufficiently aligned according to the CPU's maxiumum. In
case a CPU has no alignment requirement whatsoever it would be legal to
use any number as linesize, even when it does not divide
sizeof(uint16_t), sizeof(float) etc.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".