| Message ID | AS8P250MB0744BFB966118D831CEF97E58FDA2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/matroskaenc: Fix potential stack-buffer-overflow | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 7/8/2024 2:24 PM, Andreas Rheinhardt wrote: > The check "left >= INT_MAX - right" is supposed to check for > whether left + right does not overflow/wraparound, but given that > left and top are uint32_t INT_MAX - right can already wraparound > for big values of right (and ordinary 32-bit ints): > If right == UINT32_MAX, INT_MAX - right is INT_MAX + 1; > for left in 0..par->width both checks will be passed. > > Fix this and simplify the check by using 64-bit types, > where the addition is guaranteed not to overflow. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavformat/matroskaenc.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index e1adc0eba6..4b91283119 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -1786,16 +1786,14 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv, > st->codecpar->nb_coded_side_data, > AV_PKT_DATA_FRAME_CROPPING); > if (sd && sd->size == sizeof(uint32_t) * 4) { > - uint32_t top, bottom, left, right; > + uint64_t top, bottom, left, right; > > top = AV_RL32(sd->data + 0); > bottom = AV_RL32(sd->data + 4); > left = AV_RL32(sd->data + 8); > right = AV_RL32(sd->data + 12); > > - if (left >= INT_MAX - right || > - top >= INT_MAX - bottom || > - (left + right) >= par->width || > + if ((left + right) >= par->width || > (top + bottom) >= par->height) { > av_log(s, AV_LOG_ERROR, "Invalid cropping dimensions in stream side data\n"); > return AVERROR(EINVAL); LGTM.
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index e1adc0eba6..4b91283119 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -1786,16 +1786,14 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv, st->codecpar->nb_coded_side_data, AV_PKT_DATA_FRAME_CROPPING); if (sd && sd->size == sizeof(uint32_t) * 4) { - uint32_t top, bottom, left, right; + uint64_t top, bottom, left, right; top = AV_RL32(sd->data + 0); bottom = AV_RL32(sd->data + 4); left = AV_RL32(sd->data + 8); right = AV_RL32(sd->data + 12); - if (left >= INT_MAX - right || - top >= INT_MAX - bottom || - (left + right) >= par->width || + if ((left + right) >= par->width || (top + bottom) >= par->height) { av_log(s, AV_LOG_ERROR, "Invalid cropping dimensions in stream side data\n"); return AVERROR(EINVAL);
The check "left >= INT_MAX - right" is supposed to check for whether left + right does not overflow/wraparound, but given that left and top are uint32_t INT_MAX - right can already wraparound for big values of right (and ordinary 32-bit ints): If right == UINT32_MAX, INT_MAX - right is INT_MAX + 1; for left in 0..par->width both checks will be passed. Fix this and simplify the check by using 64-bit types, where the addition is guaranteed not to overflow. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavformat/matroskaenc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)