diff mbox series

[FFmpeg-devel,4/5] avfilter/vf_morpho: Fix invalid frees on error

Message ID AM7PR03MB666076D96F65ACDE120926018FAD9@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 2ee4077248dcd96e50efc0738e60519ffb9a4c7f
Headers show
Series [FFmpeg-devel,1/5] avfilter/vf_morpho: Fix leak of output frame on error | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Oct. 3, 2021, 9:48 a.m. UTC
The current code used a pointer to an array (of arrays) that
is offset relative to the start of the actually allocated buffer.
Yet offsetting the pointer is only done on success, whereas the
freeing code believes it to have happened even on error.
So if any of the subarrays (or the subarrays' subarrays) can't
be successfully allocated, one gets a bad free in free_lut().

Furthermore, said offsetting is only permissible in case the
offsetted pointer points in the allocated buffer (here: in case
the LUT's min_r is <= 0), as pointer arithmetic is undefined
in case it exceeds the allocated object.

Moreover, in case one of the subarrays couldn't be allocated,
the code nevertheless tried to free the subarray's subarrays;
and in case one of the subarray's subarrays could not be allocated
successfully, there will be an invalid free, too, because the
pointers for the subarrays' subarrays are also offset compared
to the base pointer.

This commit fixes all of this, by using the actually allocated
pointer for freeing and by adding appropriate checks before
freeing the subarrays. The former also allows to distinguish
the cases in which the lut is currently only half-allocated due to
an error in an earlier allocation attempt from the success case.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/vf_morpho.c | 49 +++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

Comments

Paul B Mahol Oct. 3, 2021, 9:53 a.m. UTC | #1
ok, i guess it was tested
diff mbox series

Patch

diff --git a/libavfilter/vf_morpho.c b/libavfilter/vf_morpho.c
index 8c1e084e7e..818ebd6b9a 100644
--- a/libavfilter/vf_morpho.c
+++ b/libavfilter/vf_morpho.c
@@ -61,7 +61,10 @@  typedef struct IPlane {
 } IPlane;
 
 typedef struct LUT {
+    /* arr is shifted from base_arr by FFMAX(min_r, 0).
+     * arr != NULL means "lut completely allocated" */
     uint8_t ***arr;
+    uint8_t ***base_arr;
     int min_r;
     int max_r;
     int I;
@@ -262,7 +265,8 @@  static void maxinplace16_fun(uint8_t *aa, const uint8_t *bb, int x)
 
 static int alloc_lut(LUT *Ty, chord_set *SE, int type_size, int mode)
 {
-    const int size = Ty->max_r + 1 - Ty->min_r;
+    const int min = FFMAX(Ty->min_r, 0);
+    const int max = min + (Ty->max_r - Ty->min_r);
     int pre_pad_x = 0;
 
     if (SE->minX < 0)
@@ -270,55 +274,58 @@  static int alloc_lut(LUT *Ty, chord_set *SE, int type_size, int mode)
     Ty->pre_pad_x = pre_pad_x;
     Ty->type_size = type_size;
 
-    Ty->arr = av_calloc(size, sizeof(*Ty->arr));
-    if (!Ty->arr)
+    Ty->base_arr = av_calloc(max + 1, sizeof(*Ty->base_arr));
+    if (!Ty->base_arr)
         return AVERROR(ENOMEM);
-    for (int r = 0; r < Ty->max_r - Ty->min_r + 1; r++) {
-        Ty->arr[r] = av_calloc(Ty->I, sizeof(uint8_t *));
-        if (!Ty->arr[r])
+    for (int r = min; r <= max; r++) {
+        uint8_t **arr = Ty->base_arr[r] = av_calloc(Ty->I, sizeof(uint8_t *));
+        if (!Ty->base_arr[r])
             return AVERROR(ENOMEM);
         for (int i = 0; i < Ty->I; i++) {
-            Ty->arr[r][i] = av_calloc(Ty->X + pre_pad_x, type_size);
-            if (!Ty->arr[r][i])
+            arr[i] = av_calloc(Ty->X + pre_pad_x, type_size);
+            if (!arr[i])
                 return AVERROR(ENOMEM);
             if (mode == ERODE)
-                memset(Ty->arr[r][i], UINT8_MAX, pre_pad_x * type_size);
+                memset(arr[i], UINT8_MAX, pre_pad_x * type_size);
             /* Shifting the X index such that negative indices correspond to
              * the pre-padding.
              */
-            Ty->arr[r][i] = &(Ty->arr[r][i][pre_pad_x * type_size]);
+            arr[i] = &(arr[i][pre_pad_x * type_size]);
         }
     }
 
-    Ty->arr = &(Ty->arr[0 - Ty->min_r]);
+    Ty->arr = &(Ty->base_arr[min - Ty->min_r]);
 
     return 0;
 }
 
 static void free_lut(LUT *table)
 {
-    uint8_t ***rp;
+    const int min = FFMAX(table->min_r, 0);
+    const int max = min + (table->max_r - table->min_r);
 
-    if (!table->arr)
+    if (!table->base_arr)
         return;
 
-    // The R index was shifted, create a pointer to the original array
-    rp = &(table->arr[table->min_r]);
-
-    for (int r = table->min_r; r <= table->max_r; r++) {
+    for (int r = min; r <= max; r++) {
+        if (!table->base_arr[r])
+            break;
         for (int i = 0; i < table->I; i++) {
+            if (!table->base_arr[r][i])
+                break;
             // The X index was also shifted, for padding purposes.
-            av_free(table->arr[r][i] - table->pre_pad_x * table->type_size);
+            av_free(table->base_arr[r][i] - table->pre_pad_x * table->type_size);
         }
-        av_freep(&table->arr[r]);
+        av_freep(&table->base_arr[r]);
     }
-    av_freep(&rp);
+    av_freep(&table->base_arr);
+    table->arr = NULL;
 }
 
 static int alloc_lut_if_necessary(LUT *Ty, IPlane *f, chord_set *SE,
                                   int y, int num, enum MorphModes mode)
 {
-    if (Ty->I != SE->Lnum ||
+    if (!Ty->arr || Ty->I != SE->Lnum ||
         Ty->X != f->w ||
         SE->minX < 0 && -SE->minX > Ty->pre_pad_x ||
         Ty->min_r != SE->minY ||