diff mbox

[FFmpeg-devel,2/2] avformat/rtmppkt: Convert ff_amf_get_field_value() to bytestream2

Message ID 20170728134705.20300-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer July 28, 2017, 1:47 p.m. UTC 
Fixes: out of array accesses

The new function uses ff_ prefix even though its static to ease future
changes toward bytestream2

Found-by: JunDong Xie of Ant-financial Light-Year Security Lab
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/rtmppkt.c | 57 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 20 deletions(-)

Comments

Michael Niedermayer July 29, 2017, 1:57 a.m. UTC | #1 
On Fri, Jul 28, 2017 at 03:47:05PM +0200, Michael Niedermayer wrote:
> Fixes: out of array accesses
> 

> The new function uses ff_ prefix even though its static to ease future
> changes toward bytestream2

applied without this and also the ff_ prefix removed as requested
for the other patch, so its consistent

[...]
James Almer July 29, 2017, 2:15 a.m. UTC | #2 
On 7/28/2017 10:57 PM, Michael Niedermayer wrote:
> On Fri, Jul 28, 2017 at 03:47:05PM +0200, Michael Niedermayer wrote:
>> Fixes: out of array accesses
>>
> 
>> The new function uses ff_ prefix even though its static to ease future
>> changes toward bytestream2
> 
> applied without this and also the ff_ prefix removed as requested
> for the other patch, so its consistent

I had actually not looked at this patch when i wrote that suggestion.

I'm not sure how the ff_ prefix would ease future changes to bytestream2
with a static function, but if you think it's worth doing then you can
keep it. It was mostly a nit.
diff mbox

Patch

diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
index 752d92a42b..68c688136c 100644
--- a/libavformat/rtmppkt.c
+++ b/libavformat/rtmppkt.c
@@ -497,53 +497,70 @@  int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end)
     return bytestream2_tell(&gb);
 }
 
-int ff_amf_get_field_value(const uint8_t *data, const uint8_t *data_end,
+static int ff_amf_get_field_value2(GetByteContext *gb,
                            const uint8_t *name, uint8_t *dst, int dst_size)
 {
     int namelen = strlen(name);
     int len;
 
-    while (*data != AMF_DATA_TYPE_OBJECT && data < data_end) {
-        len = ff_amf_tag_size(data, data_end);
-        if (len < 0)
-            len = data_end - data;
-        data += len;
+    while (bytestream2_peek_byte(gb) != AMF_DATA_TYPE_OBJECT && bytestream2_get_bytes_left(gb) > 0) {
+        int ret = ff_amf_tag_skip(gb);
+        if (ret < 0)
+            return -1;
     }
-    if (data_end - data < 3)
+    if (bytestream2_get_bytes_left(gb) < 3)
         return -1;
-    data++;
+    bytestream2_get_byte(gb);
+
     for (;;) {
-        int size = bytestream_get_be16(&data);
+        int size = bytestream2_get_be16(gb);
         if (!size)
             break;
-        if (size < 0 || size >= data_end - data)
+        if (size < 0 || size >= bytestream2_get_bytes_left(gb))
             return -1;
-        data += size;
-        if (size == namelen && !memcmp(data-size, name, namelen)) {
-            switch (*data++) {
+        bytestream2_skip(gb, size);
+        if (size == namelen && !memcmp(gb->buffer-size, name, namelen)) {
+            switch (bytestream2_get_byte(gb)) {
             case AMF_DATA_TYPE_NUMBER:
-                snprintf(dst, dst_size, "%g", av_int2double(AV_RB64(data)));
+                snprintf(dst, dst_size, "%g", av_int2double(bytestream2_get_be64(gb)));
                 break;
             case AMF_DATA_TYPE_BOOL:
-                snprintf(dst, dst_size, "%s", *data ? "true" : "false");
+                snprintf(dst, dst_size, "%s", bytestream2_get_byte(gb) ? "true" : "false");
                 break;
             case AMF_DATA_TYPE_STRING:
-                len = bytestream_get_be16(&data);
-                av_strlcpy(dst, data, FFMIN(len+1, dst_size));
+                len = bytestream2_get_be16(gb);
+                if (dst_size < 1)
+                    return -1;
+                if (dst_size < len + 1)
+                    len = dst_size - 1;
+                bytestream2_get_buffer(gb, dst, len);
+                dst[len] = 0;
                 break;
             default:
                 return -1;
             }
             return 0;
         }
-        len = ff_amf_tag_size(data, data_end);
-        if (len < 0 || len >= data_end - data)
+        len = ff_amf_tag_skip(gb);
+        if (len < 0 || bytestream2_get_bytes_left(gb) <= 0)
             return -1;
-        data += len;
     }
     return -1;
 }
 
+int ff_amf_get_field_value(const uint8_t *data, const uint8_t *data_end,
+                           const uint8_t *name, uint8_t *dst, int dst_size)
+{
+    GetByteContext gb;
+
+    if (data >= data_end)
+        return -1;
+
+    bytestream2_init(&gb, data, data_end - data);
+
+    return ff_amf_get_field_value2(&gb, name, dst, dst_size);
+}
+
 static const char* rtmp_packet_type(int type)
 {
     switch (type) {