[PATCH 1/4] libmpdemux/mf: Replace sprintf by mp_asprintf
It's not a good idea to use sprintf. Especially in this case where the format string is supplied by the user. Signed-off-by: Alexander Strasser <eclipse7@gmx.net> --- libmpdemux/mf.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/libmpdemux/mf.c b/libmpdemux/mf.c index 7cdd533d7..d4f5be98b 100644 --- a/libmpdemux/mf.c +++ b/libmpdemux/mf.c @@ -37,6 +37,7 @@ #include "mp_msg.h" #include "help_mp.h" #include "stream/stream.h" +#include "mp_strings.h" #include "mf.h" @@ -110,10 +111,10 @@ mf_t* open_mf(char * filename){ goto exit_mf; } - fname=malloc( strlen( filename ) + 32 ); - if ( !strchr( filename,'%' ) ) { + fname=malloc( strlen( filename ) + 32 ); + strcpy( fname,filename ); if ( !strchr( filename,'*' ) ) strcat( fname,"*" ); @@ -142,19 +143,23 @@ mf_t* open_mf(char * filename){ while ( error_count < 5 ) { - sprintf( fname,filename,count++ ); + fname = mp_asprintf( filename,count++ ); + if ( stat( fname,&fs ) ) { error_count++; mp_msg( MSGT_STREAM,MSGL_V,"[mf] file not found: '%s'\n",fname ); + free(fname); } else { mf->names=realloc( mf->names,( mf->nr_of_files + 1 ) * sizeof( char* ) ); - mf->names[mf->nr_of_files]=strdup( fname ); + mf->names[mf->nr_of_files]=fname; // mp_msg( MSGT_STREAM,MSGL_V,"[mf] added file %d.: %s\n",mf->nr_of_files,mf->names[mf->nr_of_files] ); mf->nr_of_files++; } + + fname = NULL; } mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] number of files: %d\n",mf->nr_of_files ); --
Make the code more readable; especially regarding the usage of the now often shared local variables. Signed-off-by: Alexander Strasser <eclipse7@gmx.net> --- libmpdemux/mf.c | 93 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 31 deletions(-) diff --git a/libmpdemux/mf.c b/libmpdemux/mf.c index d4f5be98b..7e75b714c 100644 --- a/libmpdemux/mf.c +++ b/libmpdemux/mf.c @@ -46,26 +46,15 @@ int mf_h = 0; //288; double mf_fps = 25.0; char * mf_type = NULL; //"jpg"; -mf_t* open_mf(char * filename){ -#if defined(HAVE_GLOB) || defined(__MINGW32__) - glob_t gg; - struct stat fs; - int i; - char * fname; - mf_t * mf; - int error_count = 0; - int count = 0; - - mf=calloc( 1,sizeof( mf_t ) ); - if( filename[0] == '@' ) - { +static int init_mf_from_list_file(mf_t* mf, const char * filename){ FILE *lst_f=fopen(filename + 1,"r"); if ( lst_f ) { - fname=malloc(PATH_MAX); + char *fname=malloc(PATH_MAX); while ( fgets( fname,PATH_MAX,lst_f ) ) { + struct stat fs; /* remove spaces from end of fname */ char *t=fname + strlen( fname ) - 1; while ( t > fname && isspace( *t ) ) *(t--)=0; @@ -83,17 +72,21 @@ mf_t* open_mf(char * filename){ fclose( lst_f ); mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] number of files: %d\n",mf->nr_of_files ); - goto exit_mf; + free( fname ); + return 1; } mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] %s is not indirect filelist\n",filename+1 ); - } + return 0; +} - if( strchr( filename,',') ) - { + +static int init_mf_from_comma_delimited_paths(mf_t* mf, char * filename){ + char * fname; mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] filelist: %s\n",filename ); while ( ( fname=strsep( &filename,"," ) ) ) { + struct stat fs; if ( stat( fname,&fs ) ) { mp_msg( MSGT_STREAM,MSGL_V,"[mf] file not found: '%s'\n",fname ); @@ -107,13 +100,14 @@ mf_t* open_mf(char * filename){ } } mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] number of files: %d\n",mf->nr_of_files ); + return 1; +} - goto exit_mf; - } - if ( !strchr( filename,'%' ) ) - { - fname=malloc( strlen( filename ) + 32 ); +static int init_mf_from_glob_pattern(mf_t* mf, const char * filename){ + glob_t gg; + char *fname=malloc( strlen( filename ) + 32 ); + int i; strcpy( fname,filename ); if ( !strchr( filename,'*' ) ) strcat( fname,"*" ); @@ -121,7 +115,7 @@ mf_t* open_mf(char * filename){ mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] search expr: %s\n",fname ); if ( glob( fname,0,NULL,&gg ) ) - { free( mf ); free( fname ); return NULL; } + { free( fname ); return 0; } mf->nr_of_files=gg.gl_pathc; mf->names=calloc( gg.gl_pathc, sizeof( char* ) ); @@ -130,20 +124,26 @@ mf_t* open_mf(char * filename){ for( i=0;i < gg.gl_pathc;i++ ) { + struct stat fs; if (stat( gg.gl_pathv[i],&fs ) == -1) continue; if( S_ISDIR( fs.st_mode ) ) continue; mf->names[i]=strdup( gg.gl_pathv[i] ); // mp_msg( MSGT_STREAM,MSGL_DBG2,"[mf] added file %d.: %s\n",i,mf->names[i] ); } + free( fname ); globfree( &gg ); - goto exit_mf; - } + return 1; +} + +static int init_mf_from_printf_format(mf_t* mf, const char * filename){ + int count = 0, error_count = 0; mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] search expr: %s\n",filename ); while ( error_count < 5 ) { - fname = mp_asprintf( filename,count++ ); + struct stat fs; + char *fname = mp_asprintf( filename,count++ ); if ( stat( fname,&fs ) ) { @@ -158,14 +158,45 @@ mf_t* open_mf(char * filename){ // mp_msg( MSGT_STREAM,MSGL_V,"[mf] added file %d.: %s\n",mf->nr_of_files,mf->names[mf->nr_of_files] ); mf->nr_of_files++; } - - fname = NULL; } mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] number of files: %d\n",mf->nr_of_files ); + return 1; +} + + +mf_t* open_mf(char * filename){ +#if defined(HAVE_GLOB) || defined(__MINGW32__) + mf_t * mf; + int init_success = 0; + + mf=calloc( 1,sizeof( mf_t ) ); + + if( filename[0] == '@' ) + { + init_success = init_mf_from_list_file(mf, filename); + } + + /* only try one more init method depending on filename */ + if( !init_success && strchr( filename,',') ) + { + init_success = init_mf_from_comma_delimited_paths(mf, filename); + } + else if ( !init_success && !strchr( filename,'%' ) ) + { + init_success = init_mf_from_glob_pattern(mf, filename); + } + else if ( !init_success ) + { + init_success = init_mf_from_printf_format(mf, filename); + } + + if (!init_success) + { + free(mf); + return NULL; + } -exit_mf: - free( fname ); return mf; #else mp_msg(MSGT_STREAM,MSGL_FATAL,"[mf] mf support is disabled on your os\n"); --
On 2021-06-01 22:05 +0200, Alexander Strasser wrote: [...]
@@ -158,14 +158,45 @@ mf_t* open_mf(char * filename){ // mp_msg( MSGT_STREAM,MSGL_V,"[mf] added file %d.: %s\n",mf->nr_of_files,mf->names[mf->nr_of_files] ); mf->nr_of_files++; } - - fname = NULL; }
mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] number of files: %d\n",mf->nr_of_files ); + return 1; +} + + +mf_t* open_mf(char * filename){ +#if defined(HAVE_GLOB) || defined(__MINGW32__) + mf_t * mf; + int init_success = 0; + + mf=calloc( 1,sizeof( mf_t ) ); + + if( filename[0] == '@' ) + { + init_success = init_mf_from_list_file(mf, filename); + }
+ /* only try one more init method depending on filename */
Despite this comment the following wasn't really understand:
+ if( !init_success && strchr( filename,',') ) + { + init_success = init_mf_from_comma_delimited_paths(mf, filename); + } + else if ( !init_success && !strchr( filename,'%' ) ) + { + init_success = init_mf_from_glob_pattern(mf, filename); + } + else if ( !init_success ) + { + init_success = init_mf_from_printf_format(mf, filename); + }
I applied this fixup locally to make it more readable (amended patch attached): --->8--- --- a/libmpdemux/mf.c +++ b/libmpdemux/mf.c @@ -197,19 +197,21 @@ mf_t* open_mf(char * filename){ init_success = init_mf_from_list_file(mf, filename); } - /* only try one more init method depending on filename */ - if( !init_success && strchr( filename,',') ) + if( !init_success ) + { + if( strchr( filename,',') ) { init_success = init_mf_from_comma_delimited_paths(mf, filename); } - else if ( !init_success && !strchr( filename,'%' ) ) + else if ( !strchr( filename,'%' ) ) { init_success = init_mf_from_glob_pattern(mf, filename); } - else if ( !init_success ) + else { init_success = init_mf_from_printf_format(mf, filename); } + } if (!init_success) { ---8<--- I want to apply the whole patch set without much of a warning soon-ish. If there are any more comments, now would be a good time. Thanks, Alexander
+ + if (!init_success) + { + free(mf); + return NULL; + }
-exit_mf: - free( fname ); return mf; #else mp_msg(MSGT_STREAM,MSGL_FATAL,"[mf] mf support is disabled on your os\n");
On 22 Sep 2021, at 21:54, Alexander Strasser <eclipse7@gmx.net> wrote:
- /* only try one more init method depending on filename */ - if( !init_success && strchr( filename,',') ) + if( !init_success ) + { + if( strchr( filename,',') ) { init_success = init_mf_from_comma_delimited_paths(mf, filename); } - else if ( !init_success && !strchr( filename,'%' ) ) + else if ( !strchr( filename,'%' ) ) { init_success = init_mf_from_glob_pattern(mf, filename); } - else if ( !init_success ) + else { init_success = init_mf_from_printf_format(mf, filename); } + }
Possibly minor simplification/consistency change: Flip the last 2 cases to remove the negation for the second strchr
On 2021-09-26 20:10 +0200, Reimar Döffinger wrote:
On 22 Sep 2021, at 21:54, Alexander Strasser <eclipse7@gmx.net> wrote:
- /* only try one more init method depending on filename */ - if( !init_success && strchr( filename,',') ) + if( !init_success ) + { + if( strchr( filename,',') ) { init_success = init_mf_from_comma_delimited_paths(mf, filename); } - else if ( !init_success && !strchr( filename,'%' ) ) + else if ( !strchr( filename,'%' ) ) { init_success = init_mf_from_glob_pattern(mf, filename); } - else if ( !init_success ) + else { init_success = init_mf_from_printf_format(mf, filename); } + }
Possibly minor simplification/consistency change: Flip the last 2 cases to remove the negation for the second strchr
Committed Patch 1 and 2 (including Reimar's suggested change) as MPlayer SVN r38340 and r38341 respectively. Alexander
Additionally detect if the number of matches of glob was bigger than what fits into signed int and fail in that case. Signed-off-by: Alexander Strasser <eclipse7@gmx.net> --- libmpdemux/mf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libmpdemux/mf.c b/libmpdemux/mf.c index 7e75b714c..89fc2a7a0 100644 --- a/libmpdemux/mf.c +++ b/libmpdemux/mf.c @@ -118,11 +118,18 @@ static int init_mf_from_glob_pattern(mf_t* mf, const char * filename){ { free( fname ); return 0; } mf->nr_of_files=gg.gl_pathc; + if (mf->nr_of_files < 0) + { + mf->nr_of_files = 0; + free( fname ); + globfree( &gg ); + return 0; + } mf->names=calloc( gg.gl_pathc, sizeof( char* ) ); mp_msg( MSGT_STREAM, MSGL_INFO, "[mf] number of files: %d (%zu)\n", mf->nr_of_files, gg.gl_pathc * sizeof( char* ) ); - for( i=0;i < gg.gl_pathc;i++ ) + for( i=0;i < mf->nr_of_files;i++ ) { struct stat fs; if (stat( gg.gl_pathv[i],&fs ) == -1) continue; --
The strsep function modifies its parameter and we need to store our own copy in mf->names anyway. It's also not in C99. Make the function argument filename `const char *` as it is already in the other init_mf_from_ functions. --- libmpdemux/mf.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/libmpdemux/mf.c b/libmpdemux/mf.c index 89fc2a7a0..eac6053a5 100644 --- a/libmpdemux/mf.c +++ b/libmpdemux/mf.c @@ -16,8 +16,6 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -#define _BSD_SOURCE - #include <ctype.h> #include <stdio.h> #include <stdlib.h> @@ -80,21 +78,36 @@ static int init_mf_from_list_file(mf_t* mf, const char * filename){ } -static int init_mf_from_comma_delimited_paths(mf_t* mf, char * filename){ - char * fname; +static int init_mf_from_comma_delimited_paths(mf_t* mf, const char * filename){ + int finished = 0; + size_t idx = 0; mp_msg( MSGT_STREAM,MSGL_INFO,"[mf] filelist: %s\n",filename ); - while ( ( fname=strsep( &filename,"," ) ) ) + while ( !finished ) { struct stat fs; - if ( stat( fname,&fs ) ) + size_t num_bytes = strcspn( &filename[idx],"," ); + char * fname = calloc( 1,num_bytes+1 ); // always zero-terminated + if ( fname ) + { + strncpy( fname,&filename[idx],num_bytes ); + } + finished = !filename[idx + num_bytes]; + idx += num_bytes + !finished; + + if ( !fname ) + { + mp_msg( MSGT_STREAM,MSGL_WARN,"[mf] allocation for path failed.\n" ); + } + else if ( stat( fname,&fs ) ) { mp_msg( MSGT_STREAM,MSGL_V,"[mf] file not found: '%s'\n",fname ); + free( fname ); } else { mf->names=realloc( mf->names,( mf->nr_of_files + 1 ) * sizeof( char* ) ); - mf->names[mf->nr_of_files]=strdup( fname ); + mf->names[mf->nr_of_files]=fname; // mp_msg( MSGT_STREAM,MSGL_V,"[mf] added file %d.: %s\n",mf->nr_of_files,mf->names[mf->nr_of_files] ); mf->nr_of_files++; } --
participants (2)
-
Alexander Strasser -
Reimar Döffinger