Re: [FFmpeg-devel] [PATCH v2] tools/target_dec_fuzzer: use refcounted packets

2019-08-23 Thread James Almer 
On 8/22/2019 4:01 PM, James Almer wrote:
> On 8/21/2019 10:21 AM, James Almer wrote:
>> On 8/21/2019 6:15 AM, Tomas Härdin wrote:
>>> tis 2019-08-20 klockan 21:05 -0300 skrev James Almer:
 Should reduce date copying considerably.

 Signed-off-by: James Almer 
 ---
 Fixed a stupid mistake when checking the return value for av_new_packet().
 Still untested.
>>>
>>> Works great for me. Should make fuzzing faster overall, better use of
>>> computing resources imo
>>>
 @@ -186,6 +144,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
 size) {
  error("Failed memory allocation");
  
  ctx->max_pixels = maxpixels_per_frame; //To reduce false positive OOM 
 and hangs
 +ctx->refcounted_frames = 1;
>>>
>>> Could maybe have a comment that this is also to reduce false positives,
>>> or that we want to focus on the new API rather than the old one
>>
>> Sure.
>>
>>>
 @@ -240,7 +199,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, 
 size_t size) {
  if (data + sizeof(fuzz_tag) > end)
  data = end;
  
 -FDBPrepare(, , last, data - last);
 +res = av_new_packet(, data - last);
 +if (res < 0)
 +error("Failed memory allocation");
 +memcpy(parsepkt.data, last, data - last);
>>>
>>> Is there some way to avoid this copy?
>>
>> I could create packets that point to a specific offset in the input
>> fuzzed buffer, but that will mean they will lack the padding required by
>> the API, not to mention the complete lack of control over said buffer's
>> lifetime. This could either generate no issues, or make things go really
>> wrong.
>>
>> So to make proper use of the AVPacket API, i need to allocate their
>> reference counted buffers and populate them. After this patch the
>> behaviour of this fuzzer is essentially the same as if it was a
>> libavformat demuxer, creating packets out of an input file and
>> propagating them to libavcodec.
>> If the idea was to imitate an average library user's behavior, this
>> pretty much is it.
>>
>>>
>>> /Tomas
> 
> Will push with the requested comment added soon.

Pushed, thanks.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] tools/target_dec_fuzzer: use refcounted packets

2019-08-22 Thread James Almer 
On 8/21/2019 10:21 AM, James Almer wrote:
> On 8/21/2019 6:15 AM, Tomas Härdin wrote:
>> tis 2019-08-20 klockan 21:05 -0300 skrev James Almer:
>>> Should reduce date copying considerably.
>>>
>>> Signed-off-by: James Almer 
>>> ---
>>> Fixed a stupid mistake when checking the return value for av_new_packet().
>>> Still untested.
>>
>> Works great for me. Should make fuzzing faster overall, better use of
>> computing resources imo
>>
>>> @@ -186,6 +144,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
>>> size) {
>>>  error("Failed memory allocation");
>>>  
>>>  ctx->max_pixels = maxpixels_per_frame; //To reduce false positive OOM 
>>> and hangs
>>> +ctx->refcounted_frames = 1;
>>
>> Could maybe have a comment that this is also to reduce false positives,
>> or that we want to focus on the new API rather than the old one
> 
> Sure.
> 
>>
>>> @@ -240,7 +199,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
>>> size) {
>>>  if (data + sizeof(fuzz_tag) > end)
>>>  data = end;
>>>  
>>> -FDBPrepare(, , last, data - last);
>>> +res = av_new_packet(, data - last);
>>> +if (res < 0)
>>> +error("Failed memory allocation");
>>> +memcpy(parsepkt.data, last, data - last);
>>
>> Is there some way to avoid this copy?
> 
> I could create packets that point to a specific offset in the input
> fuzzed buffer, but that will mean they will lack the padding required by
> the API, not to mention the complete lack of control over said buffer's
> lifetime. This could either generate no issues, or make things go really
> wrong.
> 
> So to make proper use of the AVPacket API, i need to allocate their
> reference counted buffers and populate them. After this patch the
> behaviour of this fuzzer is essentially the same as if it was a
> libavformat demuxer, creating packets out of an input file and
> propagating them to libavcodec.
> If the idea was to imitate an average library user's behavior, this
> pretty much is it.
> 
>>
>> /Tomas

Will push with the requested comment added soon.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] tools/target_dec_fuzzer: use refcounted packets

