[FFmpeg-devel] [PATCH] pass MpegTSContext ptr explicitly when it's needed (fixes #3721)

[Marton Balint]

On Sun, 6 Jul 2014, Alexander V. Lukyanov wrote:

> From: "Alexander V. Lukyanov" <lavv17f at gmail.com>
>
> AVFormatContext->priv_data is not always a MpegTSContext, it can be
> RTSPState when decoding a RTP stream. So it is necessary to pass
> MpegTSContext pointer explicitly along with AVFormatContext (or alone).
>
> This fixes memory corruption from bug #3721 (RTSPState is smaller than
> MpegTSContext thus innocent memory gets overwritten).
>
> Signed-off-by: Alexander V. Lukyanov <lavv17f at gmail.com>
> ---
> libavformat/mpegts.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 3434341..6dc5d77 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -357,10 +357,9 @@ static int discard_pid(MpegTSContext *ts, unsigned int pid)
>  *  Assemble PES packets out of TS packets, and then call the "section_cb"
>  *  function when they are complete.
>  */
> -static void write_section_data(AVFormatContext *s, MpegTSFilter *tss1,
> +static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>                                const uint8_t *buf, int buf_size, int is_start)
> {
> -    MpegTSContext *ts = s->priv_data;
>     MpegTSSectionFilter *tss = &tss1->u.section_filter;
>     int len;
>
> @@ -2010,7 +2009,6 @@ static int parse_pcr(int64_t *ppcr_high, int *ppcr_low,
> /* handle one TS packet */
> static int handle_packet(MpegTSContext *ts, const uint8_t *packet)
> {
> -    AVFormatContext *s = ts->stream;
>     MpegTSFilter *tss;
>     int len, pid, cc, expected_cc, cc_ok, afc, is_start, is_discontinuity,
>         has_adaptation, has_payload;
> @@ -2084,7 +2082,7 @@ static int handle_packet(MpegTSContext *ts, const uint8_t *packet)
>                 return 0;
>             if (len && cc_ok) {
>                 /* write remaining section bytes */
> -                write_section_data(s, tss,
> +                write_section_data(ts, tss,
>                                    p, len, 0);
>                 /* check whether filter has been closed */
>                 if (!ts->pids[pid])
> @@ -2092,12 +2090,12 @@ static int handle_packet(MpegTSContext *ts, const uint8_t *packet)
>             }
>             p += len;
>             if (p < p_end) {
> -                write_section_data(s, tss,
> +                write_section_data(ts, tss,
>                                    p, p_end - p, 1);
>             }
>         } else {
>             if (cc_ok) {
> -                write_section_data(s, tss,
> +                write_section_data(ts, tss,
>                                    p, p_end - p, 0);
>             }
>         }

So far, the patch is OK. But do you really need the remaining hunks in 
order to fix the crash? Because as far as I see, the remaining hunks 
affects code that is not used by the parsing functions. Or am I missing 
something?

> @@ -2170,7 +2168,7 @@ static void reanalyze(MpegTSContext *ts) {
>
> /* XXX: try to find a better synchro over several packets (use
>  * get_packet_size() ?) */
> -static int mpegts_resync(AVFormatContext *s)
> +static int mpegts_resync(AVFormatContext *s, MpegTSContext *ts)
> {
>     AVIOContext *pb = s->pb;
>     int c, i;
> @@ -2181,7 +2179,7 @@ static int mpegts_resync(AVFormatContext *s)
>             return AVERROR_EOF;
>         if (c == 0x47) {
>             avio_seek(pb, -1, SEEK_CUR);
> -            reanalyze(s->priv_data);
> +            reanalyze(ts);
>             return 0;
>         }
>     }
> @@ -2192,7 +2190,7 @@ static int mpegts_resync(AVFormatContext *s)
> }
>
> /* return AVERROR_something if error or EOF. Return 0 if OK. */
> -static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
> +static int read_packet(AVFormatContext *s, MpegTSContext *ts, uint8_t *buf, int raw_packet_size,
>                        const uint8_t **data)
> {
>     AVIOContext *pb = s->pb;
> @@ -2208,7 +2206,7 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
>             uint64_t pos = avio_tell(pb);
>             avio_seek(pb, -FFMIN(raw_packet_size, pos), SEEK_CUR);
>
> -            if (mpegts_resync(s) < 0)
> +            if (mpegts_resync(s, ts) < 0)
>                 return AVERROR(EAGAIN);
>             else
>                 continue;
> @@ -2265,7 +2263,7 @@ static int handle_packets(MpegTSContext *ts, int nb_packets)
>         if (ts->stop_parse > 0)
>             break;
>
> -        ret = read_packet(s, packet, ts->raw_packet_size, &data);
> +        ret = read_packet(s, ts, packet, ts->raw_packet_size, &data);
>         if (ret != 0)
>             break;
>         ret = handle_packet(ts, data);
> @@ -2409,7 +2407,7 @@ static int mpegts_read_header(AVFormatContext *s)
>         nb_pcrs    = 0;
>         nb_packets = 0;
>         for (;;) {
> -            ret = read_packet(s, packet, ts->raw_packet_size, &data);
> +            ret = read_packet(s, ts, packet, ts->raw_packet_size, &data);
>             if (ret < 0)
>                 return ret;
>             pid = AV_RB16(data + 1) & 0x1fff;
> @@ -2456,7 +2454,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
>
>     if (av_new_packet(pkt, TS_PACKET_SIZE) < 0)
>         return AVERROR(ENOMEM);
> -    ret = read_packet(s, pkt->data, ts->raw_packet_size, &data);
> +    ret = read_packet(s, ts, pkt->data, ts->raw_packet_size, &data);
>     pkt->pos = avio_tell(s->pb);
>     if (ret < 0) {
>         av_free_packet(pkt);
> @@ -2558,7 +2556,7 @@ static av_unused int64_t mpegts_get_pcr(AVFormatContext *s, int stream_index,
>             return AV_NOPTS_VALUE;
>         if (buf[0] != 0x47) {
>             avio_seek(s->pb, -TS_PACKET_SIZE, SEEK_CUR);
> -            if (mpegts_resync(s) < 0)
> +            if (mpegts_resync(s, ts) < 0)
>                 return AV_NOPTS_VALUE;
>             pos = avio_tell(s->pb);
>             continue;
> -- 
> 1.8.3.1
>

Thanks,
Marton