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

Marton Balint cus at passwd.hu
Mon Jul 7 21:24:42 CEST 2014



On Mon, 7 Jul 2014, Alexander Lukyanov wrote:

> The remaining parts were changed for consistency. As priv_data is not
> always MpegTSContext, it is illegal to cast it without certain checks.

I believe for the remaining parts priv_data is always MpegTSContext, 
therefore it is safe to cast to it. If really that is the case, I don't 
see why we should change the remaining code.

>
>
> 2014-07-07 3:24 GMT+04:00 Marton Balint <cus at passwd.hu>:
>
>>
>>
>> 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
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
>
> --
>   Alexander.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Regards,
Marton


More information about the ffmpeg-devel mailing list