[x264-devel] Re: [PATCH] malloc result test

Loïc Le Loarer lll+vlc at m4x.org
Mon Jun 5 10:48:14 CEST 2006


Le Tuesday 30 May 2006 à 00:19:22 -0700, Loren Merritt a écrit:
> On Wed, 24 May 2006, Loïc Le Loarer wrote:
> 
> >It seems that x264_malloc result is nearly never tested in allocation
> >functions, which could lead to segfaults in some situations.
> >
> >Here a small patch which adds the correct tests for x264_frame_new
> >function with correct unallocation when something goes wrong. If this
> >patch is accepted, I'll go on to have this kind of test where necessary.
> >
> >Best regards.
> 
> OK, but it doesn't help unless you check x264_frame_new's return value too 
> :)

Yes, but I just wanted to know if this kind of changes would be
accepted. Because checking x264_frame_new's return value is not enough,
there are several memory leaks in case of error in x264_encoder_open and
some other initialisation functions (and you added more by your checks)
and correcting them deserves a patch on it is own.

Here is a patch which completes the tests and removes all the memory and
file descriptors leaks in case of error I could see in x264_encoder_open
function.

Please see if it fits for you.

Best regards.

-- 
Loïc

"heaven is not a place, it's a feeling"
-------------- next part --------------
Index: encoder/encoder.c
===================================================================
--- encoder/encoder.c	(révision 531)
+++ encoder/encoder.c	(copie de travail)
@@ -457,6 +457,50 @@
 }
 
 /****************************************************************************
+ * x264_encoder_delete:
+ ****************************************************************************/
+static void x264_encoder_delete ( x264_t *h )
+{
+    int i;
+
+    /* param */
+    /* allocation is done by strdup, must use free here, not x264_free */
+    if( h->param.rc.psz_stat_out )
+        free( h->param.rc.psz_stat_out );
+    if( h->param.rc.psz_stat_in )
+        free( h->param.rc.psz_stat_in );
+    if( h->param.rc.psz_rc_eq )
+        free( h->param.rc.psz_rc_eq );
+
+    /* bitstream */
+    x264_free( h->out.p_bitstream );
+
+    /* frames */
+    for( i = 0; i < X264_BFRAME_MAX + 3; i++ )
+    {
+        if( h->frames.current[i] ) x264_frame_delete( h->frames.current[i] );
+        if( h->frames.next[i] )    x264_frame_delete( h->frames.next[i] );
+        if( h->frames.unused[i] )  x264_frame_delete( h->frames.unused[i] );
+    }
+    /* ref frames */
+    for( i = 0; i < X264_MAX_REFERENCES; i++ )
+    {
+        if( h->frames.reference[i] ) x264_frame_delete( h->frames.reference[i] );
+    }
+
+    /* cache */
+    x264_macroblock_cache_end( h );
+
+    /* rc */
+    x264_ratecontrol_delete( h );
+
+    /* thread */
+    for( i = 1; i < X264_THREAD_MAX; i++ )
+        x264_free( h->thread[i] );
+    x264_free( h );
+}
+
+/****************************************************************************
  * x264_encoder_open:
  ****************************************************************************/
 x264_t *x264_encoder_open   ( x264_param_t *param )
@@ -464,6 +508,11 @@
     x264_t *h = x264_malloc( sizeof( x264_t ) );
     int i;
 
+    if( NULL == h )
+    {
+        return NULL;
+    }
+
     memset( h, 0, sizeof( x264_t ) );
 
     /* Create a copy of param */
@@ -471,15 +520,13 @@
 
     if( x264_validate_parameters( h ) < 0 )
     {
-        x264_free( h );
-        return NULL;
+        goto fail;
     }
 
     if( h->param.psz_cqm_file )
         if( x264_cqm_parse_file( h, h->param.psz_cqm_file ) < 0 )
         {
-            x264_free( h );
-            return NULL;
+            goto fail;
         }
 
     if( h->param.rc.psz_stat_out )
@@ -524,7 +571,7 @@
     h->out.i_bitstream = X264_MAX( 1000000, h->param.i_width * h->param.i_height * 1.7
         * ( h->param.rc.b_cbr ? pow( 0.5, h->param.rc.i_qp_min )
           : pow( 0.5, h->param.rc.i_qp_constant ) * X264_MAX( 1, h->param.rc.f_ip_factor )));
-    h->out.p_bitstream = x264_malloc( h->out.i_bitstream );
+    CHECKED_MALLOC( h->out.p_bitstream, h->out.i_bitstream );
 
     h->i_frame = 0;
     h->i_frame_num = 0;
@@ -550,23 +597,18 @@
     h->frames.b_have_lowres = !h->param.rc.b_stat_read
         && ( h->param.rc.b_cbr || h->param.rc.i_rf_constant || h->param.b_bframe_adaptive );
 
