Re: [FFmpeg-devel] [PATCH 2/3] wavpackenc: report too small buffer

2014-08-21 Thread Michael Niedermayer 
On Thu, Aug 21, 2014 at 02:32:46PM +0200, Christophe Gisquet wrote:
 Hi,
 
 2014-08-19 18:28 GMT+02:00 Christophe Gisquet christophe.gisq...@gmail.com:
  2014-08-19 16:20 GMT+02:00 Nicolas George geo...@nsup.org:
  IMHO, the correct error depends on how sure you are that a buffer too small
  SHOULD not happen.
 
  If you are very sure, then av_assert0().
 
  That would be it: I'm sure that, if the condition occurs and the
  packet is written anyway, the file will be incorrect.
 
 Anyway, that should never occur, so here's a version with av_assert0.
 
 -- 
 Christophe

  wavpackenc.c |2 ++
  1 file changed, 2 insertions(+)
 04628a0ec235881d99b91fda00ddebb79dbc340d  
 0001-wavpackenc-assert-on-too-small-buffer.patch
 From ae8998602fa0d9114a09291c4623a1609a5fcb8c Mon Sep 17 00:00:00 2001
 From: Christophe Gisquet christophe.gisq...@gmail.com
 Date: Tue, 19 Aug 2014 14:05:56 +0200
 Subject: [PATCH] wavpackenc: assert on too small buffer
 
 bytestream2_* will not cause buffer overflow, but in that case, this means
 the allocation would be incorrect and the encoded result invalid. Therefore,
 assert no overflow occurred.

applied

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/3] wavpackenc: report too small buffer

2014-08-19 Thread Christophe Gisquet 
bytestream2_* will not cause buffer overflow, but on the other hand,
it should be checked whether overflows have been prevented.
---
 libavcodec/wavpackenc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavcodec/wavpackenc.c b/libavcodec/wavpackenc.c
index 5b8973c..46c69a3 100644
--- a/libavcodec/wavpackenc.c
+++ b/libavcodec/wavpackenc.c
@@ -2813,6 +2813,11 @@ static int wavpack_encode_block(WavPackEncodeContext *s,
 block_size = bytestream2_tell_p(pb);
 AV_WL32(out + 4, block_size - 8);
 
+if (put_bits_left(s-pb) = 0) {
+av_log(s-avctx, AV_LOG_ERROR, Packet allocated too small\n);
+return AVERROR_INVALIDDATA;
+}
+
 return block_size;
 }
 
-- 
1.9.2.msysgit.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/3] wavpackenc: report too small buffer

2014-08-19 Thread Paul B Mahol 
On 8/19/14, Christophe Gisquet christophe.gisq...@gmail.com wrote:
 bytestream2_* will not cause buffer overflow, but on the other hand,
 it should be checked whether overflows have been prevented.
 ---
  libavcodec/wavpackenc.c | 5 +
  1 file changed, 5 insertions(+)

 diff --git a/libavcodec/wavpackenc.c b/libavcodec/wavpackenc.c
 index 5b8973c..46c69a3 100644
 --- a/libavcodec/wavpackenc.c
 +++ b/libavcodec/wavpackenc.c
 @@ -2813,6 +2813,11 @@ static int wavpack_encode_block(WavPackEncodeContext
 *s,
  block_size = bytestream2_tell_p(pb);
  AV_WL32(out + 4, block_size - 8);

 +if (put_bits_left(s-pb) = 0) {
 +av_log(s-avctx, AV_LOG_ERROR, Packet allocated too small\n);
 +return AVERROR_INVALIDDATA;
 +}
 +
  return block_size;
  }

 --
 1.9.2.msysgit.0

 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


ok
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/3] wavpackenc: report too small buffer

2014-08-19 Thread Nicolas George 
Le duodi 2 fructidor, an CCXXII, Christophe Gisquet a écrit :
  +return AVERROR_INVALIDDATA;
  Shouldn't this be AVERROR_BUG?
 This was selected as a default, not knowing exactly what to put here.
 There's even a AVERROR_BUFFER_TOO_SMALL, but I though it would be used
 for user-provided buffers.

IMHO, the correct error depends on how sure you are that a buffer too small
SHOULD not happen.

If you are very sure, then av_assert0().

If you are moderately sure but a mistake could have bad consequences
(corrupted memory; should not happen here), then av_assert0() too to prevent
the bad consequences from propagating.

If you are moderately sure and a buffer too small is not a severe issue,
AVERROR_BUG.

If you are not sure at all, then av_realloc().

As you pointed, BUFFER_TOO_SMALL is when you do not choose the size of the
buffer.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/3] wavpackenc: report too small buffer

2014-08-19 Thread Christophe Gisquet 
Hi,

2014-08-19 16:20 GMT+02:00 Nicolas George geo...@nsup.org:
 IMHO, the correct error depends on how sure you are that a buffer too small
 SHOULD not happen.

 If you are very sure, then av_assert0().

That would be it: I'm sure that, if the condition occurs and the
packet is written anyway, the file will be incorrect.

But that's if the packet is written. I would think this is a
recoverable issue/possibly with different outcomes, and just returning
the failure is good enough.

Reallocating is an alternate good solution, but I've already had my
share of that with prores. So I would prefer the assert then.

-- 
Christophe
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel