[PATCH 1/2] 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..31401952e 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 ) ) { + free(fname); error_count++; mp_msg( MSGT_STREAM,MSGL_V,"[mf] file not found: '%s'\n",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 ); --
If open_mf in demux_open_mf returns an object with zero paths: Return directly without returning a demuxer object. Previously we segfaulted e.g. when trying to access the first path to guess its file type. Signed-off-by: Alexander Strasser <eclipse7@gmx.net> --- libmpdemux/demux_mf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libmpdemux/demux_mf.c b/libmpdemux/demux_mf.c index 1f322cd41..68a51665f 100644 --- a/libmpdemux/demux_mf.c +++ b/libmpdemux/demux_mf.c @@ -126,7 +126,7 @@ static demuxer_t* demux_open_mf(demuxer_t* demuxer){ mf=open_mf(demuxer->stream->url + 5); - if(!mf) return NULL; + if(!mf || mf->nr_of_files == 0) return NULL; if(!mf_type){ char* p=strrchr(mf->names[0],'.'); --
Committed. Alexander On 2021-04-27 09:16 +0200, Alexander Strasser wrote:
If open_mf in demux_open_mf returns an object with zero paths: Return directly without returning a demuxer object.
Previously we segfaulted e.g. when trying to access the first path to guess its file type.
Signed-off-by: Alexander Strasser <eclipse7@gmx.net> --- libmpdemux/demux_mf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libmpdemux/demux_mf.c b/libmpdemux/demux_mf.c index 1f322cd41..68a51665f 100644 --- a/libmpdemux/demux_mf.c +++ b/libmpdemux/demux_mf.c @@ -126,7 +126,7 @@ static demuxer_t* demux_open_mf(demuxer_t* demuxer){
mf=open_mf(demuxer->stream->url + 5); - if(!mf) return NULL; + if(!mf || mf->nr_of_files == 0) return NULL;
if(!mf_type){ char* p=strrchr(mf->names[0],'.'); --
@@ -142,19 +143,23 @@ mf_t* open_mf(char * filename){
while ( error_count < 5 ) { - sprintf( fname,filename,count++ ); + fname = mp_asprintf( filename,count++ );
A new variable local to this block would make it much easier to argue memory correctness. This probably also applies to the other usages of this “fname” variable, but that’s a somewhat separate issue.
On 2021-04-28 21:09 +0200, Reimar Döffinger wrote:
@@ -142,19 +143,23 @@ mf_t* open_mf(char * filename){
while ( error_count < 5 ) { - sprintf( fname,filename,count++ ); + fname = mp_asprintf( filename,count++ );
A new variable local to this block would make it much easier to argue memory correctness. This probably also applies to the other usages of this “fname” variable, but that’s a somewhat separate issue.
Well. Not sure I fully grasp how you reason about it. There's always a "goto exit_mf", which looks like this: exit_mf: free( fname ); return mf; We definitely don't want that to free a needed path and that's why we need fname to be NULL after we went into the printf format loop. The other gotcha is: We detect the end of the sequence by running into errors. Those failed tries should be freed. Maybe my patch could be simplified further by exploiting that, but as it's non-obvious and might make future errors more likely I don't really want to do that. Did I overlook another problem? If you want to review it, I could try to refactor the individual ways to gather the file lists into individual functions. Localizing variables to those functions as much as possible. Shall it be OK to commit this patch anyway? Alexander
On 2021-04-29 18:43 +0200, Alexander Strasser wrote:
On 2021-04-28 21:09 +0200, Reimar Döffinger wrote:
A new variable local to this block would make it much easier to argue memory correctness. This probably also applies to the other usages of this “fname” variable, but that’s a somewhat separate issue.
[...]
If you want to review it, I could try to refactor the individual ways to gather the file lists into individual functions. Localizing variables to those functions as much as possible.
Patch attached. Only lightly tested. First wanted to know if you like it. Greetings, Alexander [...]
On 2021-05-12 21:42 +0200, Alexander Strasser wrote:
On 2021-04-29 18:43 +0200, Alexander Strasser wrote:
On 2021-04-28 21:09 +0200, Reimar Döffinger wrote:
A new variable local to this block would make it much easier to argue memory correctness. This probably also applies to the other usages of this “fname” variable, but that’s a somewhat separate issue.
[...]
If you want to review it, I could try to refactor the individual ways to gather the file lists into individual functions. Localizing variables to those functions as much as possible.
Patch attached. Only lightly tested. First wanted to know if you like it.
Improved version resubmitted in the new patch set. Alexander
On 2021-04-27 09:15 +0200, Alexander Strasser wrote:
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..31401952e 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 ) ) { + free(fname); error_count++; mp_msg( MSGT_STREAM,MSGL_V,"[mf] file not found: '%s'\n",fname );
Commenting on my patch: This free needs to go after the mp_msg. Alexander
} 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 ); --
participants (2)
-
Alexander Strasser -
Reimar Döffinger