-    for( i = 0; i < X264_BFRAME_MAX + 3; i++ )
-    {
-        h->frames.current[i] = NULL;
-        h->frames.next[i]    = NULL;
-        h->frames.unused[i]  = NULL;
-    }
+    /* Note that all frames pointers are initialised at NULL by memset 0 upper */
     for( i = 0; i < 1 + h->frames.i_delay; i++ )
     {
         h->frames.unused[i] =  x264_frame_new( h );
         if( !h->frames.unused[i] )
-            return NULL;
+            goto fail;
     }
     for( i = 0; i < h->frames.i_max_dpb; i++ )
     {
         h->frames.reference[i] = x264_frame_new( h );
         if( !h->frames.reference[i] )
-            return NULL;
+            goto fail;
     }
     h->frames.reference[h->frames.i_max_dpb] = NULL;
     h->frames.i_last_idr = - h->param.i_keyint_max;
@@ -579,7 +621,7 @@
     h->fdec = h->frames.reference[0];
 
     if( x264_macroblock_cache_init( h ) < 0 )
-        return NULL;
+        goto fail;
     x264_rdo_init( );
 
     /* init CPU functions */
@@ -601,7 +643,7 @@
 
     /* rate control */
     if( x264_ratecontrol_new( h ) < 0 )
-        return NULL;
+        goto fail;
 
     x264_log( h, X264_LOG_INFO, "using cpu capabilities %s%s%s%s%s%s\n",
              param->cpu&X264_CPU_MMX ? "MMX " : "",
@@ -614,7 +656,7 @@
     h->thread[0] = h;
     h->i_thread_num = 0;
     for( i = 1; i < param->i_threads; i++ )
-        h->thread[i] = x264_malloc( sizeof(x264_t) );
+        CHECKED_MALLOC( h->thread[i], sizeof(x264_t) );
 
 #ifdef DEBUG_DUMP_FRAME
     {
@@ -632,6 +674,10 @@
 #endif
 
     return h;
+
+fail:
+    x264_encoder_delete( h );
+    return NULL;
 }
 
 /****************************************************************************
@@ -1788,34 +1834,6 @@
             x264_log( h, X264_LOG_INFO, "kb/s:%.1f\n", f_bitrate );
     }
 
-    /* frames */
-    for( i = 0; i < X264_BFRAME_MAX + 3; i++ )
-    {
-        if( h->frames.current[i] ) x264_frame_delete( h->frames.current[i] );
-        if( h->frames.next[i] )    x264_frame_delete( h->frames.next[i] );
-        if( h->frames.unused[i] )  x264_frame_delete( h->frames.unused[i] );
-    }
-    /* ref frames */
-    for( i = 0; i < h->frames.i_max_dpb; i++ )
-    {
-        x264_frame_delete( h->frames.reference[i] );
-    }
-
-    /* rc */
-    x264_ratecontrol_delete( h );
-
-    /* param */
-    if( h->param.rc.psz_stat_out )
-        free( h->param.rc.psz_stat_out );
-    if( h->param.rc.psz_stat_in )
-        free( h->param.rc.psz_stat_in );
-    if( h->param.rc.psz_rc_eq )
-        free( h->param.rc.psz_rc_eq );
-
-    x264_macroblock_cache_end( h );
-    x264_free( h->out.p_bitstream );
-    for( i = 1; i < h->param.i_threads; i++ )
-        x264_free( h->thread[i] );
-    x264_free( h );
+    x264_encoder_delete( h );
 }
 
Index: encoder/ratecontrol.c
===================================================================
--- encoder/ratecontrol.c	(révision 531)
+++ encoder/ratecontrol.c	(copie de travail)
@@ -194,7 +194,8 @@
 
     x264_cpu_restore( h->param.cpu );
 
-    h->rc = rc = x264_malloc( h->param.i_threads * sizeof(x264_ratecontrol_t) );
+    CHECKED_MALLOC( rc, h->param.i_threads * sizeof(x264_ratecontrol_t) );
+    h->rc = rc;
     memset( rc, 0, h->param.i_threads * sizeof(x264_ratecontrol_t) );
 
     rc->b_abr = ( h->param.rc.b_cbr || h->param.rc.i_rf_constant ) && !h->param.rc.b_stat_read;
@@ -215,7 +216,7 @@
     if( h->param.rc.i_rf_constant && h->param.rc.b_stat_read )
     {
         x264_log(h, X264_LOG_ERROR, "constant rate-factor is incompatible with 2pass.\n");
-        return -1;
+        goto fail;
     }
     if( h->param.rc.i_vbv_buffer_size && !h->param.rc.b_cbr && !h->param.rc.i_rf_constant )
         x264_log(h, X264_LOG_WARNING, "VBV is incompatible with constant QP.\n");
