| Message ID | CAEVbG5rNwLD3hVh-JigVMA+RiU2LHiEWCnM7psPseV_d5YKr1A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 16, 2018 at 04:32:14PM -0700, Fredrik Hubinette wrote: > With some (garbled) OGG data, PTS can overflow causing undefined behavior. > This patch avoids that by zeroing out PTS values greater than 2^62. > oggdec.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > ff003b78842c7724ccc1a42f9584b1f8aa0b0b3d 0001-Avoid-undefined-behavior-by-limiting-PTS-to-62-bits-.patch > From 26a8582bc04f5bddc037ffcce99025e2f977abe0 Mon Sep 17 00:00:00 2001 > From: Fredrik Hubinette <hubbe@google.com> > Date: Mon, 16 Jul 2018 14:54:43 -0700 > Subject: [PATCH] Avoid undefined behavior by limiting PTS to 62 bits in ogg > decoder > > --- > libavformat/oggdec.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) I think someone reading this commit message would like to know more about where that undefined behaviour occurs and how this is guranteeing to fix it [...]
Not sure how to update the commit message. The undefined behavior occurs in av_add_stable, which is called from compute_packet_fields. In that code PTS can be equal to -(1<<63), which then causes a int64_t to overflow. On Wed, Jul 18, 2018 at 4:04 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Jul 16, 2018 at 04:32:14PM -0700, Fredrik Hubinette wrote: > > With some (garbled) OGG data, PTS can overflow causing undefined > behavior. > > This patch avoids that by zeroing out PTS values greater than 2^62. > > > oggdec.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > ff003b78842c7724ccc1a42f9584b1f8aa0b0b3d > 0001-Avoid-undefined-behavior-by-limiting-PTS-to-62-bits-.patch > > From 26a8582bc04f5bddc037ffcce99025e2f977abe0 Mon Sep 17 00:00:00 2001 > > From: Fredrik Hubinette <hubbe@google.com> > > Date: Mon, 16 Jul 2018 14:54:43 -0700 > > Subject: [PATCH] Avoid undefined behavior by limiting PTS to 62 bits in > ogg > > decoder > > > > --- > > libavformat/oggdec.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > I think someone reading this commit message would like to know more > about where that undefined behaviour occurs and how this is guranteeing > to fix it > > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > "Nothing to hide" only works if the folks in power share the values of > you and everyone you know entirely and always will -- Tom Scott > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, Jul 30, 2018 at 01:31:59PM -0700, Fredrik Hubinette wrote: > Not sure how to update the commit message. git commit --amend > The undefined behavior occurs in av_add_stable, which is called from > compute_packet_fields. > In that code PTS can be equal to -(1<<63), which then causes a int64_t to > overflow. This does not sound specific to oggdec. Thus a change in oggdec would likely not fix it outside ogg. Or am i missing something ? thx > > On Wed, Jul 18, 2018 at 4:04 AM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Mon, Jul 16, 2018 at 04:32:14PM -0700, Fredrik Hubinette wrote: > > > With some (garbled) OGG data, PTS can overflow causing undefined > > behavior. > > > This patch avoids that by zeroing out PTS values greater than 2^62. > > > > > oggdec.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > ff003b78842c7724ccc1a42f9584b1f8aa0b0b3d > > 0001-Avoid-undefined-behavior-by-limiting-PTS-to-62-bits-.patch > > > From 26a8582bc04f5bddc037ffcce99025e2f977abe0 Mon Sep 17 00:00:00 2001 > > > From: Fredrik Hubinette <hubbe@google.com> > > > Date: Mon, 16 Jul 2018 14:54:43 -0700 > > > Subject: [PATCH] Avoid undefined behavior by limiting PTS to 62 bits in > > ogg > > > decoder > > > > > > --- > > > libavformat/oggdec.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > I think someone reading this commit message would like to know more > > about where that undefined behaviour occurs and how this is guranteeing > > to fix it > > > > > > [...] > > > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > "Nothing to hide" only works if the folks in power share the values of > > you and everyone you know entirely and always will -- Tom Scott > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
I'm uncertain what the right thing to do here is. Oggdec generates PTS values which are a bit crazy, and the generic code that deals which those values encounters undefined behavior. It seems like there should be some input validation happening somewhere, but it's not clear to me if that belongs in oggdec or somewhere else. On Mon, Jul 30, 2018 at 4:23 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Jul 30, 2018 at 01:31:59PM -0700, Fredrik Hubinette wrote: > > Not sure how to update the commit message. > > git commit --amend > > > > The undefined behavior occurs in av_add_stable, which is called from > > compute_packet_fields. > > In that code PTS can be equal to -(1<<63), which then causes a int64_t to > > overflow. > > This does not sound specific to oggdec. Thus a change in oggdec would > likely > not fix it outside ogg. > Or am i missing something ? > > > thx > > > > > On Wed, Jul 18, 2018 at 4:04 AM Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > > > On Mon, Jul 16, 2018 at 04:32:14PM -0700, Fredrik Hubinette wrote: > > > > With some (garbled) OGG data, PTS can overflow causing undefined > > > behavior. > > > > This patch avoids that by zeroing out PTS values greater than 2^62. > > > > > > > oggdec.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > ff003b78842c7724ccc1a42f9584b1f8aa0b0b3d > > > 0001-Avoid-undefined-behavior-by-limiting-PTS-to-62-bits-.patch > > > > From 26a8582bc04f5bddc037ffcce99025e2f977abe0 Mon Sep 17 00:00:00 > 2001 > > > > From: Fredrik Hubinette <hubbe@google.com> > > > > Date: Mon, 16 Jul 2018 14:54:43 -0700 > > > > Subject: [PATCH] Avoid undefined behavior by limiting PTS to 62 bits > in > > > ogg > > > > decoder > > > > > > > > --- > > > > libavformat/oggdec.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > I think someone reading this commit message would like to know more > > > about where that undefined behaviour occurs and how this is guranteeing > > > to fix it > > > > > > > > > [...] > > > > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > > > "Nothing to hide" only works if the folks in power share the values of > > > you and everyone you know entirely and always will -- Tom Scott > > > > > > _______________________________________________ > > > ffmpeg-devel mailing list > > > ffmpeg-devel@ffmpeg.org > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Its not that you shouldnt use gotos but rather that you should write > readable code and code with gotos often but not always is less readable > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
From 26a8582bc04f5bddc037ffcce99025e2f977abe0 Mon Sep 17 00:00:00 2001 From: Fredrik Hubinette <hubbe@google.com> Date: Mon, 16 Jul 2018 14:54:43 -0700 Subject: [PATCH] Avoid undefined behavior by limiting PTS to 62 bits in ogg decoder --- libavformat/oggdec.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h index 4a2b6ddee8..798c74f671 100644 --- a/libavformat/oggdec.h +++ b/libavformat/oggdec.h @@ -162,8 +162,9 @@ ogg_gptopts (AVFormatContext * s, int i, uint64_t gp, int64_t *dts) if (dts) *dts = pts; } - if (pts > INT64_MAX && pts != AV_NOPTS_VALUE) { + if (pts > INT64_MAX / 2 && pts != AV_NOPTS_VALUE) { // The return type is unsigned, we thus cannot return negative pts + // Limit the return value to 62 bits to avoid undefined behavior. av_log(s, AV_LOG_ERROR, "invalid pts %"PRId64"\n", pts); pts = AV_NOPTS_VALUE; } -- 2.18.0.203.gfac676dfb9-goog