[x264-devel] Re: [PATCH] malloc result test
Loïc Le Loarer
lll+vlc at m4x.org
Tue Jun 6 08:58:30 CEST 2006
Le Monday 05 June 2006 à 10:48:14 +0200, Loïc Le Loarer a écrit:
> 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
> > :)
>
> 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.
I had forgotten some tests in ratecontrol.c initialisation functions,
here is a more complete patch which replace the previous one completely.
Please, see if it can be applied.
Thank you
--
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;
@@ -442,7 +455,7 @@
{
char *p;
- rc->psz_stat_file_tmpname = x264_malloc( strlen(h->param.rc.psz_stat_out) + 6 );
+ CHECKED_MALLOC( rc->psz_stat_file_tmpname, strlen(h->param.rc.psz_stat_out) + 6 );
strcpy( rc->psz_stat_file_tmpname, h->param.rc.psz_stat_out );
strcat( rc->psz_stat_file_tmpname, ".temp" );
@@ -450,28 +463,35 @@
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 )
{
x264_ratecontrol_t *rc = h->rc;
int i;
+ h->param.rc.zones = NULL;
if( h->param.rc.psz_zones && !h->param.rc.i_zones )
{
char *p;
h->param.rc.i_zones = 1;
for( p = h->param.rc.psz_zones; *p; p++ )
h->param.rc.i_zones += (*p == '/');
- h->param.rc.zones = x264_malloc( h->param.rc.i_zones * sizeof(x264_zone_t) );
+ CHECKED_MALLOC( h->param.rc.zones, h->param.rc.i_zones * sizeof(x264_zone_t) );
p = h->param.rc.psz_zones;
for( i = 0; i < h->param.rc.i_zones; i++)
{
@@ -485,7 +505,7 @@
char *slash = strchr(p, '/');
if(slash) *slash = '\0';
x264_log( h, X264_LOG_ERROR, "invalid zone: \"%s\"\n", p );
- return -1;
+ goto fail;
}
p = strchr(p, '/') + 1;
}
@@ -500,22 +520,25 @@
{
x264_log( h, X264_LOG_ERROR, "invalid zone: start=%d end=%d\n",
z.i_start, z.i_end );
- return -1;
+ goto fail;
}
else if( !z.b_force_qp && z.f_bitrate_factor <= 0 )
{
x264_log( h, X264_LOG_ERROR, "invalid zone: bitrate_factor=%f\n",
z.f_bitrate_factor );
- return -1;
+ goto fail;
}
}
rc->i_zones = h->param.rc.i_zones;
- rc->zones = x264_malloc( rc->i_zones * sizeof(x264_zone_t) );
- memcpy( rc->zones, h->param.rc.zones, rc->i_zones * sizeof(x264_zone_t) );
+ rc->zones = h->param.rc.zones;
+ h->param.rc.zones = NULL;
}
return 0;
+fail:
+ x264_free( h->param.rc.zones );
+ return -1;
}
void x264_ratecontrol_summary( x264_t *h )
@@ -534,6 +557,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 +568,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/20060606/533d64b6/attachment.pgp
More information about the x264-devel
mailing list