| Message ID | 20191125210920.1874-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: > Signed-off-by: Paul B Mahol <onemda@gmail.com> > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame) > +{ > + GetByteContext *gb = &s->gb; > + GetBitContext mask; > + GetByteContext idx9; > + uint16_t nb_vectors, intra_flag; > + const uint8_t *vec; > + const uint8_t *mask_start; > + uint8_t *skip; > + int mask_size; > + int idx9bits = 0; > + int idx9val = 0; > + int num_blocks; > + > + nb_vectors = bytestream2_get_le16(gb); > + intra_flag = bytestream2_get_le16(gb); > + if (intra_flag) { > + num_blocks = (avctx->width / 2) * (avctx->height / 2); Will UB if width*height/4 > INT_MAX > + } else { > + int skip_linesize; > + > + num_blocks = bytestream2_get_le32(gb); Might want to use uint32_t so this doesn't lead to weirdness on 32-bit > + skip_linesize = avctx->width >> 1; > + mask_start = gb->buffer_start + bytestream2_tell(gb); > + mask_size = (avctx->width >> 5) * (avctx->height >> 2); This can also UB /Tomas
On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame >> *frame) >> +{ >> + GetByteContext *gb = &s->gb; >> + GetBitContext mask; >> + GetByteContext idx9; >> + uint16_t nb_vectors, intra_flag; >> + const uint8_t *vec; >> + const uint8_t *mask_start; >> + uint8_t *skip; >> + int mask_size; >> + int idx9bits = 0; >> + int idx9val = 0; >> + int num_blocks; >> + >> + nb_vectors = bytestream2_get_le16(gb); >> + intra_flag = bytestream2_get_le16(gb); >> + if (intra_flag) { >> + num_blocks = (avctx->width / 2) * (avctx->height / 2); > > Will UB if width*height/4 > INT_MAX > >> + } else { >> + int skip_linesize; >> + >> + num_blocks = bytestream2_get_le32(gb); > > Might want to use uint32_t so this doesn't lead to weirdness on 32-bit > >> + skip_linesize = avctx->width >> 1; >> + mask_start = gb->buffer_start + bytestream2_tell(gb); >> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); > > This can also UB > > /Tomas > > _______________________________________________ > 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". Nothing of this can actually happen. Applied.
Am Di., 26. Nov. 2019 um 10:53 Uhr schrieb Paul B Mahol <onemda@gmail.com>: > > On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: > >> Signed-off-by: Paul B Mahol <onemda@gmail.com> > >> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame > >> *frame) > >> +{ > >> + GetByteContext *gb = &s->gb; > >> + GetBitContext mask; > >> + GetByteContext idx9; > >> + uint16_t nb_vectors, intra_flag; > >> + const uint8_t *vec; > >> + const uint8_t *mask_start; > >> + uint8_t *skip; > >> + int mask_size; > >> + int idx9bits = 0; > >> + int idx9val = 0; > >> + int num_blocks; > >> + > >> + nb_vectors = bytestream2_get_le16(gb); > >> + intra_flag = bytestream2_get_le16(gb); > >> + if (intra_flag) { > >> + num_blocks = (avctx->width / 2) * (avctx->height / 2); > > > > Will UB if width*height/4 > INT_MAX > > > >> + } else { > >> + int skip_linesize; > >> + > >> + num_blocks = bytestream2_get_le32(gb); > > > > Might want to use uint32_t so this doesn't lead to weirdness on 32-bit > > > >> + skip_linesize = avctx->width >> 1; > >> + mask_start = gb->buffer_start + bytestream2_tell(gb); > >> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); > > > > This can also UB > Nothing of this can actually happen. Then you could add asserts (and cut your quotes). Carl Eugen
On 11/26/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Di., 26. Nov. 2019 um 10:53 Uhr schrieb Paul B Mahol <onemda@gmail.com>: >> >> On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >> > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> >> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >> >> AVFrame >> >> *frame) >> >> +{ >> >> + GetByteContext *gb = &s->gb; >> >> + GetBitContext mask; >> >> + GetByteContext idx9; >> >> + uint16_t nb_vectors, intra_flag; >> >> + const uint8_t *vec; >> >> + const uint8_t *mask_start; >> >> + uint8_t *skip; >> >> + int mask_size; >> >> + int idx9bits = 0; >> >> + int idx9val = 0; >> >> + int num_blocks; >> >> + >> >> + nb_vectors = bytestream2_get_le16(gb); >> >> + intra_flag = bytestream2_get_le16(gb); >> >> + if (intra_flag) { >> >> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >> > >> > Will UB if width*height/4 > INT_MAX >> > >> >> + } else { >> >> + int skip_linesize; >> >> + >> >> + num_blocks = bytestream2_get_le32(gb); >> > >> > Might want to use uint32_t so this doesn't lead to weirdness on 32-bit >> > >> >> + skip_linesize = avctx->width >> 1; >> >> + mask_start = gb->buffer_start + bytestream2_tell(gb); >> >> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >> > >> > This can also UB > >> Nothing of this can actually happen. > > Then you could add asserts (and cut your quotes). Asserts for checking UBs? Non sense. > > Carl Eugen > _______________________________________________ > 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".
Paul B Mahol (12019-11-26): > >> Nothing of this can actually happen. > > Then you could add asserts (and cut your quotes). > Asserts for checking UBs? Non sense. Assert to have debug builds check that the things you say cannot happen do not actually happen. That is what asserts are for: check a code invariant that is useful but not obvious. Regards,
On 11/26/19, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12019-11-26): >> >> Nothing of this can actually happen. >> > Then you could add asserts (and cut your quotes). >> Asserts for checking UBs? Non sense. > > Assert to have debug builds check that the things you say cannot happen > do not actually happen. That is what asserts are for: check a code > invariant that is useful but not obvious. > Thank you for reminding me what are asserts for. I will not bloat code with pointless asserts or other kind of lines.
On 11/26/2019 6:47 AM, Paul B Mahol wrote: > On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame >>> *frame) >>> +{ >>> + GetByteContext *gb = &s->gb; >>> + GetBitContext mask; >>> + GetByteContext idx9; >>> + uint16_t nb_vectors, intra_flag; >>> + const uint8_t *vec; >>> + const uint8_t *mask_start; >>> + uint8_t *skip; >>> + int mask_size; >>> + int idx9bits = 0; >>> + int idx9val = 0; >>> + int num_blocks; >>> + >>> + nb_vectors = bytestream2_get_le16(gb); >>> + intra_flag = bytestream2_get_le16(gb); >>> + if (intra_flag) { >>> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >> >> Will UB if width*height/4 > INT_MAX >> >>> + } else { >>> + int skip_linesize; >>> + >>> + num_blocks = bytestream2_get_le32(gb); >> >> Might want to use uint32_t so this doesn't lead to weirdness on 32-bit >> >>> + skip_linesize = avctx->width >> 1; >>> + mask_start = gb->buffer_start + bytestream2_tell(gb); >>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >> >> This can also UB >> >> /Tomas >> >> _______________________________________________ >> 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". > > Nothing of this can actually happen. It can and i'm fairly sure it will happen as soon as the fuzzer starts testing this decoder using big dimensions. You don't need asserts here, you just need to check the calculations will not overflow. Do something like "if ((int64_t)avctx->width * avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a day. Also, maybe num_blocks should be unsigned, seeing you set it using bytestream2_get_le32() for P-frames. > > Applied. > _______________________________________________ > 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". >
On 11/25/2019 6:09 PM, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/Makefile | 1 + > libavcodec/allcodecs.c | 1 + > libavcodec/avcodec.h | 1 + > libavcodec/codec_desc.c | 7 ++ > libavcodec/midivid.c | 272 ++++++++++++++++++++++++++++++++++++++++ > libavformat/riff.c | 1 + > 6 files changed, 283 insertions(+) > create mode 100644 libavcodec/midivid.c [...] > diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c > new file mode 100644 > index 0000000000..36ed4b83bd > --- /dev/null > +++ b/libavcodec/midivid.c > @@ -0,0 +1,272 @@ > +/* > + * MidiVid decoder > + * Copyright (c) 2019 Paul B Mahol > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > + > +#include "libavutil/imgutils.h" > +#include "libavutil/internal.h" > +#include "libavutil/intreadwrite.h" > +#include "libavutil/mem.h" > + > +#define BITSTREAM_READER_LE > +#include "avcodec.h" > +#include "get_bits.h" > +#include "bytestream.h" > +#include "internal.h" > + > +typedef struct MidiVidContext { > + GetByteContext gb; > + > + uint8_t *uncompressed; > + int uncompressed_size; Should be unsigned int, seeing it's used as argument for av_fast_padded_malloc(). > + uint8_t *skip; > + > + AVFrame *frame; > +} MidiVidContext; > + > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame) > +{ > + GetByteContext *gb = &s->gb; > + GetBitContext mask; > + GetByteContext idx9; > + uint16_t nb_vectors, intra_flag; > + const uint8_t *vec; > + const uint8_t *mask_start; > + uint8_t *skip; > + int mask_size; > + int idx9bits = 0; > + int idx9val = 0; > + int num_blocks; > + > + nb_vectors = bytestream2_get_le16(gb); > + intra_flag = bytestream2_get_le16(gb); > + if (intra_flag) { > + num_blocks = (avctx->width / 2) * (avctx->height / 2); > + } else { > + int skip_linesize; > + > + num_blocks = bytestream2_get_le32(gb); > + skip_linesize = avctx->width >> 1; > + mask_start = gb->buffer_start + bytestream2_tell(gb); > + mask_size = (avctx->width >> 5) * (avctx->height >> 2); > + init_get_bits8(&mask, mask_start, mask_size); > + bytestream2_skip(gb, mask_size); > + skip = s->skip; > + > + for (int y = 0; y < avctx->height >> 2; y++) { > + for (int x = 0; x < avctx->width >> 2; x++) { > + int flag = !get_bits1(&mask); > + > + skip[(y*2) *skip_linesize + x*2 ] = flag; > + skip[(y*2) *skip_linesize + x*2+1] = flag; > + skip[(y*2+1)*skip_linesize + x*2 ] = flag; > + skip[(y*2+1)*skip_linesize + x*2+1] = flag; > + } > + } > + } > + > + vec = gb->buffer_start + bytestream2_tell(gb); Isn't this the same as doing vec = g->buffer? > + if (bytestream2_get_bytes_left(gb) < nb_vectors * 12) > + return AVERROR_INVALIDDATA; > + bytestream2_skip(gb, nb_vectors * 12); > + if (nb_vectors > 256) { > + if (bytestream2_get_bytes_left(gb) < (num_blocks + 7) / 8) > + return AVERROR_INVALIDDATA; > + bytestream2_init(&idx9, gb->buffer_start + bytestream2_tell(gb), (num_blocks + 7) / 8); Ditto.
On 11/26/19, James Almer <jamrial@gmail.com> wrote: > On 11/25/2019 6:09 PM, Paul B Mahol wrote: >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavcodec/Makefile | 1 + >> libavcodec/allcodecs.c | 1 + >> libavcodec/avcodec.h | 1 + >> libavcodec/codec_desc.c | 7 ++ >> libavcodec/midivid.c | 272 ++++++++++++++++++++++++++++++++++++++++ >> libavformat/riff.c | 1 + >> 6 files changed, 283 insertions(+) >> create mode 100644 libavcodec/midivid.c > > [...] > >> diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c >> new file mode 100644 >> index 0000000000..36ed4b83bd >> --- /dev/null >> +++ b/libavcodec/midivid.c >> @@ -0,0 +1,272 @@ >> +/* >> + * MidiVid decoder >> + * Copyright (c) 2019 Paul B Mahol >> + * >> + * This file is part of FFmpeg. >> + * >> + * FFmpeg is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * FFmpeg is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with FFmpeg; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> 02110-1301 USA >> + */ >> + >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <string.h> >> + >> +#include "libavutil/imgutils.h" >> +#include "libavutil/internal.h" >> +#include "libavutil/intreadwrite.h" >> +#include "libavutil/mem.h" >> + >> +#define BITSTREAM_READER_LE >> +#include "avcodec.h" >> +#include "get_bits.h" >> +#include "bytestream.h" >> +#include "internal.h" >> + >> +typedef struct MidiVidContext { >> + GetByteContext gb; >> + >> + uint8_t *uncompressed; >> + int uncompressed_size; > > Should be unsigned int, seeing it's used as argument for > av_fast_padded_malloc(). Ok. > >> + uint8_t *skip; >> + >> + AVFrame *frame; >> +} MidiVidContext; >> + >> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame >> *frame) >> +{ >> + GetByteContext *gb = &s->gb; >> + GetBitContext mask; >> + GetByteContext idx9; >> + uint16_t nb_vectors, intra_flag; >> + const uint8_t *vec; >> + const uint8_t *mask_start; >> + uint8_t *skip; >> + int mask_size; >> + int idx9bits = 0; >> + int idx9val = 0; >> + int num_blocks; >> + >> + nb_vectors = bytestream2_get_le16(gb); >> + intra_flag = bytestream2_get_le16(gb); >> + if (intra_flag) { >> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >> + } else { >> + int skip_linesize; >> + >> + num_blocks = bytestream2_get_le32(gb); >> + skip_linesize = avctx->width >> 1; >> + mask_start = gb->buffer_start + bytestream2_tell(gb); >> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >> + init_get_bits8(&mask, mask_start, mask_size); >> + bytestream2_skip(gb, mask_size); >> + skip = s->skip; >> + >> + for (int y = 0; y < avctx->height >> 2; y++) { >> + for (int x = 0; x < avctx->width >> 2; x++) { >> + int flag = !get_bits1(&mask); >> + >> + skip[(y*2) *skip_linesize + x*2 ] = flag; >> + skip[(y*2) *skip_linesize + x*2+1] = flag; >> + skip[(y*2+1)*skip_linesize + x*2 ] = flag; >> + skip[(y*2+1)*skip_linesize + x*2+1] = flag; >> + } >> + } >> + } >> + >> + vec = gb->buffer_start + bytestream2_tell(gb); > > Isn't this the same as doing vec = g->buffer? No point in changing now. > >> + if (bytestream2_get_bytes_left(gb) < nb_vectors * 12) >> + return AVERROR_INVALIDDATA; >> + bytestream2_skip(gb, nb_vectors * 12); >> + if (nb_vectors > 256) { >> + if (bytestream2_get_bytes_left(gb) < (num_blocks + 7) / 8) >> + return AVERROR_INVALIDDATA; >> + bytestream2_init(&idx9, gb->buffer_start + bytestream2_tell(gb), >> (num_blocks + 7) / 8); > > Ditto. > _______________________________________________ > 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".
On 11/26/19, James Almer <jamrial@gmail.com> wrote: > On 11/26/2019 6:47 AM, Paul B Mahol wrote: >> On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >>>> AVFrame >>>> *frame) >>>> +{ >>>> + GetByteContext *gb = &s->gb; >>>> + GetBitContext mask; >>>> + GetByteContext idx9; >>>> + uint16_t nb_vectors, intra_flag; >>>> + const uint8_t *vec; >>>> + const uint8_t *mask_start; >>>> + uint8_t *skip; >>>> + int mask_size; >>>> + int idx9bits = 0; >>>> + int idx9val = 0; >>>> + int num_blocks; >>>> + >>>> + nb_vectors = bytestream2_get_le16(gb); >>>> + intra_flag = bytestream2_get_le16(gb); >>>> + if (intra_flag) { >>>> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >>> >>> Will UB if width*height/4 > INT_MAX >>> >>>> + } else { >>>> + int skip_linesize; >>>> + >>>> + num_blocks = bytestream2_get_le32(gb); >>> >>> Might want to use uint32_t so this doesn't lead to weirdness on 32-bit >>> >>>> + skip_linesize = avctx->width >> 1; >>>> + mask_start = gb->buffer_start + bytestream2_tell(gb); >>>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >>> >>> This can also UB >>> >>> /Tomas >>> >>> _______________________________________________ >>> 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". >> >> Nothing of this can actually happen. > > It can and i'm fairly sure it will happen as soon as the fuzzer starts > testing this decoder using big dimensions. I'm not that guy doing such work. Please stop bikesheding those patches for once. > > You don't need asserts here, you just need to check the calculations > will not overflow. Do something like "if ((int64_t)avctx->width * > avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a day. > Also, maybe num_blocks should be unsigned, seeing you set it using > bytestream2_get_le32() for P-frames. No decoder does this. > >> >> Applied. >> _______________________________________________ >> 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". >> > > _______________________________________________ > 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".
On 11/26/2019 4:29 PM, Paul B Mahol wrote: > On 11/26/19, James Almer <jamrial@gmail.com> wrote: >> On 11/26/2019 6:47 AM, Paul B Mahol wrote: >>> On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >>>>> AVFrame >>>>> *frame) >>>>> +{ >>>>> + GetByteContext *gb = &s->gb; >>>>> + GetBitContext mask; >>>>> + GetByteContext idx9; >>>>> + uint16_t nb_vectors, intra_flag; >>>>> + const uint8_t *vec; >>>>> + const uint8_t *mask_start; >>>>> + uint8_t *skip; >>>>> + int mask_size; >>>>> + int idx9bits = 0; >>>>> + int idx9val = 0; >>>>> + int num_blocks; >>>>> + >>>>> + nb_vectors = bytestream2_get_le16(gb); >>>>> + intra_flag = bytestream2_get_le16(gb); >>>>> + if (intra_flag) { >>>>> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >>>> >>>> Will UB if width*height/4 > INT_MAX >>>> >>>>> + } else { >>>>> + int skip_linesize; >>>>> + >>>>> + num_blocks = bytestream2_get_le32(gb); >>>> >>>> Might want to use uint32_t so this doesn't lead to weirdness on 32-bit >>>> >>>>> + skip_linesize = avctx->width >> 1; >>>>> + mask_start = gb->buffer_start + bytestream2_tell(gb); >>>>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >>>> >>>> This can also UB >>>> >>>> /Tomas >>>> >>>> _______________________________________________ >>>> 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". >>> >>> Nothing of this can actually happen. >> >> It can and i'm fairly sure it will happen as soon as the fuzzer starts >> testing this decoder using big dimensions. > > I'm not that guy doing such work. Please stop bikesheding those > patches for once. > >> >> You don't need asserts here, you just need to check the calculations >> will not overflow. Do something like "if ((int64_t)avctx->width * >> avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a day. >> Also, maybe num_blocks should be unsigned, seeing you set it using >> bytestream2_get_le32() for P-frames. > > No decoder does this. Most decoders call av_image_check_size2() directly or indirectly to set dimensions, which does w*h overflow checks similar to the one above. > >> >>> >>> Applied. >>> _______________________________________________ >>> 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". >>> >> >> _______________________________________________ >> 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". > _______________________________________________ > 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". >
On 11/26/19, James Almer <jamrial@gmail.com> wrote: > On 11/26/2019 4:29 PM, Paul B Mahol wrote: >> On 11/26/19, James Almer <jamrial@gmail.com> wrote: >>> On 11/26/2019 6:47 AM, Paul B Mahol wrote: >>>> On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >>>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >>>>>> AVFrame >>>>>> *frame) >>>>>> +{ >>>>>> + GetByteContext *gb = &s->gb; >>>>>> + GetBitContext mask; >>>>>> + GetByteContext idx9; >>>>>> + uint16_t nb_vectors, intra_flag; >>>>>> + const uint8_t *vec; >>>>>> + const uint8_t *mask_start; >>>>>> + uint8_t *skip; >>>>>> + int mask_size; >>>>>> + int idx9bits = 0; >>>>>> + int idx9val = 0; >>>>>> + int num_blocks; >>>>>> + >>>>>> + nb_vectors = bytestream2_get_le16(gb); >>>>>> + intra_flag = bytestream2_get_le16(gb); >>>>>> + if (intra_flag) { >>>>>> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >>>>> >>>>> Will UB if width*height/4 > INT_MAX >>>>> >>>>>> + } else { >>>>>> + int skip_linesize; >>>>>> + >>>>>> + num_blocks = bytestream2_get_le32(gb); >>>>> >>>>> Might want to use uint32_t so this doesn't lead to weirdness on 32-bit >>>>> >>>>>> + skip_linesize = avctx->width >> 1; >>>>>> + mask_start = gb->buffer_start + bytestream2_tell(gb); >>>>>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >>>>> >>>>> This can also UB >>>>> >>>>> /Tomas >>>>> >>>>> _______________________________________________ >>>>> 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". >>>> >>>> Nothing of this can actually happen. >>> >>> It can and i'm fairly sure it will happen as soon as the fuzzer starts >>> testing this decoder using big dimensions. >> >> I'm not that guy doing such work. Please stop bikesheding those >> patches for once. >> >>> >>> You don't need asserts here, you just need to check the calculations >>> will not overflow. Do something like "if ((int64_t)avctx->width * >>> avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a >>> day. >>> Also, maybe num_blocks should be unsigned, seeing you set it using >>> bytestream2_get_le32() for P-frames. >> >> No decoder does this. > > Most decoders call av_image_check_size2() directly or indirectly to set > dimensions, which does w*h overflow checks similar to the one above. > Only if they store width/height out of container.
tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol: > On 11/26/19, James Almer <jamrial@gmail.com> wrote: > > On 11/26/2019 6:47 AM, Paul B Mahol wrote: > > > On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, > > > > > AVFrame > > > > > *frame) > > > > > +{ > > > > > + GetByteContext *gb = &s->gb; > > > > > + GetBitContext mask; > > > > > + GetByteContext idx9; > > > > > + uint16_t nb_vectors, intra_flag; > > > > > + const uint8_t *vec; > > > > > + const uint8_t *mask_start; > > > > > + uint8_t *skip; > > > > > + int mask_size; > > > > > + int idx9bits = 0; > > > > > + int idx9val = 0; > > > > > + int num_blocks; > > > > > + > > > > > + nb_vectors = bytestream2_get_le16(gb); > > > > > + intra_flag = bytestream2_get_le16(gb); > > > > > + if (intra_flag) { > > > > > + num_blocks = (avctx->width / 2) * (avctx->height / 2); > > > > > > > > Will UB if width*height/4 > INT_MAX > > > > > > > > > + } else { > > > > > + int skip_linesize; > > > > > + > > > > > + num_blocks = bytestream2_get_le32(gb); > > > > > > > > Might want to use uint32_t so this doesn't lead to weirdness on 32-bit > > > > > > > > > + skip_linesize = avctx->width >> 1; > > > > > + mask_start = gb->buffer_start + bytestream2_tell(gb); > > > > > + mask_size = (avctx->width >> 5) * (avctx->height >> 2); > > > > > > > > This can also UB > > > > > > > > /Tomas > > > > > > > > > > Nothing of this can actually happen. This assumes max_pixels will never be increased above INT_MAX. "64k" video is most definitely within the range of possibility in the coming years, if it isn't already. Film archival and DPX come to mind. > > It can and i'm fairly sure it will happen as soon as the fuzzer starts > > testing this decoder using big dimensions. > > I'm not that guy doing such work. Please stop bikesheding those > patches for once. This reads like an admission of pushing insecure code via "not my problem" > > You don't need asserts here, you just need to check the calculations > > will not overflow. Do something like "if ((int64_t)avctx->width * > > avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a day. > > Also, maybe num_blocks should be unsigned, seeing you set it using > > bytestream2_get_le32() for P-frames. > > No decoder does this. zmbv does All this is really about the lack of any way to reason about assumptions like "dimensions can't be larger than X*Y" at compile time, which is a thing I've been pointing out on this list for a while now. /Tomas
On 11/27/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol: >> On 11/26/19, James Almer <jamrial@gmail.com> wrote: >> > On 11/26/2019 6:47 AM, Paul B Mahol wrote: >> > > On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >> > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >> > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> >> > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >> > > > > AVFrame >> > > > > *frame) >> > > > > +{ >> > > > > + GetByteContext *gb = &s->gb; >> > > > > + GetBitContext mask; >> > > > > + GetByteContext idx9; >> > > > > + uint16_t nb_vectors, intra_flag; >> > > > > + const uint8_t *vec; >> > > > > + const uint8_t *mask_start; >> > > > > + uint8_t *skip; >> > > > > + int mask_size; >> > > > > + int idx9bits = 0; >> > > > > + int idx9val = 0; >> > > > > + int num_blocks; >> > > > > + >> > > > > + nb_vectors = bytestream2_get_le16(gb); >> > > > > + intra_flag = bytestream2_get_le16(gb); >> > > > > + if (intra_flag) { >> > > > > + num_blocks = (avctx->width / 2) * (avctx->height / 2); >> > > > >> > > > Will UB if width*height/4 > INT_MAX >> > > > >> > > > > + } else { >> > > > > + int skip_linesize; >> > > > > + >> > > > > + num_blocks = bytestream2_get_le32(gb); >> > > > >> > > > Might want to use uint32_t so this doesn't lead to weirdness on >> > > > 32-bit >> > > > >> > > > > + skip_linesize = avctx->width >> 1; >> > > > > + mask_start = gb->buffer_start + bytestream2_tell(gb); >> > > > > + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >> > > > >> > > > This can also UB >> > > > >> > > > /Tomas >> > > > >> > > >> > > Nothing of this can actually happen. > > This assumes max_pixels will never be increased above INT_MAX. "64k" > video is most definitely within the range of possibility in the coming > years, if it isn't already. Film archival and DPX come to mind. > >> > It can and i'm fairly sure it will happen as soon as the fuzzer starts >> > testing this decoder using big dimensions. >> >> I'm not that guy doing such work. Please stop bikesheding those >> patches for once. > > This reads like an admission of pushing insecure code via "not my > problem" > >> > You don't need asserts here, you just need to check the calculations >> > will not overflow. Do something like "if ((int64_t)avctx->width * >> > avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a >> > day. >> > Also, maybe num_blocks should be unsigned, seeing you set it using >> > bytestream2_get_le32() for P-frames. >> >> No decoder does this. > > zmbv does > > All this is really about the lack of any way to reason about > assumptions like "dimensions can't be larger than X*Y" at compile time, > which is a thing I've been pointing out on this list for a while now. > Nobody tells you not to fix it yourself. > /Tomas > > _______________________________________________ > 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".
On 11/27/2019 3:42 PM, Paul B Mahol wrote: > On 11/27/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >> tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol: >>> On 11/26/19, James Almer <jamrial@gmail.com> wrote: >>>> On 11/26/2019 6:47 AM, Paul B Mahol wrote: >>>>> On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >>>>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >>>>>>> AVFrame >>>>>>> *frame) >>>>>>> +{ >>>>>>> + GetByteContext *gb = &s->gb; >>>>>>> + GetBitContext mask; >>>>>>> + GetByteContext idx9; >>>>>>> + uint16_t nb_vectors, intra_flag; >>>>>>> + const uint8_t *vec; >>>>>>> + const uint8_t *mask_start; >>>>>>> + uint8_t *skip; >>>>>>> + int mask_size; >>>>>>> + int idx9bits = 0; >>>>>>> + int idx9val = 0; >>>>>>> + int num_blocks; >>>>>>> + >>>>>>> + nb_vectors = bytestream2_get_le16(gb); >>>>>>> + intra_flag = bytestream2_get_le16(gb); >>>>>>> + if (intra_flag) { >>>>>>> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >>>>>> >>>>>> Will UB if width*height/4 > INT_MAX >>>>>> >>>>>>> + } else { >>>>>>> + int skip_linesize; >>>>>>> + >>>>>>> + num_blocks = bytestream2_get_le32(gb); >>>>>> >>>>>> Might want to use uint32_t so this doesn't lead to weirdness on >>>>>> 32-bit >>>>>> >>>>>>> + skip_linesize = avctx->width >> 1; >>>>>>> + mask_start = gb->buffer_start + bytestream2_tell(gb); >>>>>>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >>>>>> >>>>>> This can also UB >>>>>> >>>>>> /Tomas >>>>>> >>>>> >>>>> Nothing of this can actually happen. >> >> This assumes max_pixels will never be increased above INT_MAX. "64k" >> video is most definitely within the range of possibility in the coming >> years, if it isn't already. Film archival and DPX come to mind. >> >>>> It can and i'm fairly sure it will happen as soon as the fuzzer starts >>>> testing this decoder using big dimensions. >>> >>> I'm not that guy doing such work. Please stop bikesheding those >>> patches for once. >> >> This reads like an admission of pushing insecure code via "not my >> problem" >> >>>> You don't need asserts here, you just need to check the calculations >>>> will not overflow. Do something like "if ((int64_t)avctx->width * >>>> avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a >>>> day. >>>> Also, maybe num_blocks should be unsigned, seeing you set it using >>>> bytestream2_get_le32() for P-frames. >>> >>> No decoder does this. >> >> zmbv does >> >> All this is really about the lack of any way to reason about >> assumptions like "dimensions can't be larger than X*Y" at compile time, >> which is a thing I've been pointing out on this list for a while now. >> > > Nobody tells you not to fix it yourself. Just add a the suggested overflow checks, Christ. It's a one line addition each. What do you gain arguing like this?
On 11/27/19, James Almer <jamrial@gmail.com> wrote: > On 11/27/2019 3:42 PM, Paul B Mahol wrote: >> On 11/27/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >>> tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol: >>>> On 11/26/19, James Almer <jamrial@gmail.com> wrote: >>>>> On 11/26/2019 6:47 AM, Paul B Mahol wrote: >>>>>> On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >>>>>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>>>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, >>>>>>>> AVFrame >>>>>>>> *frame) >>>>>>>> +{ >>>>>>>> + GetByteContext *gb = &s->gb; >>>>>>>> + GetBitContext mask; >>>>>>>> + GetByteContext idx9; >>>>>>>> + uint16_t nb_vectors, intra_flag; >>>>>>>> + const uint8_t *vec; >>>>>>>> + const uint8_t *mask_start; >>>>>>>> + uint8_t *skip; >>>>>>>> + int mask_size; >>>>>>>> + int idx9bits = 0; >>>>>>>> + int idx9val = 0; >>>>>>>> + int num_blocks; >>>>>>>> + >>>>>>>> + nb_vectors = bytestream2_get_le16(gb); >>>>>>>> + intra_flag = bytestream2_get_le16(gb); >>>>>>>> + if (intra_flag) { >>>>>>>> + num_blocks = (avctx->width / 2) * (avctx->height / 2); >>>>>>> >>>>>>> Will UB if width*height/4 > INT_MAX >>>>>>> >>>>>>>> + } else { >>>>>>>> + int skip_linesize; >>>>>>>> + >>>>>>>> + num_blocks = bytestream2_get_le32(gb); >>>>>>> >>>>>>> Might want to use uint32_t so this doesn't lead to weirdness on >>>>>>> 32-bit >>>>>>> >>>>>>>> + skip_linesize = avctx->width >> 1; >>>>>>>> + mask_start = gb->buffer_start + bytestream2_tell(gb); >>>>>>>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2); >>>>>>> >>>>>>> This can also UB >>>>>>> >>>>>>> /Tomas >>>>>>> >>>>>> >>>>>> Nothing of this can actually happen. >>> >>> This assumes max_pixels will never be increased above INT_MAX. "64k" >>> video is most definitely within the range of possibility in the coming >>> years, if it isn't already. Film archival and DPX come to mind. >>> >>>>> It can and i'm fairly sure it will happen as soon as the fuzzer starts >>>>> testing this decoder using big dimensions. >>>> >>>> I'm not that guy doing such work. Please stop bikesheding those >>>> patches for once. >>> >>> This reads like an admission of pushing insecure code via "not my >>> problem" >>> >>>>> You don't need asserts here, you just need to check the calculations >>>>> will not overflow. Do something like "if ((int64_t)avctx->width * >>>>> avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a >>>>> day. >>>>> Also, maybe num_blocks should be unsigned, seeing you set it using >>>>> bytestream2_get_le32() for P-frames. >>>> >>>> No decoder does this. >>> >>> zmbv does >>> >>> All this is really about the lack of any way to reason about >>> assumptions like "dimensions can't be larger than X*Y" at compile time, >>> which is a thing I've been pointing out on this list for a while now. >>> >> >> Nobody tells you not to fix it yourself. > > Just add a the suggested overflow checks, Christ. It's a one line > addition each. What do you gain arguing like this? Than next day he or you will come with another great idea. > _______________________________________________ > 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".
ons 2019-11-27 klockan 20:00 +0100 skrev Paul B Mahol: > On 11/27/19, James Almer <jamrial@gmail.com> wrote: > > On 11/27/2019 3:42 PM, Paul B Mahol wrote: > > > On 11/27/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > > > > tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol: > > > > > On 11/26/19, James Almer <jamrial@gmail.com> wrote: > > > > > > On 11/26/2019 6:47 AM, Paul B Mahol wrote: > > > > > > > On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > > > > > > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: > > > > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, > > > > > > > > > AVFrame > > > > > > > > > *frame) > > > > > > > > > +{ > > > > > > > > > + GetByteContext *gb = &s->gb; > > > > > > > > > + GetBitContext mask; > > > > > > > > > + GetByteContext idx9; > > > > > > > > > + uint16_t nb_vectors, intra_flag; > > > > > > > > > + const uint8_t *vec; > > > > > > > > > + const uint8_t *mask_start; > > > > > > > > > + uint8_t *skip; > > > > > > > > > + int mask_size; > > > > > > > > > + int idx9bits = 0; > > > > > > > > > + int idx9val = 0; > > > > > > > > > + int num_blocks; > > > > > > > > > + > > > > > > > > > + nb_vectors = bytestream2_get_le16(gb); > > > > > > > > > + intra_flag = bytestream2_get_le16(gb); > > > > > > > > > + if (intra_flag) { > > > > > > > > > + num_blocks = (avctx->width / 2) * (avctx->height / 2); > > > > > > > > > > > > > > > > Will UB if width*height/4 > INT_MAX > > > > > > > > > > > > > > > > > + } else { > > > > > > > > > + int skip_linesize; > > > > > > > > > + > > > > > > > > > + num_blocks = bytestream2_get_le32(gb); > > > > > > > > > > > > > > > > Might want to use uint32_t so this doesn't lead to weirdness on > > > > > > > > 32-bit > > > > > > > > > > > > > > > > > + skip_linesize = avctx->width >> 1; > > > > > > > > > + mask_start = gb->buffer_start + bytestream2_tell(gb); > > > > > > > > > + mask_size = (avctx->width >> 5) * (avctx->height >> 2); > > > > > > > > > > > > > > > > This can also UB > > > > > > > > > > > > > > > > /Tomas > > > > > > > > > > > > > > > > > > > > > > Nothing of this can actually happen. > > > > > > > > This assumes max_pixels will never be increased above INT_MAX. "64k" > > > > video is most definitely within the range of possibility in the coming > > > > years, if it isn't already. Film archival and DPX come to mind. > > > > > > > > > > It can and i'm fairly sure it will happen as soon as the fuzzer starts > > > > > > testing this decoder using big dimensions. > > > > > > > > > > I'm not that guy doing such work. Please stop bikesheding those > > > > > patches for once. > > > > > > > > This reads like an admission of pushing insecure code via "not my > > > > problem" > > > > > > > > > > You don't need asserts here, you just need to check the calculations > > > > > > will not overflow. Do something like "if ((int64_t)avctx->width * > > > > > > avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a > > > > > > day. > > > > > > Also, maybe num_blocks should be unsigned, seeing you set it using > > > > > > bytestream2_get_le32() for P-frames. > > > > > > > > > > No decoder does this. > > > > > > > > zmbv does > > > > > > > > All this is really about the lack of any way to reason about > > > > assumptions like "dimensions can't be larger than X*Y" at compile time, > > > > which is a thing I've been pointing out on this list for a while now. > > > > > > > > > > Nobody tells you not to fix it yourself. > > > > Just add a the suggested overflow checks, Christ. It's a one line > > addition each. What do you gain arguing like this? > > Than next day he or you will come with another great idea. Great ideas like pushing inevitable 0days? /Tomas
On 11/27/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: > ons 2019-11-27 klockan 20:00 +0100 skrev Paul B Mahol: >> On 11/27/19, James Almer <jamrial@gmail.com> wrote: >> > On 11/27/2019 3:42 PM, Paul B Mahol wrote: >> > > On 11/27/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >> > > > tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol: >> > > > > On 11/26/19, James Almer <jamrial@gmail.com> wrote: >> > > > > > On 11/26/2019 6:47 AM, Paul B Mahol wrote: >> > > > > > > On 11/25/19, Tomas Härdin <tjoppen@acc.umu.se> wrote: >> > > > > > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol: >> > > > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> >> > > > > > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext >> > > > > > > > > *avctx, >> > > > > > > > > AVFrame >> > > > > > > > > *frame) >> > > > > > > > > +{ >> > > > > > > > > + GetByteContext *gb = &s->gb; >> > > > > > > > > + GetBitContext mask; >> > > > > > > > > + GetByteContext idx9; >> > > > > > > > > + uint16_t nb_vectors, intra_flag; >> > > > > > > > > + const uint8_t *vec; >> > > > > > > > > + const uint8_t *mask_start; >> > > > > > > > > + uint8_t *skip; >> > > > > > > > > + int mask_size; >> > > > > > > > > + int idx9bits = 0; >> > > > > > > > > + int idx9val = 0; >> > > > > > > > > + int num_blocks; >> > > > > > > > > + >> > > > > > > > > + nb_vectors = bytestream2_get_le16(gb); >> > > > > > > > > + intra_flag = bytestream2_get_le16(gb); >> > > > > > > > > + if (intra_flag) { >> > > > > > > > > + num_blocks = (avctx->width / 2) * (avctx->height >> > > > > > > > > / 2); >> > > > > > > > >> > > > > > > > Will UB if width*height/4 > INT_MAX >> > > > > > > > >> > > > > > > > > + } else { >> > > > > > > > > + int skip_linesize; >> > > > > > > > > + >> > > > > > > > > + num_blocks = bytestream2_get_le32(gb); >> > > > > > > > >> > > > > > > > Might want to use uint32_t so this doesn't lead to weirdness >> > > > > > > > on >> > > > > > > > 32-bit >> > > > > > > > >> > > > > > > > > + skip_linesize = avctx->width >> 1; >> > > > > > > > > + mask_start = gb->buffer_start + >> > > > > > > > > bytestream2_tell(gb); >> > > > > > > > > + mask_size = (avctx->width >> 5) * (avctx->height >> > > > > > > > > >> 2); >> > > > > > > > >> > > > > > > > This can also UB >> > > > > > > > >> > > > > > > > /Tomas >> > > > > > > > >> > > > > > > >> > > > > > > Nothing of this can actually happen. >> > > > >> > > > This assumes max_pixels will never be increased above INT_MAX. "64k" >> > > > video is most definitely within the range of possibility in the >> > > > coming >> > > > years, if it isn't already. Film archival and DPX come to mind. >> > > > >> > > > > > It can and i'm fairly sure it will happen as soon as the fuzzer >> > > > > > starts >> > > > > > testing this decoder using big dimensions. >> > > > > >> > > > > I'm not that guy doing such work. Please stop bikesheding those >> > > > > patches for once. >> > > > >> > > > This reads like an admission of pushing insecure code via "not my >> > > > problem" >> > > > >> > > > > > You don't need asserts here, you just need to check the >> > > > > > calculations >> > > > > > will not overflow. Do something like "if ((int64_t)avctx->width >> > > > > > * >> > > > > > avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and >> > > > > > call it a >> > > > > > day. >> > > > > > Also, maybe num_blocks should be unsigned, seeing you set it >> > > > > > using >> > > > > > bytestream2_get_le32() for P-frames. >> > > > > >> > > > > No decoder does this. >> > > > >> > > > zmbv does >> > > > >> > > > All this is really about the lack of any way to reason about >> > > > assumptions like "dimensions can't be larger than X*Y" at compile >> > > > time, >> > > > which is a thing I've been pointing out on this list for a while >> > > > now. >> > > > >> > > >> > > Nobody tells you not to fix it yourself. >> > >> > Just add a the suggested overflow checks, Christ. It's a one line >> > addition each. What do you gain arguing like this? >> >> Than next day he or you will come with another great idea. > > Great ideas like pushing inevitable 0days? Very friendly reviews and developers.
diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 006a472a6d..52e5b4f345 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -493,6 +493,7 @@ OBJS-$(CONFIG_MSZH_DECODER) += lcldec.o OBJS-$(CONFIG_MTS2_DECODER) += mss4.o OBJS-$(CONFIG_MVC1_DECODER) += mvcdec.o OBJS-$(CONFIG_MVC2_DECODER) += mvcdec.o +OBJS-$(CONFIG_MVDV_DECODER) += midivid.o OBJS-$(CONFIG_MWSC_DECODER) += mwsc.o OBJS-$(CONFIG_MXPEG_DECODER) += mxpegdec.o OBJS-$(CONFIG_NELLYMOSER_DECODER) += nellymoserdec.o nellymoser.o diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c index 0c0741936c..4eb1afbea1 100644 --- a/libavcodec/allcodecs.c +++ b/libavcodec/allcodecs.c @@ -218,6 +218,7 @@ extern AVCodec ff_mszh_decoder; extern AVCodec ff_mts2_decoder; extern AVCodec ff_mvc1_decoder; extern AVCodec ff_mvc2_decoder; +extern AVCodec ff_mvdv_decoder; extern AVCodec ff_mwsc_decoder; extern AVCodec ff_mxpeg_decoder; extern AVCodec ff_nuv_decoder; diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 813a43b72e..1cbc9c9ef1 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -458,6 +458,7 @@ enum AVCodecID { AV_CODEC_ID_LSCR, AV_CODEC_ID_VP4, AV_CODEC_ID_IMM5, + AV_CODEC_ID_MVDV, /* various PCM "codecs" */ AV_CODEC_ID_FIRST_AUDIO = 0x10000, ///< A dummy id pointing at the start of audio codecs diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c index 5961af3c85..3e634bbec7 100644 --- a/libavcodec/codec_desc.c +++ b/libavcodec/codec_desc.c @@ -1733,6 +1733,13 @@ static const AVCodecDescriptor codec_descriptors[] = { .long_name = NULL_IF_CONFIG_SMALL("Infinity IMM5"), .props = AV_CODEC_PROP_LOSSY, }, + { + .id = AV_CODEC_ID_MVDV, + .type = AVMEDIA_TYPE_VIDEO, + .name = "mvdv", + .long_name = NULL_IF_CONFIG_SMALL("MidiVid VQ"), + .props = AV_CODEC_PROP_LOSSY, + }, /* various PCM "codecs" */ { diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c new file mode 100644 index 0000000000..36ed4b83bd --- /dev/null +++ b/libavcodec/midivid.c @@ -0,0 +1,272 @@ +/* + * MidiVid decoder + * Copyright (c) 2019 Paul B Mahol + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include "libavutil/imgutils.h" +#include "libavutil/internal.h" +#include "libavutil/intreadwrite.h" +#include "libavutil/mem.h" + +#define BITSTREAM_READER_LE +#include "avcodec.h" +#include "get_bits.h" +#include "bytestream.h" +#include "internal.h" + +typedef struct MidiVidContext { + GetByteContext gb; + + uint8_t *uncompressed; + int uncompressed_size; + uint8_t *skip; + + AVFrame *frame; +} MidiVidContext; + +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame) +{ + GetByteContext *gb = &s->gb; + GetBitContext mask; + GetByteContext idx9; + uint16_t nb_vectors, intra_flag; + const uint8_t *vec; + const uint8_t *mask_start; + uint8_t *skip; + int mask_size; + int idx9bits = 0; + int idx9val = 0; + int num_blocks; + + nb_vectors = bytestream2_get_le16(gb); + intra_flag = bytestream2_get_le16(gb); + if (intra_flag) { + num_blocks = (avctx->width / 2) * (avctx->height / 2); + } else { + int skip_linesize; + + num_blocks = bytestream2_get_le32(gb); + skip_linesize = avctx->width >> 1; + mask_start = gb->buffer_start + bytestream2_tell(gb); + mask_size = (avctx->width >> 5) * (avctx->height >> 2); + init_get_bits8(&mask, mask_start, mask_size); + bytestream2_skip(gb, mask_size); + skip = s->skip; + + for (int y = 0; y < avctx->height >> 2; y++) { + for (int x = 0; x < avctx->width >> 2; x++) { + int flag = !get_bits1(&mask); + + skip[(y*2) *skip_linesize + x*2 ] = flag; + skip[(y*2) *skip_linesize + x*2+1] = flag; + skip[(y*2+1)*skip_linesize + x*2 ] = flag; + skip[(y*2+1)*skip_linesize + x*2+1] = flag; + } + } + } + + vec = gb->buffer_start + bytestream2_tell(gb); + if (bytestream2_get_bytes_left(gb) < nb_vectors * 12) + return AVERROR_INVALIDDATA; + bytestream2_skip(gb, nb_vectors * 12); + if (nb_vectors > 256) { + if (bytestream2_get_bytes_left(gb) < (num_blocks + 7) / 8) + return AVERROR_INVALIDDATA; + bytestream2_init(&idx9, gb->buffer_start + bytestream2_tell(gb), (num_blocks + 7) / 8); + bytestream2_skip(gb, (num_blocks + 7) / 8); + } + + skip = s->skip; + + for (int y = avctx->height - 2; y >= 0; y -= 2) { + uint8_t *dsty = frame->data[0] + y * frame->linesize[0]; + uint8_t *dstu = frame->data[1] + y * frame->linesize[1]; + uint8_t *dstv = frame->data[2] + y * frame->linesize[2]; + + for (int x = 0; x < avctx->width; x += 2) { + int idx; + + if (!intra_flag && *skip++) + continue; + if (bytestream2_get_bytes_left(gb) <= 0) + return AVERROR_INVALIDDATA; + if (nb_vectors <= 256) { + idx = bytestream2_get_byte(gb); + } else { + if (idx9bits == 0) { + idx9val = bytestream2_get_byte(&idx9); + idx9bits = 8; + } + idx9bits--; + idx = bytestream2_get_byte(gb) | (((idx9val >> (7 - idx9bits)) & 1) << 8); + } + + dsty[x +frame->linesize[0]] = vec[idx * 12 + 0]; + dsty[x+1+frame->linesize[0]] = vec[idx * 12 + 3]; + dsty[x] = vec[idx * 12 + 6]; + dsty[x+1] = vec[idx * 12 + 9]; + + dstu[x +frame->linesize[1]] = vec[idx * 12 + 1]; + dstu[x+1+frame->linesize[1]] = vec[idx * 12 + 4]; + dstu[x] = vec[idx * 12 + 7]; + dstu[x+1] = vec[idx * 12 +10]; + + dstv[x +frame->linesize[2]] = vec[idx * 12 + 2]; + dstv[x+1+frame->linesize[2]] = vec[idx * 12 + 5]; + dstv[x] = vec[idx * 12 + 8]; + dstv[x+1] = vec[idx * 12 +11]; + } + } + + return intra_flag; +} + +static ptrdiff_t lzss_uncompress(MidiVidContext *s, GetByteContext *gb, uint8_t *dst, int size) +{ + uint8_t *dst_start = dst; + uint8_t *dst_end = dst + size; + + for (;bytestream2_get_bytes_left(gb) >= 3;) { + int op = bytestream2_get_le16(gb); + + for (int i = 0; i < 16; i++) { + if (op & 1) { + int s0 = bytestream2_get_byte(gb); + int s1 = bytestream2_get_byte(gb); + int offset = ((s0 & 0xF0) << 4) | s1; + int length = (s0 & 0xF) + 3; + + if (dst + length > dst_end || + dst - offset < dst_start) + return AVERROR_INVALIDDATA; + if (offset > 0) { + for (int j = 0; j < length; j++) { + dst[j] = dst[j - offset]; + } + } + dst += length; + } else { + if (dst >= dst_end) + return AVERROR_INVALIDDATA; + *dst++ = bytestream2_get_byte(gb); + } + op >>= 1; + } + } + + return dst - dst_start; +} + +static int decode_frame(AVCodecContext *avctx, void *data, + int *got_frame, AVPacket *avpkt) +{ + MidiVidContext *s = avctx->priv_data; + GetByteContext *gb = &s->gb; + AVFrame *frame = s->frame; + int ret, key, uncompressed; + + if (avpkt->size <= 13) + return AVERROR_INVALIDDATA; + + bytestream2_init(gb, avpkt->data, avpkt->size); + bytestream2_skip(gb, 8); + uncompressed = bytestream2_get_le32(gb); + + if ((ret = ff_reget_buffer(avctx, s->frame, 0)) < 0) + return ret; + + if (uncompressed) { + ret = decode_mvdv(s, avctx, frame); + } else { + av_fast_padded_malloc(&s->uncompressed, &s->uncompressed_size, 16LL * (avpkt->size - 12)); + if (!s->uncompressed) + return AVERROR(ENOMEM); + + ret = lzss_uncompress(s, gb, s->uncompressed, s->uncompressed_size); + if (ret < 0) + return ret; + bytestream2_init(gb, s->uncompressed, ret); + ret = decode_mvdv(s, avctx, frame); + } + + if (ret < 0) + return ret; + key = ret; + + if ((ret = av_frame_ref(data, s->frame)) < 0) + return ret; + + frame->pict_type = key ? AV_PICTURE_TYPE_I : AV_PICTURE_TYPE_P; + frame->key_frame = key; + *got_frame = 1; + + return avpkt->size; +} + +static av_cold int decode_init(AVCodecContext *avctx) +{ + MidiVidContext *s = avctx->priv_data; + + avctx->pix_fmt = AV_PIX_FMT_YUV444P; + + s->frame = av_frame_alloc(); + if (!s->frame) + return AVERROR(ENOMEM); + s->skip = av_calloc(avctx->width >> 1, avctx->height >> 1); + if (!s->skip) + return AVERROR(ENOMEM); + + return 0; +} + +static void decode_flush(AVCodecContext *avctx) +{ + MidiVidContext *s = avctx->priv_data; + + av_frame_unref(s->frame); +} + +static av_cold int decode_close(AVCodecContext *avctx) +{ + MidiVidContext *s = avctx->priv_data; + + av_frame_free(&s->frame); + av_freep(&s->uncompressed); + av_freep(&s->skip); + + return 0; +} + +AVCodec ff_mvdv_decoder = { + .name = "mvdv", + .long_name = NULL_IF_CONFIG_SMALL("MidiVid VQ"), + .type = AVMEDIA_TYPE_VIDEO, + .id = AV_CODEC_ID_MVDV, + .priv_data_size = sizeof(MidiVidContext), + .init = decode_init, + .decode = decode_frame, + .flush = decode_flush, + .close = decode_close, + .capabilities = AV_CODEC_CAP_DR1, + .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, +}; diff --git a/libavformat/riff.c b/libavformat/riff.c index 048a79e394..25ccedc8ce 100644 --- a/libavformat/riff.c +++ b/libavformat/riff.c @@ -489,6 +489,7 @@ const AVCodecTag ff_codec_bmp_tags[] = { { AV_CODEC_ID_AGM, MKTAG('A', 'G', 'M', '7') }, { AV_CODEC_ID_LSCR, MKTAG('L', 'S', 'C', 'R') }, { AV_CODEC_ID_IMM5, MKTAG('I', 'M', 'M', '5') }, + { AV_CODEC_ID_MVDV, MKTAG('M', 'V', 'D', 'V') }, { AV_CODEC_ID_NONE, 0 } };
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/Makefile | 1 + libavcodec/allcodecs.c | 1 + libavcodec/avcodec.h | 1 + libavcodec/codec_desc.c | 7 ++ libavcodec/midivid.c | 272 ++++++++++++++++++++++++++++++++++++++++ libavformat/riff.c | 1 + 6 files changed, 283 insertions(+) create mode 100644 libavcodec/midivid.c