| Message ID | GV1P250MB073747D27D2794CFC913647A8F569@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 02c19dfb8552cb40b02ff9c8d8d3c5e12d5c71e1 |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avcodec/sunrast: Use ptrdiff_t for stride | expand |
Andreas Rheinhardt: > 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; > > x = 0; > while (ptr != end && buf < buf_end) { Will apply this patchset tonight unless there are objections. - Andreas
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? > > x = 0; > while (ptr != end && buf < buf_end) {
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.
fre 2022-09-30 klockan 19:05 +0200 skrev Andreas Rheinhardt: > SGI is intra-frame only; the decoder therefore does not > maintain any state between frames, so remove > the private context. > > Also rename depth to nb_components. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/sgidec.c | 158 ++++++++++++++++++++---------------------- > -- > 1 file changed, 73 insertions(+), 85 deletions(-) Looks OK /Tomas
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; x = 0; while (ptr != end && buf < buf_end) {
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(-)