diff mbox series

[FFmpeg-devel] Segfault calling av_interleaved_write_frame

Message ID CAC9dhghCr-cH2mANtjTcQz9DUDE8vWXnCSntoktZACZB3CoUVw@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] Segfault calling av_interleaved_write_frame
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork fail Failed to apply patch

Commit Message

Jonathan Noble Jan. 18, 2020, 9:40 p.m. UTC
Hello,

I found I was getting sigsegv when calling av_interleaved_write_frame().

From a18f4cb5b86392c5c161878daea8e4b1204881eb Mon Sep 17 00:00:00 2001
From: jon noble <jonnobleuk@gmail.com>
Date: Sat, 18 Jan 2020 21:33:11 +0000
Subject: [PATCH] libavformat/mux: prevent segfault in
 compute_muxer_pkt_fields()

---
 libavformat/mux.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


     //calculate dts from pts

Comments

Andreas Rheinhardt Jan. 18, 2020, 9:45 p.m. UTC | #1
On Sat, Jan 18, 2020 at 10:40 PM Jonathan Noble <jonnobleuk@gmail.com>
wrote:

> Hello,
>
> I found I was getting sigsegv when calling av_interleaved_write_frame().
>
> From a18f4cb5b86392c5c161878daea8e4b1204881eb Mon Sep 17 00:00:00 2001
> From: jon noble <jonnobleuk@gmail.com>
> Date: Sat, 18 Jan 2020 21:33:11 +0000
> Subject: [PATCH] libavformat/mux: prevent segfault in
>  compute_muxer_pkt_fields()
>
> ---
>  libavformat/mux.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 8ab5ea8c2b..ea79981fe8 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -598,9 +598,11 @@ static int compute_muxer_pkt_fields(AVFormatContext
> *s, AVStream *st, AVPacket *
>              av_log(s, AV_LOG_WARNING, "Encoder did not produce proper pts,
> making some up.\n");
>              warned = 1;
>          }
> -        pkt->dts =
> -//        pkt->pts= st->cur_dts;
> -            pkt->pts = st->internal->priv_pts->val;
> +        if (st->internal->priv_pts == NULL) {
> +          av_log(s, AV_LOG_WARNING, "Null private stream data.\n");
> +          return AVERROR(EINVAL);
> +        }
> +        pkt->dts = pkt->pts = st->internal->priv_pts->val;
>      }
>
>      //calculate dts from pts
> --
> 2.25.0
>
> Can you give us the command line together with the necessary sample files
that allow to reproduce this segfault?

- Andreas
Jonathan Noble Jan. 18, 2020, 9:46 p.m. UTC | #2
Oh, it wasn't via ffmpeg. I was using the library. What is the correct dev
mailing list?

On Sat, 18 Jan 2020 at 21:45, Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> On Sat, Jan 18, 2020 at 10:40 PM Jonathan Noble <jonnobleuk@gmail.com>
> wrote:
>
> > Hello,
> >
> > I found I was getting sigsegv when calling av_interleaved_write_frame().
> >
> > From a18f4cb5b86392c5c161878daea8e4b1204881eb Mon Sep 17 00:00:00 2001
> > From: jon noble <jonnobleuk@gmail.com>
> > Date: Sat, 18 Jan 2020 21:33:11 +0000
> > Subject: [PATCH] libavformat/mux: prevent segfault in
> >  compute_muxer_pkt_fields()
> >
> > ---
> >  libavformat/mux.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavformat/mux.c b/libavformat/mux.c
> > index 8ab5ea8c2b..ea79981fe8 100644
> > --- a/libavformat/mux.c
> > +++ b/libavformat/mux.c
> > @@ -598,9 +598,11 @@ static int compute_muxer_pkt_fields(AVFormatContext
> > *s, AVStream *st, AVPacket *
> >              av_log(s, AV_LOG_WARNING, "Encoder did not produce proper
> pts,
> > making some up.\n");
> >              warned = 1;
> >          }
> > -        pkt->dts =
> > -//        pkt->pts= st->cur_dts;
> > -            pkt->pts = st->internal->priv_pts->val;
> > +        if (st->internal->priv_pts == NULL) {
> > +          av_log(s, AV_LOG_WARNING, "Null private stream data.\n");
> > +          return AVERROR(EINVAL);
> > +        }
> > +        pkt->dts = pkt->pts = st->internal->priv_pts->val;
> >      }
> >
> >      //calculate dts from pts
> > --
> > 2.25.0
> >
> > Can you give us the command line together with the necessary sample files
> that allow to reproduce this segfault?
>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Jan. 18, 2020, 10:20 p.m. UTC | #3
Jonathan Noble (12020-01-18):
> Oh, it wasn't via ffmpeg. I was using the library. What is the correct dev
> mailing list?

Probably. We would need to see the code with the sample, to be sure
where the bug lies.

> On Sat, 18 Jan 2020 at 21:45, Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:

