Message ID | 20190929164605.20835-1-sw@jkqxz.net |
---|---|
State | New |
Headers | show |
On 9/29/2019 1:45 PM, Mark Thompson wrote: > Fixes CID 1419833. > --- > libavcodec/cbs_h2645.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index 2dc261f7a5..185c458f61 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, > nb_arrays = bytestream2_get_byte(&gbc); > for (i = 0; i < nb_arrays; i++) { > nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f; > + > nb_nals = bytestream2_get_be16(&gbc); > + if (nb_nals > 64) { Why not check for the actual limit of each ps type instead? This code will still try to parse the file if it reports more than 16 sps, for example, despite it being invalid. Maybe also check for nb_nals == 0. > + // Too many NALs of this type - the header must be invalid. > + return AVERROR_INVALIDDATA; > + } > > start = bytestream2_tell(&gbc); > for (j = 0; j < nb_nals; j++) { >
On 29/09/2019 20:54, James Almer wrote: > On 9/29/2019 1:45 PM, Mark Thompson wrote: >> Fixes CID 1419833. >> --- >> libavcodec/cbs_h2645.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >> index 2dc261f7a5..185c458f61 100644 >> --- a/libavcodec/cbs_h2645.c >> +++ b/libavcodec/cbs_h2645.c >> @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, >> nb_arrays = bytestream2_get_byte(&gbc); >> for (i = 0; i < nb_arrays; i++) { >> nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f; >> + >> nb_nals = bytestream2_get_be16(&gbc); >> + if (nb_nals > 64) { > > Why not check for the actual limit of each ps type instead? Mainly because that would require a big switch statement to determine the limit - since it's an early check for invalid streams which will fail later anyway I didn't want to clutter the code too much. > This code > will still try to parse the file if it reports more than 16 sps, for > example, despite it being invalid. > > Maybe also check for nb_nals == 0. Is that actually invalid rather than just pointless? I don't have a specification for this, and hevc_parse.c also accepts it (not checking the count at all). >> + // Too many NALs of this type - the header must be invalid. >> + return AVERROR_INVALIDDATA; >> + } >> >> start = bytestream2_tell(&gbc); >> for (j = 0; j < nb_nals; j++) { >> Thanks, - Mark
On 9/29/2019 5:01 PM, Mark Thompson wrote: > On 29/09/2019 20:54, James Almer wrote: >> On 9/29/2019 1:45 PM, Mark Thompson wrote: >>> Fixes CID 1419833. >>> --- >>> libavcodec/cbs_h2645.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >>> index 2dc261f7a5..185c458f61 100644 >>> --- a/libavcodec/cbs_h2645.c >>> +++ b/libavcodec/cbs_h2645.c >>> @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, >>> nb_arrays = bytestream2_get_byte(&gbc); >>> for (i = 0; i < nb_arrays; i++) { >>> nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f; >>> + >>> nb_nals = bytestream2_get_be16(&gbc); >>> + if (nb_nals > 64) { >> >> Why not check for the actual limit of each ps type instead? > > Mainly because that would require a big switch statement to determine the limit - since it's an early check for invalid streams which will fail later anyway I didn't want to clutter the code too much. > >> This code >> will still try to parse the file if it reports more than 16 sps, for >> example, despite it being invalid. >> >> Maybe also check for nb_nals == 0. > > Is that actually invalid rather than just pointless? I don't have a specification for this, and hevc_parse.c also accepts it (not checking the count at all). Yeah, it would obviously be invalid data. The for loop below wouldn't trigger in that case and both start and end will have the same value, meaning ff_h2645_packet_split() will be called with a length of 0, which apparently will not be considered an error and instead will just create and empty H2645Packet. If that's going to be an issue further in the fragment splitting process, i don't know. > >>> + // Too many NALs of this type - the header must be invalid. >>> + return AVERROR_INVALIDDATA; >>> + } >>> >>> start = bytestream2_tell(&gbc); >>> for (j = 0; j < nb_nals; j++) { >>> > > Thanks, > > - Mark > _______________________________________________ > 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". >
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 2dc261f7a5..185c458f61 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, nb_arrays = bytestream2_get_byte(&gbc); for (i = 0; i < nb_arrays; i++) { nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f; + nb_nals = bytestream2_get_be16(&gbc); + if (nb_nals > 64) { + // Too many NALs of this type - the header must be invalid. + return AVERROR_INVALIDDATA; + } start = bytestream2_tell(&gbc); for (j = 0; j < nb_nals; j++) {