@@ -294,7 +295,7 @@
     rc->pred_b_from_p = rc->pred[0];
 
     if( parse_zones( h ) < 0 )
-        return -1;
+        goto fail;
 
     /* Load stat file and init 2pass algo */
     if( h->param.rc.b_stat_read )
@@ -304,10 +305,10 @@
         /* read 1st pass stats */
         assert( h->param.rc.psz_stat_in );
         stats_buf = stats_in = x264_slurp_file( h->param.rc.psz_stat_in );
-        if( !stats_buf )
+        if( NULL == stats_buf )
         {
             x264_log(h, X264_LOG_ERROR, "ratecontrol_init: can't open stats file\n");
-            return -1;
+            goto fail;
         }
 
         /* check whether 1st pass options were compatible with current options */
@@ -317,7 +318,10 @@
             char *opts = stats_buf;
             stats_in = strchr( stats_buf, '\n' );
             if( !stats_in )
-                return -1;
+            {
+                x264_free( stats_buf );
+                goto fail;
+            }
             *stats_in = '\0';
             stats_in++;
 
@@ -326,7 +330,8 @@
             {
                 x264_log( h, X264_LOG_ERROR, "different number of B-frames than 1st pass (%d vs %d)\n",
                           h->param.i_bframe, i );
-                return -1;
+                x264_free( stats_buf );
+                goto fail;
             }
 
             /* since B-adapt doesn't (yet) take into account B-pyramid,
@@ -350,7 +355,8 @@
         if(i==0)
         {
             x264_log(h, X264_LOG_ERROR, "empty stats file\n");
-            return -1;
+            x264_free( stats_buf );
+            goto fail;
         }
         rc->num_entries = i;
 
@@ -363,13 +369,19 @@
         {
             x264_log( h, X264_LOG_ERROR, "2nd pass has more frames than 1st pass (%d vs %d)\n",
                       h->param.i_frame_total, rc->num_entries );
-            return -1;
+            x264_free( stats_buf );
+            goto fail;
         }
 
         /* FIXME: ugly padding because VfW drops delayed B-frames */
         rc->num_entries += h->param.i_bframe;
 
         rc->entry = (ratecontrol_entry_t*) x264_malloc(rc->num_entries * sizeof(ratecontrol_entry_t));
+        if( NULL == rc->entry )
+        {
+            x264_free( stats_buf );
+            goto fail;
+        }
         memset(rc->entry, 0, rc->num_entries * sizeof(ratecontrol_entry_t));
 
         /* init all to skipped p frames */
@@ -401,7 +413,8 @@
             if(frame_number < 0 || frame_number >= rc->num_entries)
             {
                 x264_log(h, X264_LOG_ERROR, "bad frame number (%d) at stats line %d\n", frame_number, i);
-                return -1;
+                x264_free( stats_buf );
+                goto fail;
             }
             rce = &rc->entry[frame_number];
             rce->direct_mode = 0;
@@ -450,15 +463,21 @@
         if( rc->p_stat_file_out == NULL )
         {
             x264_log(h, X264_LOG_ERROR, "ratecontrol_init: can't open stats file\n");
-            return -1;
+            goto fail;
         }
 
         p = x264_param2string( &h->param, 1 );
+        if ( NULL == p )
+            goto fail;
         fprintf( rc->p_stat_file_out, "#options: %s\n", p );
         x264_free( p );
     }
 
     return 0;
+    
+fail:
+    x264_ratecontrol_delete( h );
+    return -1;
 }
 
 static int parse_zones( x264_t *h )
@@ -512,6 +531,8 @@
 
         rc->i_zones = h->param.rc.i_zones;
         rc->zones = x264_malloc( rc->i_zones * sizeof(x264_zone_t) );
+        if ( NULL == rc->zones )
+            return -1;
         memcpy( rc->zones, h->param.rc.zones, rc->i_zones * sizeof(x264_zone_t) );
     }
 
@@ -534,6 +555,8 @@
 {
     x264_ratecontrol_t *rc = h->rc;
 
+    if( NULL == rc ) return;
+
     if( rc->p_stat_file_out )
     {
         fclose( rc->p_stat_file_out );
@@ -543,8 +566,8 @@
                 x264_log( h, X264_LOG_ERROR, "failed to rename \"%s\" to \"%s\"\n",
                           rc->psz_stat_file_tmpname, h->param.rc.psz_stat_out );
             }
-        x264_free( rc->psz_stat_file_tmpname );
     }
+    x264_free( rc->psz_stat_file_tmpname );
     x264_free( rc->entry );
     x264_free( rc->zones );
     x264_free( rc );