2019-08-21 Thread James Almer 
On 8/21/2019 6:15 AM, Tomas Härdin wrote:
> tis 2019-08-20 klockan 21:05 -0300 skrev James Almer:
>> Should reduce date copying considerably.
>>
>> Signed-off-by: James Almer 
>> ---
>> Fixed a stupid mistake when checking the return value for av_new_packet().
>> Still untested.
> 
> Works great for me. Should make fuzzing faster overall, better use of
> computing resources imo
> 
>> @@ -186,6 +144,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
>> size) {
>>  error("Failed memory allocation");
>>  
>>  ctx->max_pixels = maxpixels_per_frame; //To reduce false positive OOM 
>> and hangs
>> +ctx->refcounted_frames = 1;
> 
> Could maybe have a comment that this is also to reduce false positives,
> or that we want to focus on the new API rather than the old one

Sure.

> 
>> @@ -240,7 +199,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
>> size) {
>>  if (data + sizeof(fuzz_tag) > end)
>>  data = end;
>>  
>> -FDBPrepare(, , last, data - last);
>> +res = av_new_packet(, data - last);
>> +if (res < 0)
>> +error("Failed memory allocation");
>> +memcpy(parsepkt.data, last, data - last);
> 
> Is there some way to avoid this copy?

I could create packets that point to a specific offset in the input
fuzzed buffer, but that will mean they will lack the padding required by
the API, not to mention the complete lack of control over said buffer's
lifetime. This could either generate no issues, or make things go really
wrong.

So to make proper use of the AVPacket API, i need to allocate their
reference counted buffers and populate them. After this patch the
behaviour of this fuzzer is essentially the same as if it was a
libavformat demuxer, creating packets out of an input file and
propagating them to libavcodec.
If the idea was to imitate an average library user's behavior, this
pretty much is it.

> 
> /Tomas
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] tools/target_dec_fuzzer: use refcounted packets

2019-08-21 Thread Tomas Härdin 
tis 2019-08-20 klockan 21:05 -0300 skrev James Almer:
> Should reduce date copying considerably.
> 
> Signed-off-by: James Almer 
> ---
> Fixed a stupid mistake when checking the return value for av_new_packet().
> Still untested.

Works great for me. Should make fuzzing faster overall, better use of
computing resources imo

> @@ -186,6 +144,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
> size) {
>  error("Failed memory allocation");
>  
>  ctx->max_pixels = maxpixels_per_frame; //To reduce false positive OOM 
> and hangs
> +ctx->refcounted_frames = 1;

Could maybe have a comment that this is also to reduce false positives,
or that we want to focus on the new API rather than the old one

> @@ -240,7 +199,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
> size) {
>  if (data + sizeof(fuzz_tag) > end)
>  data = end;
>  
> -FDBPrepare(, , last, data - last);
> +res = av_new_packet(, data - last);
> +if (res < 0)
> +error("Failed memory allocation");
> +memcpy(parsepkt.data, last, data - last);

Is there some way to avoid this copy?

/Tomas

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v2] tools/target_dec_fuzzer: use refcounted packets

2019-08-20 Thread James Almer 
Should reduce date copying considerably.

Signed-off-by: James Almer 
---
Fixed a stupid mistake when checking the return value for av_new_packet().
Still untested.

 tools/target_dec_fuzzer.c | 71 ---
 1 file changed, 21 insertions(+), 50 deletions(-)

diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c
index d83039417c..0d10503cfb 100644
--- a/tools/target_dec_fuzzer.c
+++ b/tools/target_dec_fuzzer.c
@@ -85,47 +85,6 @@ static int subtitle_handler(AVCodecContext *avctx, void 
*frame,
 return ret;
 }
 