Top-posting is not allowed on this mailing-list. If you do not know what
it means, look it up.

Regards,
Andreas Rheinhardt Jan. 18, 2020, 10:24 p.m. UTC | #4
On Sat, Jan 18, 2020 at 11:18 PM Jonathan Noble <jonnobleuk@gmail.com>
wrote:

> Oh, it wasn't via ffmpeg. I was using the library. What is the correct dev
> mailing list?
>
>
Your patch was for FFmpeg itself and so this is the right mailing list. But
you should nevertheless give us enough information so that we can reproduce
the problem. After all, the bug may be in a completely different part than
the part where the segfault is triggered. (That being said, doing a
separate allocation for priv_pts seems weird in general.)

- Andreas

PS: Please avoid top-posting on this list.
Jonathan Noble Jan. 19, 2020, 9:35 a.m. UTC | #5
On Sat, 18 Jan 2020 at 22:20, Nicolas George <george@nsup.org> wrote:

> Jonathan Noble (12020-01-18):
> > Oh, it wasn't via ffmpeg. I was using the library. What is the correct
> dev
> > mailing list?
>
> Probably. We would need to see the code with the sample, to be sure
> where the bug lies.
>
I'm writing a wrapper around libav.
https://github.com/jonno85uk/mediahandling/tree/Issue4_encoding
I know what was causing this condition,  avformat_write_header() hadn't
been called at that time.

>
> > On Sat, 18 Jan 2020 at 21:45, Andreas Rheinhardt <
> > andreas.rheinhardt@gmail.com> wrote:
>
> Top-posting is not allowed on this mailing-list. If you do not know what
> it means, look it up.
>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Jan. 19, 2020, 11:52 a.m. UTC | #6
Jonathan Noble (12020-01-19):
> I'm writing a wrapper around libav.
> https://github.com/jonno85uk/mediahandling/tree/Issue4_encoding
> I know what was causing this condition,  avformat_write_header() hadn't
> been called at that time.

Calling avformat_write_header() is not optional, writing frames before
it is not supported.

For questions about the proper usage of the API, see this mailing-list:
https://ffmpeg.org/mailman/listinfo/libav-user

Regards,
Jonathan Noble Jan. 19, 2020, 10:50 p.m. UTC | #7
On Sun, 19 Jan 2020 at 11:52, Nicolas George <george@nsup.org> wrote:

> Jonathan Noble (12020-01-19):
> > I'm writing a wrapper around libav.
> > https://github.com/jonno85uk/mediahandling/tree/Issue4_encoding
> > I know what was causing this condition,  avformat_write_header() hadn't
> > been called at that time.
>
> Calling avformat_write_header() is not optional, writing frames before
> it is not supported.

I'd gathered that, but rather than bombing out shouldn't there be a AVERROR
returned or something else to notify the user of their error, like so many
other non-optional checks in ffmpeg?

>
> For questions about the proper usage of the API, see this mailing-list:
> https://ffmpeg.org/mailman/listinfo/libav-user
>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Jan. 19, 2020, 10:54 p.m. UTC | #8
Jonathan Noble (12020-01-19):
> I'd gathered that, but rather than bombing out shouldn't there be a AVERROR
> returned or something else to notify the user of their error, like so many
> other non-optional checks in ffmpeg?

No. AVERROR codes are not for errors that can be detected statically;
these need to be fixed, period.

This is not specific to FFmpeg: what do you get for calling fprintf() on
a NULL FILE pointer?

Regards,
Jonathan Noble Jan. 20, 2020, 9:34 a.m. UTC | #9
On Sun, 19 Jan 2020 at 22:54, Nicolas George <george@nsup.org> wrote:

> Jonathan Noble (12020-01-19):
> > I'd gathered that, but rather than bombing out shouldn't there be a
> AVERROR
> > returned or something else to notify the user of their error, like so
> many
> > other non-optional checks in ffmpeg?
>
> No. AVERROR codes are not for errors that can be detected statically;
> these need to be fixed, period.
>
> This is not specific to FFmpeg: what do you get for calling fprintf() on
> a NULL FILE pointer?
>
I wouldn't know. I try my best not to use C.

> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/mux.c b/libavformat/mux.c
index 8ab5ea8c2b..ea79981fe8 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -598,9 +598,11 @@  static int compute_muxer_pkt_fields(AVFormatContext
*s, AVStream *st, AVPacket *
             av_log(s, AV_LOG_WARNING, "Encoder did not produce proper pts,
making some up.\n");
             warned = 1;
         }
-        pkt->dts =
-//        pkt->pts= st->cur_dts;
-            pkt->pts = st->internal->priv_pts->val;
+        if (st->internal->priv_pts == NULL) {
+          av_log(s, AV_LOG_WARNING, "Null private stream data.\n");
+          return AVERROR(EINVAL);
+        }
+        pkt->dts = pkt->pts = st->internal->priv_pts->val;
     }