[FFmpeg-devel,5/6] h264_mp4toannexb: Improve overread checks

Submitted by Andreas Rheinhardt on July 24, 2019, 5:15 p.m.

Details

Message ID 20190724171557.10037-4-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt July 24, 2019, 5:15 p.m.
1. Left shifts of negative values are undefined as soon as the sign bit
is involved. Therefore make nal_size an uint32_t and drop the check for
whether it is < 0.
2. The two checks for overreads (whether the length field is contained
in the packet and whether the actual unit is contained in the packet)
can be combined into one because the packet is padded, i.e. a potential
overread caused by reading the length field without checking whether
said length field is actually part of the packet's buffer is allowed
as one always stays within the padding. But one has to be aware of
a pitfall: The comparison must be performed in (at least) int64_t as
otherwise buf_end - buf might be promoted to uint32_t in which case
an already occured overread would appear as a very large number.
A comment explaining this has been added, too.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/h264_mp4toannexb_bsf.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
index 0f46ad907c..5d9b71000e 100644
--- a/libavcodec/h264_mp4toannexb_bsf.c
+++ b/libavcodec/h264_mp4toannexb_bsf.c
@@ -172,8 +172,7 @@  static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
 
     AVPacket *in;
     uint8_t unit_type;
-    int32_t nal_size;
-    uint32_t cumul_size    = 0;
+    uint32_t cumul_size = 0, nal_size;
     const uint8_t *buf;
     const uint8_t *buf_end;
     int            buf_size;
@@ -195,18 +194,19 @@  static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
     buf_end  = in->data + in->size;
 
     do {
-        ret= AVERROR(EINVAL);
-        if (buf + s->length_size > buf_end)
-            goto fail;
-
+        /* possible overread ok due to padding */
         for (nal_size = 0, i = 0; i<s->length_size; i++)
             nal_size = (nal_size << 8) | buf[i];
 
         buf += s->length_size;
         unit_type = *buf & 0x1f;
 
-        if (nal_size > buf_end - buf || nal_size < 0)
+        /* This check requires the cast as the right side might
+         * otherwise be promoted to an unsigned value. */
+        if ((int64_t)nal_size > buf_end - buf) {
+            ret = AVERROR(EINVAL);
             goto fail;
+        }
 
         if (unit_type == H264_NAL_SPS)
             s->idr_sps_seen = s->new_idr = 1;