Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/27/19, Tomas Härdin wrote:
> ons 2019-11-27 klockan 20:00 +0100 skrev Paul B Mahol:
>> On 11/27/19, James Almer wrote:
>> > On 11/27/2019 3:42 PM, Paul B Mahol wrote:
>> > > On 11/27/19, Tomas Härdin wrote:
>> > > > tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
>> > > > > On 11/26/19, James Almer wrote:
>> > > > > > On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>> > > > > > > On 11/25/19, Tomas Härdin wrote:
>> > > > > > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>> > > > > > > > > Signed-off-by: Paul B Mahol
>> > > > > > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext
>> > > > > > > > > *avctx,
>> > > > > > > > > AVFrame
>> > > > > > > > > *frame)
>> > > > > > > > > +{
>> > > > > > > > > +GetByteContext *gb = >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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
ons 2019-11-27 klockan 20:00 +0100 skrev Paul B Mahol:
> On 11/27/19, James Almer wrote:
> > On 11/27/2019 3:42 PM, Paul B Mahol wrote:
> > > On 11/27/19, Tomas Härdin wrote:
> > > > tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
> > > > > On 11/26/19, James Almer wrote:
> > > > > > On 11/26/2019 6:47 AM, Paul B Mahol wrote:
> > > > > > > On 11/25/19, Tomas Härdin wrote:
> > > > > > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
> > > > > > > > > Signed-off-by: Paul B Mahol
> > > > > > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext
> > > > > > > > > *avctx,
> > > > > > > > > AVFrame
> > > > > > > > > *frame)
> > > > > > > > > +{
> > > > > > > > > +GetByteContext *gb = >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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/27/19, James Almer wrote:
> On 11/27/2019 3:42 PM, Paul B Mahol wrote:
>> On 11/27/19, Tomas Härdin wrote:
>>> tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
On 11/26/19, James Almer wrote:
> On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>> On 11/25/19, Tomas Härdin wrote:
>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
Signed-off-by: Paul B Mahol
+static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
AVFrame
*frame)
+{
+GetByteContext *gb = >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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/27/2019 3:42 PM, Paul B Mahol wrote:
> On 11/27/19, Tomas Härdin wrote:
>> tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
>>> On 11/26/19, James Almer wrote:
On 11/26/2019 6:47 AM, Paul B Mahol wrote:
> On 11/25/19, Tomas Härdin wrote:
>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>>> Signed-off-by: Paul B Mahol
>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
>>> AVFrame
>>> *frame)
>>> +{
>>> +GetByteContext *gb = >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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/27/19, Tomas Härdin wrote:
> tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
>> On 11/26/19, James Almer wrote:
>> > On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>> > > On 11/25/19, Tomas Härdin wrote:
>> > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>> > > > > Signed-off-by: Paul B Mahol
>> > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
>> > > > > AVFrame
>> > > > > *frame)
>> > > > > +{
>> > > > > +GetByteContext *gb = >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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
> On 11/26/19, James Almer wrote:
> > On 11/26/2019 6:47 AM, Paul B Mahol wrote:
> > > On 11/25/19, Tomas Härdin wrote:
> > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
> > > > > Signed-off-by: Paul B Mahol
> > > > > +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
> > > > > AVFrame
> > > > > *frame)
> > > > > +{
> > > > > +GetByteContext *gb = >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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/19, James Almer wrote:
> On 11/25/2019 6:09 PM, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol
>> ---
>> 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 00..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
>> +#include
>> +#include
>> +
>> +#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 = >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_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();
>> +
>> +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(, 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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/19, James Almer wrote:
> On 11/26/2019 4:29 PM, Paul B Mahol wrote:
>> On 11/26/19, James Almer wrote:
>>> On 11/26/2019 6:47 AM, Paul B Mahol wrote:
On 11/25/19, Tomas Härdin wrote:
> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>> Signed-off-by: Paul B Mahol
>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
>> AVFrame
>> *frame)
>> +{
>> +GetByteContext *gb = >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-requ...@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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/2019 4:29 PM, Paul B Mahol wrote:
> On 11/26/19, James Almer wrote:
>> On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>>> On 11/25/19, Tomas Härdin wrote:
mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
> Signed-off-by: Paul B Mahol
> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
> AVFrame
> *frame)
> +{
> +GetByteContext *gb = >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-requ...@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-requ...@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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/19, James Almer wrote:
> On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>> On 11/25/19, Tomas Härdin wrote:
>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
Signed-off-by: Paul B Mahol
+static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
AVFrame
*frame)
+{
+GetByteContext *gb = >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-requ...@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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/25/2019 6:09 PM, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol
> ---
> 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 00..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
> +#include
> +#include
> +
> +#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 = >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_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();
> +
> +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(, 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/2019 6:47 AM, Paul B Mahol wrote:
> On 11/25/19, Tomas Härdin wrote:
>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>>> Signed-off-by: Paul B Mahol
>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame
>>> *frame)
>>> +{
>>> +GetByteContext *gb = >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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/19, Nicolas George 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/26/19, Carl Eugen Hoyos wrote:
> Am Di., 26. Nov. 2019 um 10:53 Uhr schrieb Paul B Mahol :
>>
>> On 11/25/19, Tomas Härdin wrote:
>> > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>> >> Signed-off-by: Paul B Mahol
>> >> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
>> >> AVFrame
>> >> *frame)
>> >> +{
>> >> +GetByteContext *gb = >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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
Am Di., 26. Nov. 2019 um 10:53 Uhr schrieb Paul B Mahol :
>
> On 11/25/19, Tomas Härdin wrote:
> > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
> >> Signed-off-by: Paul B Mahol
> >> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame
> >> *frame)
> >> +{
> >> +GetByteContext *gb = >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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
On 11/25/19, Tomas Härdin wrote:
> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>> Signed-off-by: Paul B Mahol
>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame
>> *frame)
>> +{
>> +GetByteContext *gb = >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-requ...@ffmpeg.org with subject "unsubscribe".
Nothing of this can actually happen.
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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
> Signed-off-by: Paul B Mahol
> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame
> *frame)
> +{
> +GetByteContext *gb = >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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder
Signed-off-by: Paul B Mahol
---
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/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 = 0x1, ///< 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 00..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
+#include
+#include
+
+#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 = >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_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();
+
+
