<html>
    <head>
      <base href="https://bugzilla.libav.org/" />
    </head>
    <body><table border="1" cellspacing="0" cellpadding="8">
        <tr>
          <th>Bug ID</th>
          <td><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - flvenc.c potential buffer overrun with onTextData"
   href="https://bugzilla.libav.org/show_bug.cgi?id=1096">1096</a>
          </td>
        </tr>

        <tr>
          <th>Summary</th>
          <td>flvenc.c potential buffer overrun with onTextData
          </td>
        </tr>

        <tr>
          <th>Product</th>
          <td>Libav
          </td>
        </tr>

        <tr>
          <th>Version</th>
          <td>git HEAD
          </td>
        </tr>

        <tr>
          <th>Hardware</th>
          <td>Other
          </td>
        </tr>

        <tr>
          <th>OS</th>
          <td>Mac OS
          </td>
        </tr>

        <tr>
          <th>Status</th>
          <td>NEW
          </td>
        </tr>

        <tr>
          <th>Severity</th>
          <td>normal
          </td>
        </tr>

        <tr>
          <th>Priority</th>
          <td>---
          </td>
        </tr>

        <tr>
          <th>Component</th>
          <td>libavformat
          </td>
        </tr>

        <tr>
          <th>Assignee</th>
          <td>bugzilla@libav.org
          </td>
        </tr>

        <tr>
          <th>Reporter</th>
          <td>johan@stromnet.se
          </td>
        </tr></table>
      <p>
        <div>
        <pre>When flvenc.c writes onTextData packets in flv_write_packet[1], it assumes that
pkt->data is a null-terminated string (it uses strlen to determine length in
put_amf_string).

In flvdec.c, flv_data_packet[2] reads text packages using avio_rb16 +
av_get_packet which will just read 16 binary bytes and not do any null
termination. The output is actually null terminated though, due to 0 padding of
pkt->buf, but can that be relied upon?


Background:
I've got a use case where I was re-streaming (copy codec) rtmp -> rtmp,
unfortunately I cannot share any examples, but the TCPdump below [3] was seen
(focused on the text data as written by put_amf_string(pb, pkt->data)). 

A simple patch for this would be [4], but if pkt->data *is supposed* to be null
terminated, this might just hide the real problem (some other buffer overrun?)


1:
<a href="https://git.libav.org/?p=libav.git;a=blob;f=libavformat/flvenc.c;h=00bd65cb58549eed562f7f57004b4a10e44845ee;hb=HEAD#l543">https://git.libav.org/?p=libav.git;a=blob;f=libavformat/flvenc.c;h=00bd65cb58549eed562f7f57004b4a10e44845ee;hb=HEAD#l543</a>

2:
<a href="https://git.libav.org/?p=libav.git;a=blob;f=libavformat/flvdec.c;h=81a71d39f4cb9ec56f34d24233a73d6f2f17dd41;hb=HEAD#l697">https://git.libav.org/?p=libav.git;a=blob;f=libavformat/flvdec.c;h=81a71d39f4cb9ec56f34d24233a73d6f2f17dd41;hb=HEAD#l697</a>

3:
TCPDUMP of input stream:
0000   02 00 10 31 35 30 37 38 30 37 32 37 32 36 34 35  ...1507807272645
0010   38 35 30                                         850

TCPDUMP of output stream:
0000   02 00 11 31 35 30 37 38 30 37 32 37 32 36 34 35  ...1507807272645
0010   38 35 30 c5                                      850.

Note that the input indicates packet length 0x10 (16), but output is 0x11 (17)
with one garbage character added.
The number of extra bytes seems to vary, from 1-~5.




4: simple patch (line numbers are not correct due to other local patch on this
branch, but you understand the change):

--- a/libavformat/flvenc.c
+++ b/libavformat/flvenc.c
@@ -175,6 +175,12 @@ static void put_amf_string(AVIOContext *pb, const char
*str)
     avio_write(pb, str, len);
 }

+static void put_amf_string_pkt_data(AVIOContext *pb, const AVPacket *pkt)
+{
+    avio_wb16(pb, pkt->size);
+    avio_write(pb, pkt->data, pkt->size);
+}
+
 static void put_avc_eos_tag(AVIOContext *pb, unsigned ts)
 {
     avio_w8(pb, FLV_TAG_TYPE_VIDEO);
@@ -701,7 +707,7 @@ static int flv_write_packet(AVFormatContext *s, AVPacket
*pkt)
         put_amf_string(pb, "Text");
         put_amf_string(pb, "text");
         avio_w8(pb, AMF_DATA_TYPE_STRING);
-        put_amf_string(pb, pkt->data);
+        put_amf_string_pkt_data(pb, pkt);
         put_amf_string(pb, "");
         avio_w8(pb, AMF_END_OF_OBJECT);
         /* write total size of tag */</pre>
        </div>
      </p>
      <hr>
      <span>You are receiving this mail because:</span>
      
      <ul>
          <li>You are watching all bug changes.</li>
      </ul>
    </body>
</html>