On Sun, 1 Jun 2014, Derek Buitenhuis wrote:

On 6/1/2014 6:19 PM, Luca Barbato wrote:
The code itself looks fine, you might factorize the deallocation with a
"goto fail;".

No.

On 6/1/2014 1:10 PM, Nidhi Makhijani wrote:
-            if (!hdr)
+            if (!hdr) {
+                int j;
+                for (j=1; j < i; j++)
+                    av_freep(&nut->header_len[j]);
+                av_freep(nut->time_base);

Either:

   av_freep(&nut->time_base);

or

   av_free(nut->time_base);

     nut->stream = av_mallocz(sizeof(StreamContext) * stream_count);
+    if (!nut->stream) {
+        int j;
+        for (j=1; j<nut->header_count; j++)
+            av_freep(&nut->header_len[j]);
+        av_freep(nut->time_base);
+        return AVERROR(ENOMEM);
+    }

Ditto.

For code like this, it's imperative that the author of the patches actually test run the error cleanup codepaths (in valgrind or something similar). Error handling codepaths that aren't normally exercised can easily be arbitrarily broken, which will only be found once someone runs fuzzing the next time.

// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to