[FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
Check that the required plane pointers and only
those are set up.
Signed-off-by: Reimar Döffinger
---
libavcodec/utils.c | 15 +++
libavutil/frame.h | 3 +++
2 files changed, 18 insertions(+)
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index af21cdd..56c8d7c 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -885,6 +885,21 @@ static int get_buffer_internal(AVCodecContext *avctx,
AVFrame *frame, int flags)
avctx->sw_pix_fmt = avctx->pix_fmt;
ret = avctx->get_buffer2(avctx, frame, flags);
+if (ret >= 0 && avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
+int i;
+int num_planes = av_pix_fmt_count_planes(frame->format);
+av_assert0(frame->format == avctx->pix_fmt);
+for (i = 0; i < num_planes; i++) {
+av_assert0(frame->data[i]);
+}
+// for formats without data like hwaccel allow
+// pointers to be non-NULL.
+for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
+if (frame->data[i])
+av_log(avctx, AV_LOG_ERROR, "Buffer returned by get_buffer2()
did not zero unused plane pointers\n");
+frame->data[i] = NULL;
+}
+}
end:
if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 56001a8..76a8123 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -187,6 +187,9 @@ typedef struct AVFrame {
* see avcodec_align_dimensions2(). Some filters and swscale can read
* up to 16 bytes beyond the planes, if these filters are to be used,
* then 16 extra bytes must be allocated.
+ *
+ * NOTE: Except for hwaccel formats, pointers not needed by the format
+ * MUST be set to NULL.
*/
uint8_t *data[AV_NUM_DATA_POINTERS];
--
2.7.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
Check that the required plane pointers and only
those are set up.
Signed-off-by: Reimar Döffinger
---
libavcodec/utils.c | 14 ++
libavutil/frame.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index af21cdd..af5ff93 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -885,6 +885,20 @@ static int get_buffer_internal(AVCodecContext *avctx,
AVFrame *frame, int flags)
avctx->sw_pix_fmt = avctx->pix_fmt;
ret = avctx->get_buffer2(avctx, frame, flags);
+if (ret >= 0 && avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
+int i;
+int num_planes = av_pix_fmt_count_planes(frame->format);
+for (i = 0; i < num_planes; i++) {
+av_assert0(frame->data[i]);
+}
+// for formats without data like hwaccel allow
+// pointers to be non-NULL.
+for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
+if (frame->data[i])
+av_log(avctx, AV_LOG_ERROR, "Buffer returned by get_buffer2()
did not zero unused plane pointers\n");
+frame->data[i] = NULL;
+}
+}
end:
if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 56001a8..76a8123 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -187,6 +187,9 @@ typedef struct AVFrame {
* see avcodec_align_dimensions2(). Some filters and swscale can read
* up to 16 bytes beyond the planes, if these filters are to be used,
* then 16 extra bytes must be allocated.
+ *
+ * NOTE: Except for hwaccel formats, pointers not needed by the format
+ * MUST be set to NULL.
*/
uint8_t *data[AV_NUM_DATA_POINTERS];
--
2.7.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
Check that the required plane pointers and only
those are set up.
Signed-off-by: Reimar Döffinger
---
libavcodec/utils.c | 20
libavutil/frame.h | 3 +++
2 files changed, 23 insertions(+)
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index af21cdd..1b6397c 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -853,6 +853,24 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame
*frame)
return ff_init_buffer_info(avctx, frame);
}
+static void validate_avframe_allocation(const AVCodecContext *avctx, AVFrame
*frame)
+{
+if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
+int i;
+int num_planes = av_pix_fmt_count_planes(frame->format);
+for (i = 0; i < num_planes; i++) {
+av_assert0(frame->data[i]);
+}
+// for formats without data like hwaccel allow
+// pointers to be non-NULL.
+for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
+if (frame->data[i])
+av_log(avctx, AV_LOG_ERROR, "Buffer returned by get_buffer2()
did not zero unused plane pointers\n");
+frame->data[i] = NULL;
+}
+}
+}
+
static int get_buffer_internal(AVCodecContext *avctx, AVFrame *frame, int
flags)
{
const AVHWAccel *hwaccel = avctx->hwaccel;
@@ -885,6 +903,8 @@ static int get_buffer_internal(AVCodecContext *avctx,
AVFrame *frame, int flags)
avctx->sw_pix_fmt = avctx->pix_fmt;
ret = avctx->get_buffer2(avctx, frame, flags);
+if (ret >= 0)
+validate_avframe_allocation(avctx, frame);
end:
if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 56001a8..76a8123 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -187,6 +187,9 @@ typedef struct AVFrame {
* see avcodec_align_dimensions2(). Some filters and swscale can read
* up to 16 bytes beyond the planes, if these filters are to be used,
* then 16 extra bytes must be allocated.
+ *
+ * NOTE: Except for hwaccel formats, pointers not needed by the format
+ * MUST be set to NULL.
*/
uint8_t *data[AV_NUM_DATA_POINTERS];
--
2.7.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
Check that the required plane pointers and only
those are set up.
Currently does not enforce anything for the palette
pointer of pseudopal formats as I am unsure about the
requirements.
Signed-off-by: Reimar Döffinger
---
libavcodec/utils.c | 27 +++
libavutil/frame.h | 3 +++
2 files changed, 30 insertions(+)
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index af21cdd..1967f03 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -853,6 +853,31 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame
*frame)
return ff_init_buffer_info(avctx, frame);
}
+static void validate_avframe_allocation(AVCodecContext *avctx, AVFrame *frame)
+{
+if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
+int i;
+int num_planes = av_pix_fmt_count_planes(frame->format);
+const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
+int flags = desc ? desc->flags : 0;
+if (num_planes == 1 && (flags & AV_PIX_FMT_FLAG_PAL))
+num_planes = 2;
+for (i = 0; i < num_planes; i++) {
+av_assert0(frame->data[i]);
+}
+// TODO: what are the exact requirements on pseudopal formats for
get_buffer2?
+if (num_planes == 1 && (flags & AV_PIX_FMT_FLAG_PSEUDOPAL))
+num_planes = 2;
+// for formats without data like hwaccel allow
+// pointers to be non-NULL.
+for (i = num_planes; num_planes > 0 && i <
FF_ARRAY_ELEMS(frame->data); i++) {
+if (frame->data[i])
+av_log(avctx, AV_LOG_ERROR, "Buffer returned by get_buffer2()
did not zero unused plane pointers\n");
+frame->data[i] = NULL;
+}
+}
+}
+
static int get_buffer_internal(AVCodecContext *avctx, AVFrame *frame, int
flags)
{
const AVHWAccel *hwaccel = avctx->hwaccel;
@@ -885,6 +910,8 @@ static int get_buffer_internal(AVCodecContext *avctx,
AVFrame *frame, int flags)
avctx->sw_pix_fmt = avctx->pix_fmt;
ret = avctx->get_buffer2(avctx, frame, flags);
+if (ret >= 0)
+validate_avframe_allocation(avctx, frame);
end:
if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 56001a8..76a8123 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -187,6 +187,9 @@ typedef struct AVFrame {
* see avcodec_align_dimensions2(). Some filters and swscale can read
* up to 16 bytes beyond the planes, if these filters are to be used,
* then 16 extra bytes must be allocated.
+ *
+ * NOTE: Except for hwaccel formats, pointers not needed by the format
+ * MUST be set to NULL.
*/
uint8_t *data[AV_NUM_DATA_POINTERS];
--
2.7.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
Check that the required plane pointers and only
those are set up.
Currently does not enforce anything for the palette
pointer of pseudopal formats as I am unsure about the
requirements.
Signed-off-by: Reimar Döffinger
---
doc/APIchanges | 6 ++
libavcodec/utils.c | 27 +++
libavcodec/version.h | 2 +-
libavutil/frame.h| 3 +++
4 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/doc/APIchanges b/doc/APIchanges
index 4e952a8..ee407da 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,12 @@ libavutil: 2015-08-28
API changes, most recent first:
+2016-02-xx - xxx - lavc 57.25.101
+ Validate AVFrame returned by get_buffer2 to have required
+ planes not NULL and unused planes set to NULL as crashes
+ and buffer overflow are possible with certain streams if
+ that is not the case.
+
2016-xx-xx - lavc 57.25.0 - avcodec.h
Add AVCodecContext.hw_frames_ctx.
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index af21cdd..1967f03 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -853,6 +853,31 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame
*frame)
return ff_init_buffer_info(avctx, frame);
}
+static void validate_avframe_allocation(AVCodecContext *avctx, AVFrame *frame)
+{
+if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
+int i;
+int num_planes = av_pix_fmt_count_planes(frame->format);
+const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
+int flags = desc ? desc->flags : 0;
+if (num_planes == 1 && (flags & AV_PIX_FMT_FLAG_PAL))
+num_planes = 2;
+for (i = 0; i < num_planes; i++) {
+av_assert0(frame->data[i]);
+}
+// TODO: what are the exact requirements on pseudopal formats for
get_buffer2?
+if (num_planes == 1 && (flags & AV_PIX_FMT_FLAG_PSEUDOPAL))
+num_planes = 2;
+// for formats without data like hwaccel allow
+// pointers to be non-NULL.
+for (i = num_planes; num_planes > 0 && i <
FF_ARRAY_ELEMS(frame->data); i++) {
+if (frame->data[i])
+av_log(avctx, AV_LOG_ERROR, "Buffer returned by get_buffer2()
did not zero unused plane pointers\n");
+frame->data[i] = NULL;
+}
+}
+}
+
static int get_buffer_internal(AVCodecContext *avctx, AVFrame *frame, int
flags)
{
const AVHWAccel *hwaccel = avctx->hwaccel;
@@ -885,6 +910,8 @@ static int get_buffer_internal(AVCodecContext *avctx,
AVFrame *frame, int flags)
avctx->sw_pix_fmt = avctx->pix_fmt;
ret = avctx->get_buffer2(avctx, frame, flags);
+if (ret >= 0)
+validate_avframe_allocation(avctx, frame);
end:
if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 0856b19..c937eaf 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@
#define LIBAVCODEC_VERSION_MAJOR 57
#define LIBAVCODEC_VERSION_MINOR 25
-#define LIBAVCODEC_VERSION_MICRO 101
+#define LIBAVCODEC_VERSION_MICRO 102
#define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
LIBAVCODEC_VERSION_MINOR, \
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 56001a8..76a8123 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -187,6 +187,9 @@ typedef struct AVFrame {
* see avcodec_align_dimensions2(). Some filters and swscale can read
* up to 16 bytes beyond the planes, if these filters are to be used,
* then 16 extra bytes must be allocated.
+ *
+ * NOTE: Except for hwaccel formats, pointers not needed by the format
+ * MUST be set to NULL.
*/
uint8_t *data[AV_NUM_DATA_POINTERS];
--
2.7.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On 2/26/2016 5:42 PM, Reimar Döffinger wrote: > +av_assert0(frame->format == avctx->pix_fmt); Is this valid? Mid-stream colorspace changes are fairly common. - erek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Fri, Feb 26, 2016 at 06:13:04PM +, Derek Buitenhuis wrote: > On 2/26/2016 5:42 PM, Reimar Döffinger wrote: > > +av_assert0(frame->format == avctx->pix_fmt); > > Is this valid? Mid-stream colorspace changes are fairly common. I believed we enforce a sequence point (i.e. we make sure to finish everything, free all resources and then continue) when such a change happens. However going by the documentation the get_buffer2 API at least allows them to differ, so I'll remove it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Fri, 26 Feb 2016 22:25:21 +0100
Reimar Döffinger wrote:
> Check that the required plane pointers and only
> those are set up.
>
> Signed-off-by: Reimar Döffinger
> ---
> libavcodec/utils.c | 14 ++
> libavutil/frame.h | 3 +++
> 2 files changed, 17 insertions(+)
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index af21cdd..af5ff93 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -885,6 +885,20 @@ static int get_buffer_internal(AVCodecContext *avctx,
> AVFrame *frame, int flags)
> avctx->sw_pix_fmt = avctx->pix_fmt;
>
> ret = avctx->get_buffer2(avctx, frame, flags);
> +if (ret >= 0 && avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> +int i;
> +int num_planes = av_pix_fmt_count_planes(frame->format);
> +for (i = 0; i < num_planes; i++) {
> +av_assert0(frame->data[i]);
> +}
> +// for formats without data like hwaccel allow
> +// pointers to be non-NULL.
> +for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
> +if (frame->data[i])
> +av_log(avctx, AV_LOG_ERROR, "Buffer returned by
> get_buffer2() did not zero unused plane pointers\n");
> +frame->data[i] = NULL;
> +}
> +}
>
Can you move it to a separate function? Say, ff_verify_frame(). This
could get much more complicated too when checking AVBufferRef usage or
audio.
> end:
> if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 56001a8..76a8123 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -187,6 +187,9 @@ typedef struct AVFrame {
> * see avcodec_align_dimensions2(). Some filters and swscale can read
> * up to 16 bytes beyond the planes, if these filters are to be used,
> * then 16 extra bytes must be allocated.
> + *
> + * NOTE: Except for hwaccel formats, pointers not needed by the format
> + * MUST be set to NULL.
> */
> uint8_t *data[AV_NUM_DATA_POINTERS];
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Fri, Feb 26, 2016 at 10:39:41PM +0100, wm4 wrote:
> On Fri, 26 Feb 2016 22:25:21 +0100
> Reimar Döffinger wrote:
>
> > Check that the required plane pointers and only
> > those are set up.
> >
> > Signed-off-by: Reimar Döffinger
> > ---
> > libavcodec/utils.c | 14 ++
> > libavutil/frame.h | 3 +++
> > 2 files changed, 17 insertions(+)
> >
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index af21cdd..af5ff93 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -885,6 +885,20 @@ static int get_buffer_internal(AVCodecContext *avctx,
> > AVFrame *frame, int flags)
> > avctx->sw_pix_fmt = avctx->pix_fmt;
> >
> > ret = avctx->get_buffer2(avctx, frame, flags);
> > +if (ret >= 0 && avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> > +int i;
> > +int num_planes = av_pix_fmt_count_planes(frame->format);
> > +for (i = 0; i < num_planes; i++) {
> > +av_assert0(frame->data[i]);
> > +}
> > +// for formats without data like hwaccel allow
> > +// pointers to be non-NULL.
> > +for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
> > +if (frame->data[i])
> > +av_log(avctx, AV_LOG_ERROR, "Buffer returned by
> > get_buffer2() did not zero unused plane pointers\n");
> > +frame->data[i] = NULL;
> > +}
> > +}
> >
>
> Can you move it to a separate function? Say, ff_verify_frame(). This
> could get much more complicated too when checking AVBufferRef usage or
> audio.
Why ff_? If we've decided on it and use it in other places it
might become a av_* function in libavutil.
But until then I guess it should be a static function in that file?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Fri, 26 Feb 2016 22:59:17 +0100
Reimar Döffinger wrote:
> On Fri, Feb 26, 2016 at 10:39:41PM +0100, wm4 wrote:
> > On Fri, 26 Feb 2016 22:25:21 +0100
> > Reimar Döffinger wrote:
> >
> > > Check that the required plane pointers and only
> > > those are set up.
> > >
> > > Signed-off-by: Reimar Döffinger
> > > ---
> > > libavcodec/utils.c | 14 ++
> > > libavutil/frame.h | 3 +++
> > > 2 files changed, 17 insertions(+)
> > >
> > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > > index af21cdd..af5ff93 100644
> > > --- a/libavcodec/utils.c
> > > +++ b/libavcodec/utils.c
> > > @@ -885,6 +885,20 @@ static int get_buffer_internal(AVCodecContext
> > > *avctx, AVFrame *frame, int flags)
> > > avctx->sw_pix_fmt = avctx->pix_fmt;
> > >
> > > ret = avctx->get_buffer2(avctx, frame, flags);
> > > +if (ret >= 0 && avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> > > +int i;
> > > +int num_planes = av_pix_fmt_count_planes(frame->format);
> > > +for (i = 0; i < num_planes; i++) {
> > > +av_assert0(frame->data[i]);
> > > +}
> > > +// for formats without data like hwaccel allow
> > > +// pointers to be non-NULL.
> > > +for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
> > > +if (frame->data[i])
> > > +av_log(avctx, AV_LOG_ERROR, "Buffer returned by
> > > get_buffer2() did not zero unused plane pointers\n");
> > > +frame->data[i] = NULL;
> > > +}
> > > +}
> > >
> >
> > Can you move it to a separate function? Say, ff_verify_frame(). This
> > could get much more complicated too when checking AVBufferRef usage or
> > audio.
>
> Why ff_? If we've decided on it and use it in other places it
> might become a av_* function in libavutil.
> But until then I guess it should be a static function in that file?
For now it also might be used in the encoding functions. Don't know how
far it could go.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Fri, Feb 26, 2016 at 11:06:22PM +0100, Reimar Döffinger wrote:
> Check that the required plane pointers and only
> those are set up.
>
> Signed-off-by: Reimar Döffinger
> ---
> libavcodec/utils.c | 20
> libavutil/frame.h | 3 +++
> 2 files changed, 23 insertions(+)
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index af21cdd..1b6397c 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -853,6 +853,24 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame
> *frame)
> return ff_init_buffer_info(avctx, frame);
> }
>
> +static void validate_avframe_allocation(const AVCodecContext *avctx, AVFrame
> *frame)
> +{
> +if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> +int i;
> +int num_planes = av_pix_fmt_count_planes(frame->format);
> +for (i = 0; i < num_planes; i++) {
> +av_assert0(frame->data[i]);
> +}
> +// for formats without data like hwaccel allow
> +// pointers to be non-NULL.
> +for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
are both of i = num_planes and i < num_planes intended ?
some note in APIChanges or release notes or something might make sense
too
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Sat, Feb 27, 2016 at 02:21:18AM +0100, Michael Niedermayer wrote:
> On Fri, Feb 26, 2016 at 11:06:22PM +0100, Reimar Döffinger wrote:
> > +static void validate_avframe_allocation(const AVCodecContext *avctx,
> > AVFrame *frame)
> > +{
> > +if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> > +int i;
> > +int num_planes = av_pix_fmt_count_planes(frame->format);
> > +for (i = 0; i < num_planes; i++) {
> > +av_assert0(frame->data[i]);
> > +}
> > +// for formats without data like hwaccel allow
> > +// pointers to be non-NULL.
>
> > +for (i = num_planes; num_planes > 0 && i < num_planes; i++) {
>
> are both of i = num_planes and i < num_planes intended ?
That uncovered a few more issues.
> some note in APIChanges or release notes or something might make sense
> too
Hm. Forgot about that in latest version.
Also, why can't I pass a "const AVCodecContext *" to av_log?
That is kind of inconvenient.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Fri, Feb 26, 2016 at 11:41:41PM +0100, wm4 wrote: > On Fri, 26 Feb 2016 22:59:17 +0100 > Reimar Döffinger wrote: > > > Can you move it to a separate function? Say, ff_verify_frame(). This > > > could get much more complicated too when checking AVBufferRef usage or > > > audio. > > > > Why ff_? If we've decided on it and use it in other places it > > might become a av_* function in libavutil. > > But until then I guess it should be a static function in that file? > > For now it also might be used in the encoding functions. Don't know how > far it could go. They are in the same file though, so static would still be fine. However as for encoding those are const AVFrames I think it is much less of an issue (should only cause overreads, not writes) and I'm more inclined to not change things that after all might break someone's (probably quite crappy) code. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Sat, Feb 27, 2016 at 01:01:40PM +0100, Reimar Döffinger wrote: > Check that the required plane pointers and only > those are set up. > Currently does not enforce anything for the palette > pointer of pseudopal formats as I am unsure about the > requirements. > > Signed-off-by: Reimar Döffinger > --- > doc/APIchanges | 6 ++ > libavcodec/utils.c | 27 +++ > libavcodec/version.h | 2 +- > libavutil/frame.h| 3 +++ > 4 files changed, 37 insertions(+), 1 deletion(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 4e952a8..ee407da 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,12 @@ libavutil: 2015-08-28 > > API changes, most recent first: > > +2016-02-xx - xxx - lavc 57.25.101 Fixed locally to say 57.25.102 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Sat, 27 Feb 2016 12:54:50 +0100 Reimar Döffinger wrote: > On Fri, Feb 26, 2016 at 11:41:41PM +0100, wm4 wrote: > > On Fri, 26 Feb 2016 22:59:17 +0100 > > Reimar Döffinger wrote: > > > > Can you move it to a separate function? Say, ff_verify_frame(). This > > > > could get much more complicated too when checking AVBufferRef usage or > > > > audio. > > > > > > Why ff_? If we've decided on it and use it in other places it > > > might become a av_* function in libavutil. > > > But until then I guess it should be a static function in that file? > > > > For now it also might be used in the encoding functions. Don't know how > > far it could go. > > They are in the same file though, so static would still be fine. > However as for encoding those are const AVFrames I think it > is much less of an issue (should only cause overreads, not > writes) and I'm more inclined to not change things that > after all might break someone's (probably quite crappy) code. If you look at avcodec_encode_audio2(), it already contains a huge chunk of code that somehow checks aspects of the AVFrame from the user. Could be unified one day. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Document and validate AVFrame plane pointers.
On Sat, 27 Feb 2016 13:01:40 +0100
Reimar Döffinger wrote:
> Check that the required plane pointers and only
> those are set up.
> Currently does not enforce anything for the palette
> pointer of pseudopal formats as I am unsure about the
> requirements.
>
> Signed-off-by: Reimar Döffinger
> ---
> doc/APIchanges | 6 ++
> libavcodec/utils.c | 27 +++
> libavcodec/version.h | 2 +-
> libavutil/frame.h| 3 +++
> 4 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 4e952a8..ee407da 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,12 @@ libavutil: 2015-08-28
>
> API changes, most recent first:
>
> +2016-02-xx - xxx - lavc 57.25.101
> + Validate AVFrame returned by get_buffer2 to have required
> + planes not NULL and unused planes set to NULL as crashes
> + and buffer overflow are possible with certain streams if
> + that is not the case.
> +
> 2016-xx-xx - lavc 57.25.0 - avcodec.h
>Add AVCodecContext.hw_frames_ctx.
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index af21cdd..1967f03 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -853,6 +853,31 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame
> *frame)
> return ff_init_buffer_info(avctx, frame);
> }
>
> +static void validate_avframe_allocation(AVCodecContext *avctx, AVFrame
> *frame)
> +{
> +if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> +int i;
> +int num_planes = av_pix_fmt_count_planes(frame->format);
> +const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
> +int flags = desc ? desc->flags : 0;
> +if (num_planes == 1 && (flags & AV_PIX_FMT_FLAG_PAL))
> +num_planes = 2;
> +for (i = 0; i < num_planes; i++) {
> +av_assert0(frame->data[i]);
> +}
> +// TODO: what are the exact requirements on pseudopal formats for
> get_buffer2?
> +if (num_planes == 1 && (flags & AV_PIX_FMT_FLAG_PSEUDOPAL))
> +num_planes = 2;
This should be correct. Not allocating a palette for unpaletted but
"pseudopal" formats can cause random crashes, as all code within FFmpeg
seems to assume that the palette has been allocated. (An utter
abomination, if you ask me.) At the same time, pixdesc will indicate
that paletted formats have only 1 plane, but 2 data pointers.
> +// for formats without data like hwaccel allow
> +// pointers to be non-NULL.
> +for (i = num_planes; num_planes > 0 && i <
> FF_ARRAY_ELEMS(frame->data); i++) {
> +if (frame->data[i])
> +av_log(avctx, AV_LOG_ERROR, "Buffer returned by
> get_buffer2() did not zero unused plane pointers\n");
> +frame->data[i] = NULL;
> +}
> +}
> +}
> +
> static int get_buffer_internal(AVCodecContext *avctx, AVFrame *frame, int
> flags)
> {
> const AVHWAccel *hwaccel = avctx->hwaccel;
> @@ -885,6 +910,8 @@ static int get_buffer_internal(AVCodecContext *avctx,
> AVFrame *frame, int flags)
> avctx->sw_pix_fmt = avctx->pix_fmt;
>
> ret = avctx->get_buffer2(avctx, frame, flags);
> +if (ret >= 0)
> +validate_avframe_allocation(avctx, frame);
>
> end:
> if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions) {
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 0856b19..c937eaf 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,7 +29,7 @@
>
> #define LIBAVCODEC_VERSION_MAJOR 57
> #define LIBAVCODEC_VERSION_MINOR 25
> -#define LIBAVCODEC_VERSION_MICRO 101
> +#define LIBAVCODEC_VERSION_MICRO 102
>
> #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> LIBAVCODEC_VERSION_MINOR, \
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 56001a8..76a8123 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -187,6 +187,9 @@ typedef struct AVFrame {
> * see avcodec_align_dimensions2(). Some filters and swscale can read
> * up to 16 bytes beyond the planes, if these filters are to be used,
> * then 16 extra bytes must be allocated.
> + *
> + * NOTE: Except for hwaccel formats, pointers not needed by the format
> + * MUST be set to NULL.
> */
> uint8_t *data[AV_NUM_DATA_POINTERS];
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
