| Message ID | 20230612132256.84549-1-leo.izen@gmail.com |
|---|---|
| State | Accepted |
| Commit | 61047f0f82c76620cc42888958dd9834bcaa18fb |
| Headers | show |
| Series | [FFmpeg-devel,v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 6/12/23 09:22, Leo Izen wrote: > This switches the jpegxl_collect_codestream_header function to use > avcodec/bytestream2, which better enforces barriers, and should avoid > overrunning buffers with jxlp boxes if the size is zero or if the size > is so small the box is invalid. > > Signed-off-by: Leo Izen <leo.izen@gmail.com> > --- > libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 26 deletions(-) Will push soon if there's no objections, as this fixes a bug. - Leo Izen
Leo Izen: > This switches the jpegxl_collect_codestream_header function to use > avcodec/bytestream2, which better enforces barriers, and should avoid > overrunning buffers with jxlp boxes if the size is zero or if the size > is so small the box is invalid. > > Signed-off-by: Leo Izen <leo.izen@gmail.com> > --- > libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 26 deletions(-) > > diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c > index 6ea6c46d8f..ec400c955c 100644 > --- a/libavformat/jpegxl_anim_dec.c > +++ b/libavformat/jpegxl_anim_dec.c > @@ -28,6 +28,7 @@ > #include <stdint.h> > #include <string.h> > > +#include "libavcodec/bytestream.h" > #define BITSTREAM_READER_LE > #include "libavcodec/get_bits.h" > > @@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext { > * returns the number of bytes consumed from input, may be greater than input_len > * if the input doesn't end on an ISOBMFF-box boundary > */ > -static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) { > - const uint8_t *b = input_buffer; > +static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, > + uint8_t *buffer, int buflen, int *copied) { > + GetByteContext gb; > *copied = 0; > + bytestream2_init(&gb, input_buffer, input_len); > > while (1) { > uint64_t size; > uint32_t tag; > int head_size = 8; > > - if (b - input_buffer >= input_len - 16) > + if (bytestream2_get_bytes_left(&gb) < 16) > break; > > - size = AV_RB32(b); > - b += 4; > + size = bytestream2_get_be32(&gb); > if (size == 1) { > - size = AV_RB64(b); > - b += 8; > + size = bytestream2_get_be64(&gb); > head_size = 16; > } > /* invalid ISOBMFF size */ > - if (size > 0 && size <= head_size) > + if (size && size <= head_size) > return AVERROR_INVALIDDATA; > - if (size > 0) > + if (size) > size -= head_size; > > - tag = AV_RL32(b); > - b += 4; > + tag = bytestream2_get_le32(&gb); > if (tag == MKTAG('j', 'x', 'l', 'p')) { > - b += 4; > - size -= 4; > + if (bytestream2_get_bytes_left(&gb) < 4) > + break; > + bytestream2_skip(&gb, 4); > + if (size) { > + if (size <= 4) > + return AVERROR_INVALIDDATA; > + size -= 4; > + } > } > + /* > + * size = 0 means "until EOF". this is legal but uncommon > + * here we just set it to the remaining size of the probe buffer > + */ > + if (!size) > + size = bytestream2_get_bytes_left(&gb); > > if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) { > - /* > - * size = 0 means "until EOF". this is legal but uncommon > - * here we just set it to the remaining size of the probe buffer > - * which at this point should always be nonnegative > - */ > - if (size == 0 || size > input_len - (b - input_buffer)) > - size = input_len - (b - input_buffer); > - > if (size > buflen - *copied) > size = buflen - *copied; > /* > * arbitrary chunking of the payload makes this memcpy hard to avoid > * in practice this will only be performed one or two times at most > */ > - memcpy(buffer + *copied, b, size); > - *copied += size; > + *copied += bytestream2_get_buffer(&gb, buffer + *copied, size); > + } else { > + bytestream2_skip(&gb, size); > } > - b += size; > - if (b >= input_buffer + input_len || *copied >= buflen) > + if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen) > break; > } > > - return b - input_buffer; > + return bytestream2_tell(&gb); > } > > static int jpegxl_anim_probe(const AVProbeData *p) Is there an actual (potential) overrun or is this just a precaution? - Andreas
On 6/16/23 15:00, Andreas Rheinhardt wrote: > Leo Izen: >> This switches the jpegxl_collect_codestream_header function to use >> avcodec/bytestream2, which better enforces barriers, and should avoid >> overrunning buffers with jxlp boxes if the size is zero or if the size >> is so small the box is invalid. >> >> Signed-off-by: Leo Izen <leo.izen@gmail.com> >> --- >> libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++---------------- >> 1 file changed, 30 insertions(+), 26 deletions(-) >> >> diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c >> index 6ea6c46d8f..ec400c955c 100644 >> --- a/libavformat/jpegxl_anim_dec.c >> +++ b/libavformat/jpegxl_anim_dec.c >> @@ -28,6 +28,7 @@ >> #include <stdint.h> >> #include <string.h> >> >> +#include "libavcodec/bytestream.h" >> #define BITSTREAM_READER_LE >> #include "libavcodec/get_bits.h" >> >> @@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext { >> * returns the number of bytes consumed from input, may be greater than input_len >> * if the input doesn't end on an ISOBMFF-box boundary >> */ >> -static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) { >> - const uint8_t *b = input_buffer; >> +static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, >> + uint8_t *buffer, int buflen, int *copied) { >> + GetByteContext gb; >> *copied = 0; >> + bytestream2_init(&gb, input_buffer, input_len); >> >> while (1) { >> uint64_t size; >> uint32_t tag; >> int head_size = 8; >> >> - if (b - input_buffer >= input_len - 16) >> + if (bytestream2_get_bytes_left(&gb) < 16) >> break; >> >> - size = AV_RB32(b); >> - b += 4; >> + size = bytestream2_get_be32(&gb); >> if (size == 1) { >> - size = AV_RB64(b); >> - b += 8; >> + size = bytestream2_get_be64(&gb); >> head_size = 16; >> } >> /* invalid ISOBMFF size */ >> - if (size > 0 && size <= head_size) >> + if (size && size <= head_size) >> return AVERROR_INVALIDDATA; >> - if (size > 0) >> + if (size) >> size -= head_size; >> >> - tag = AV_RL32(b); >> - b += 4; >> + tag = bytestream2_get_le32(&gb); >> if (tag == MKTAG('j', 'x', 'l', 'p')) { >> - b += 4; >> - size -= 4; >> + if (bytestream2_get_bytes_left(&gb) < 4) >> + break; >> + bytestream2_skip(&gb, 4); >> + if (size) { >> + if (size <= 4) >> + return AVERROR_INVALIDDATA; >> + size -= 4; >> + } >> } >> + /* >> + * size = 0 means "until EOF". this is legal but uncommon >> + * here we just set it to the remaining size of the probe buffer >> + */ >> + if (!size) >> + size = bytestream2_get_bytes_left(&gb); >> >> if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) { >> - /* >> - * size = 0 means "until EOF". this is legal but uncommon >> - * here we just set it to the remaining size of the probe buffer >> - * which at this point should always be nonnegative >> - */ >> - if (size == 0 || size > input_len - (b - input_buffer)) >> - size = input_len - (b - input_buffer); >> - >> if (size > buflen - *copied) >> size = buflen - *copied; >> /* >> * arbitrary chunking of the payload makes this memcpy hard to avoid >> * in practice this will only be performed one or two times at most >> */ >> - memcpy(buffer + *copied, b, size); >> - *copied += size; >> + *copied += bytestream2_get_buffer(&gb, buffer + *copied, size); >> + } else { >> + bytestream2_skip(&gb, size); >> } >> - b += size; >> - if (b >= input_buffer + input_len || *copied >= buflen) >> + if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen) >> break; >> } >> >> - return b - input_buffer; >> + return bytestream2_tell(&gb); >> } >> >> static int jpegxl_anim_probe(const AVProbeData *p) > > Is there an actual (potential) overrun or is this just a precaution? > > - Andreas > Yea, michaelni sent a cvslog email about it earlier, mail ID <20230608002033.GF870501@pb2>. This fixes that and switches to bytestream2 at Anton's recommendation. - Leo Izen
On 6/16/23 14:35, Leo Izen wrote: > On 6/12/23 09:22, Leo Izen wrote: >> This switches the jpegxl_collect_codestream_header function to use >> avcodec/bytestream2, which better enforces barriers, and should avoid >> overrunning buffers with jxlp boxes if the size is zero or if the size >> is so small the box is invalid. >> >> Signed-off-by: Leo Izen <leo.izen@gmail.com> >> --- >> libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++---------------- >> 1 file changed, 30 insertions(+), 26 deletions(-) > > Will push soon if there's no objections, as this fixes a bug. > > - Leo Izen > Pushed as 61047f0f82c76620cc42888958dd9834bcaa18fb. - Leo Izen (Traneptora)
diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c index 6ea6c46d8f..ec400c955c 100644 --- a/libavformat/jpegxl_anim_dec.c +++ b/libavformat/jpegxl_anim_dec.c @@ -28,6 +28,7 @@ #include <stdint.h> #include <string.h> +#include "libavcodec/bytestream.h" #define BITSTREAM_READER_LE #include "libavcodec/get_bits.h" @@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext { * returns the number of bytes consumed from input, may be greater than input_len * if the input doesn't end on an ISOBMFF-box boundary */ -static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) { - const uint8_t *b = input_buffer; +static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, + uint8_t *buffer, int buflen, int *copied) { + GetByteContext gb; *copied = 0; + bytestream2_init(&gb, input_buffer, input_len); while (1) { uint64_t size; uint32_t tag; int head_size = 8; - if (b - input_buffer >= input_len - 16) + if (bytestream2_get_bytes_left(&gb) < 16) break; - size = AV_RB32(b); - b += 4; + size = bytestream2_get_be32(&gb); if (size == 1) { - size = AV_RB64(b); - b += 8; + size = bytestream2_get_be64(&gb); head_size = 16; } /* invalid ISOBMFF size */ - if (size > 0 && size <= head_size) + if (size && size <= head_size) return AVERROR_INVALIDDATA; - if (size > 0) + if (size) size -= head_size; - tag = AV_RL32(b); - b += 4; + tag = bytestream2_get_le32(&gb); if (tag == MKTAG('j', 'x', 'l', 'p')) { - b += 4; - size -= 4; + if (bytestream2_get_bytes_left(&gb) < 4) + break; + bytestream2_skip(&gb, 4); + if (size) { + if (size <= 4) + return AVERROR_INVALIDDATA; + size -= 4; + } } + /* + * size = 0 means "until EOF". this is legal but uncommon + * here we just set it to the remaining size of the probe buffer + */ + if (!size) + size = bytestream2_get_bytes_left(&gb); if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) { - /* - * size = 0 means "until EOF". this is legal but uncommon - * here we just set it to the remaining size of the probe buffer - * which at this point should always be nonnegative - */ - if (size == 0 || size > input_len - (b - input_buffer)) - size = input_len - (b - input_buffer); - if (size > buflen - *copied) size = buflen - *copied; /* * arbitrary chunking of the payload makes this memcpy hard to avoid * in practice this will only be performed one or two times at most */ - memcpy(buffer + *copied, b, size); - *copied += size; + *copied += bytestream2_get_buffer(&gb, buffer + *copied, size); + } else { + bytestream2_skip(&gb, size); } - b += size; - if (b >= input_buffer + input_len || *copied >= buflen) + if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen) break; } - return b - input_buffer; + return bytestream2_tell(&gb); } static int jpegxl_anim_probe(const AVProbeData *p)
This switches the jpegxl_collect_codestream_header function to use avcodec/bytestream2, which better enforces barriers, and should avoid overrunning buffers with jxlp boxes if the size is zero or if the size is so small the box is invalid. Signed-off-by: Leo Izen <leo.izen@gmail.com> --- libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 26 deletions(-)