-// Class to handle buffer allocation and resize for each frame
-typedef struct FuzzDataBuffer {
-size_t size_;
-uint8_t *data_;
-} FuzzDataBuffer;
-
-static void FDBCreate(FuzzDataBuffer *FDB) {
-FDB->size_ = 0x1000;
-FDB->data_ = av_malloc(FDB->size_);
-if (!FDB->data_)
-error("Failed memory allocation");
-}
-
-static void FDBDesroy(FuzzDataBuffer *FDB) { av_free(FDB->data_); }
-
-static void FDBRealloc(FuzzDataBuffer *FDB, size_t size) {
-size_t needed = size + AV_INPUT_BUFFER_PADDING_SIZE;
-av_assert0(needed > size);
-if (needed > FDB->size_) {
-av_free(FDB->data_);
-FDB->size_ = needed;
-FDB->data_ = av_malloc(FDB->size_);
-if (!FDB->data_)
-error("Failed memory allocation");
-}
-}
-
-static void FDBPrepare(FuzzDataBuffer *FDB, AVPacket *dst, const uint8_t *data,
-size_t size)
-{
-FDBRealloc(FDB, size);
-memcpy(FDB->data_, data, size);
-size_t padd = FDB->size_ - size;
-if (padd > AV_INPUT_BUFFER_PADDING_SIZE)
-padd = AV_INPUT_BUFFER_PADDING_SIZE;
-memset(FDB->data_ + size, 0, padd);
-av_init_packet(dst);
-dst->data = FDB->data_;
-dst->size = size;
-}
-
 // Ensure we don't loop forever
 const uint32_t maxiteration = 8096;
 const uint64_t maxpixels_per_frame = 4096 * 4096;
@@ -135,7 +94,6 @@ static const uint64_t FUZZ_TAG = 0x4741542D5A5A5546ULL;
 
 int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
 const uint64_t fuzz_tag = FUZZ_TAG;
-FuzzDataBuffer buffer;
 const uint8_t *last = data;
 const uint8_t *end = data + size;
 uint32_t it = 0;
@@ -186,6 +144,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
size) {
 error("Failed memory allocation");
 
 ctx->max_pixels = maxpixels_per_frame; //To reduce false positive OOM and 
hangs
+ctx->refcounted_frames = 1;
 
 if (size > 1024) {
 GetByteContext gbc;
@@ -222,7 +181,6 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
size) {
 }
 parser_avctx->codec_id = ctx->codec_id;
 
-FDBCreate();
 int got_frame;
 AVFrame *frame = av_frame_alloc();
 if (!frame)
@@ -230,6 +188,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
size) {
 
 // Read very simple container
 AVPacket avpkt, parsepkt;
+av_init_packet();
 while (data < end && it < maxiteration) {
 // Search for the TAG
 while (data + sizeof(fuzz_tag) < end) {
@@ -240,7 +199,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
size) {
 if (data + sizeof(fuzz_tag) > end)
 data = end;
 
-FDBPrepare(, , last, data - last);
+res = av_new_packet(, data - last);
+if (res < 0)
+error("Failed memory allocation");
+memcpy(parsepkt.data, last, data - last);
 data += sizeof(fuzz_tag);
 last = data;
 
@@ -257,13 +219,21 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
size) {
 avpkt.pts = parser->pts;
 avpkt.dts = parser->dts;
 avpkt.pos = parser->pos;
+if (avpkt.data == parsepkt.data) {
+avpkt.buf = av_buffer_ref(parsepkt.buf);
+if (!avpkt.buf)
+error("Failed memory allocation");
+} else {
+ret = av_packet_make_refcounted();
+if (ret < 0)
+error("Failed memory allocation");
+}
 if ( parser->key_frame == 1 ||
 (parser->key_frame == -1 && parser->pict_type == 
AV_PICTURE_TYPE_I))
 avpkt.flags |= AV_PKT_FLAG_KEY;
 avpkt.flags |= parsepkt.flags & AV_PKT_FLAG_DISCARD;
 } else {
-avpkt = parsepkt;
-parsepkt.size = 0;
+av_packet_move_ref(, );
 }
 
   // Iterate through all data
@@ -284,16 +254,17 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t 
size) {
 avpkt.data += ret;
 avpkt.size -= ret;
   }
+  av_packet_unref();
 }
+av_packet_unref();
 }
 maximums_reached:
 
-av_init_packet();
-avpkt.data = NULL;
-avpkt.size = 0;
+av_packet_unref();
 
 do {
 got_frame = 0;
+av_frame_unref(frame);
 decode_handler(ctx, frame,