Index: common/macroblock.c
===================================================================
--- common/macroblock.c	(révision 531)
+++ common/macroblock.c	(copie de travail)
@@ -863,23 +863,21 @@
     memset( h->mb.cache.ref[1], -2, X264_SCAN8_SIZE * sizeof( int8_t ) );
 
     return 0;
-fail: return -1;
+fail:
+    x264_macroblock_cache_end( h );
+    return -1;
 }
 void x264_macroblock_cache_end( x264_t *h )
 {
     int i, j;
     for( i=0; i<2; i++ )
     {
-        int i_refs = i ? 1 + h->param.b_bframe_pyramid : h->param.i_frame_reference;
-        for( j=0; j < i_refs; j++ )
+        for( j=0; j < 16; j++ )
             x264_free( h->mb.mvr[i][j] );
     }
-    if( h->param.b_cabac )
-    {
-        x264_free( h->mb.chroma_pred_mode );
-        x264_free( h->mb.mvd[0] );
-        x264_free( h->mb.mvd[1] );
-    }
+    x264_free( h->mb.chroma_pred_mode );
+    x264_free( h->mb.mvd[0] );
+    x264_free( h->mb.mvd[1] );
     x264_free( h->mb.intra4x4_pred_mode );
     x264_free( h->mb.non_zero_count );
     x264_free( h->mb.mb_transform_size );
Index: common/common.c
===================================================================
--- common/common.c	(révision 531)
+++ common/common.c	(copie de travail)
@@ -424,7 +424,7 @@
 {
     int b_error = 0;
     int i_size;
-    char *buf;
+    char *buf = NULL;
     FILE *fh = fopen( filename, "rb" );
     if( !fh )
         return NULL;
@@ -432,21 +432,23 @@
     b_error |= ( i_size = ftell( fh ) ) <= 0;
     b_error |= fseek( fh, 0, SEEK_SET ) < 0;
     if( b_error )
-        return NULL;
+        goto fail;
     buf = x264_malloc( i_size+2 );
-    if( buf == NULL )
-        return NULL;
+    if( NULL == buf )
+        goto fail;
     b_error |= fread( buf, 1, i_size, fh ) != i_size;
     if( buf[i_size-1] != '\n' )
         buf[i_size++] = '\n';
     buf[i_size] = 0;
     fclose( fh );
-    if( b_error )
+    if( !b_error )
     {
-        x264_free( buf );
-        return NULL;
+        return buf;
     }
-    return buf;
+fail:
+    x264_free( buf );
+    fclose( fh );
+    return NULL;
 }
 
 /****************************************************************************
@@ -457,6 +459,8 @@
     char *buf = x264_malloc( 1000 );
     char *s = buf;
 
+    if ( NULL == buf ) return NULL;
+
     if( b_res )
     {
         s += sprintf( s, "%dx%d ", p->i_width, p->i_height );
Index: common/set.c
===================================================================
--- common/set.c	(révision 531)
+++ common/set.c	(copie de travail)
@@ -170,7 +170,7 @@
     h->param.i_cqm_preset = X264_CQM_CUSTOM;
     
     buf = x264_slurp_file( filename );
-    if( !buf )
+    if( NULL == buf )
     {
         x264_log( h, X264_LOG_ERROR, "can't open file '%s'\n", filename );
         return -1;
Index: common/common.h
===================================================================
--- common/common.h	(révision 531)
+++ common/common.h	(copie de travail)
@@ -78,14 +78,14 @@
 #endif
 
 #define CHECKED_MALLOC( var, size )\
-{\
+do {\
     var = x264_malloc( size );\
     if( !var )\
     {\
         x264_log( h, X264_LOG_ERROR, "malloc failed\n" );\
         goto fail;\
     }\
-}
+} while( 0 )
 
 #define X264_BFRAME_MAX 16
 #define X264_SLICE_MAX 4
@@ -263,6 +263,7 @@
     /* encoder parameters */
     x264_param_t    param;
 
+#define X264_THREAD_MAX X264_SLICE_MAX
     x264_t *thread[X264_SLICE_MAX];
 
     /* bitstream output */
@@ -324,7 +325,8 @@
         x264_frame_t *last_nonb;
 
         /* frames used for reference +1 for decoding + sentinels */
-        x264_frame_t *reference[16+2+1+2];
+#define X264_MAX_REFERENCES (16+2+1+2)
+        x264_frame_t *reference[X264_MAX_REFERENCES];
 
         int i_last_idr; /* Frame number of the last IDR */
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.videolan.org/pipermail/x264-devel/attachments/20060605/f9f7bf26/attachment.pgp 


More information about the x264-devel mailing list