[PATCH] xfsdump: fix bug restricting -f pathname length

[PATCH] xfsdump: fix bug restricting -f pathname length

[Bill Kendall]

A temporary buffer storing the dump filename (-f) is smaller than
the otherwise allowed pathname length. This buffer is used to
extract the pathname from additional parameters specified as part
of the -f argument. As these parameters are an undocumented and
unused extension of the -f argument, simply remove this code.
Note that drive_alloc() always dups the given pathname, so
'optarg' is passed directly.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/drive.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/common/drive.c b/common/drive.c
index f184afa..227fecd 100644
--- a/common/drive.c
+++ b/common/drive.c
@@ -137,10 +137,6 @@ drive_init1( int argc, char *argv[ ], bool_t singlethreaded )
 	opterr = 0;
 	driveix = 0;
 	while ( ( c = getopt( argc, argv, GETOPT_CMDSTRING )) != EOF ) {
-		char optarray[100];
-		char *devname;
-		char *token;
-
 		switch ( c ) {
 		case GETOPT_DUMPDEST:
 			if ( ! optarg || optarg[ 0 ] == '-' ) {
@@ -151,21 +147,9 @@ drive_init1( int argc, char *argv[ ], bool_t singlethreaded )
 				return BOOL_FALSE;
 			}
 
-			/* remove the device name from the rest of the
-			 * parameter string. note that strdup malloc()s
-			 * a string; important since optarray is an auto.
-			 */
-			ASSERT( strlen( optarg ) < sizeof( optarray ));
-			strncpy( optarray, optarg, sizeof( optarray ));
-			optarray[ sizeof( optarray ) - 1 ] = 0;
-			if ( ( token = strtok( optarray, "," )) == NULL ) {
-				token = optarray;
-			}
-			devname = strdup( token );
-
 			/* allocate a drive descriptor
 			 */
-			drivepp[ driveix ] = drive_alloc( devname, driveix );
+			drivepp[ driveix ] = drive_alloc( optarg, driveix );
 			driveix++;
 			break;
 		}
-- 
1.7.0.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: fix bug restricting -f pathname length

[Alex Elder]

On Thu, 2011-02-24 at 12:40 -0600, Bill Kendall wrote:
> A temporary buffer storing the dump filename (-f) is smaller than
> the otherwise allowed pathname length. This buffer is used to
> extract the pathname from additional parameters specified as part
> of the -f argument. As these parameters are an undocumented and
> unused extension of the -f argument, simply remove this code.
> Note that drive_alloc() always dups the given pathname, so
> 'optarg' is passed directly.

If someone happens to put something starting with a comma
after the device path name, will it simply fail because
a device by that name (comma included) won't exist?

Either way, this looks good.  I'll hold off a bit before
taking this in, in case someone else wants a chance to
review.

Reviewed-by: Alex Elder <aelder@sgi.com>

> Signed-off-by: Bill Kendall <wkendall@sgi.com>
> ---
>  common/drive.c |   18 +-----------------
>  1 files changed, 1 insertions(+), 17 deletions(-)
> 
> diff --git a/common/drive.c b/common/drive.c
> index f184afa..227fecd 100644
> --- a/common/drive.c
> +++ b/common/drive.c
> @@ -137,10 +137,6 @@ drive_init1( int argc, char *argv[ ], bool_t singlethreaded )
>  	opterr = 0;
>  	driveix = 0;
>  	while ( ( c = getopt( argc, argv, GETOPT_CMDSTRING )) != EOF ) {
> -		char optarray[100];
> -		char *devname;
> -		char *token;
> -
>  		switch ( c ) {
>  		case GETOPT_DUMPDEST:
>  			if ( ! optarg || optarg[ 0 ] == '-' ) {
> @@ -151,21 +147,9 @@ drive_init1( int argc, char *argv[ ], bool_t singlethreaded )
>  				return BOOL_FALSE;
>  			}
>  
> -			/* remove the device name from the rest of the
> -			 * parameter string. note that strdup malloc()s
> -			 * a string; important since optarray is an auto.
> -			 */
> -			ASSERT( strlen( optarg ) < sizeof( optarray ));
> -			strncpy( optarray, optarg, sizeof( optarray ));
> -			optarray[ sizeof( optarray ) - 1 ] = 0;
> -			if ( ( token = strtok( optarray, "," )) == NULL ) {
> -				token = optarray;
> -			}
> -			devname = strdup( token );
> -
>  			/* allocate a drive descriptor
>  			 */
> -			drivepp[ driveix ] = drive_alloc( devname, driveix );
> +			drivepp[ driveix ] = drive_alloc( optarg, driveix );
>  			driveix++;
>  			break;
>  		}



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: fix bug restricting -f pathname length

[Bill Kendall]

On 02/25/2011 01:27 PM, Alex Elder wrote:
> On Thu, 2011-02-24 at 12:40 -0600, Bill Kendall wrote:
>> A temporary buffer storing the dump filename (-f) is smaller than
>> the otherwise allowed pathname length. This buffer is used to
>> extract the pathname from additional parameters specified as part
>> of the -f argument. As these parameters are an undocumented and
>> unused extension of the -f argument, simply remove this code.
>> Note that drive_alloc() always dups the given pathname, so
>> 'optarg' is passed directly.
>
> If someone happens to put something starting with a comma
> after the device path name, will it simply fail because
> a device by that name (comma included) won't exist?

xfsdump creates the file if it does not exist, so you would
end up with a comma in the dump filename.

Thanks,
Bill

>
> Either way, this looks good.  I'll hold off a bit before
> taking this in, in case someone else wants a chance to
> review.
>
> Reviewed-by: Alex Elder<aelder@sgi.com>
>
>> Signed-off-by: Bill Kendall<wkendall@sgi.com>
>> ---
>>   common/drive.c |   18 +-----------------
>>   1 files changed, 1 insertions(+), 17 deletions(-)
>>
>> diff --git a/common/drive.c b/common/drive.c
>> index f184afa..227fecd 100644
>> --- a/common/drive.c
>> +++ b/common/drive.c
>> @@ -137,10 +137,6 @@ drive_init1( int argc, char *argv[ ], bool_t singlethreaded )
>>   	opterr = 0;
>>   	driveix = 0;
>>   	while ( ( c = getopt( argc, argv, GETOPT_CMDSTRING )) != EOF ) {
>> -		char optarray[100];
>> -		char *devname;
>> -		char *token;
>> -
>>   		switch ( c ) {
>>   		case GETOPT_DUMPDEST:
>>   			if ( ! optarg || optarg[ 0 ] == '-' ) {
>> @@ -151,21 +147,9 @@ drive_init1( int argc, char *argv[ ], bool_t singlethreaded )
>>   				return BOOL_FALSE;
>>   			}
>>
>> -			/* remove the device name from the rest of the
>> -			 * parameter string. note that strdup malloc()s
>> -			 * a string; important since optarray is an auto.
>> -			 */
>> -			ASSERT( strlen( optarg )<  sizeof( optarray ));
>> -			strncpy( optarray, optarg, sizeof( optarray ));
>> -			optarray[ sizeof( optarray ) - 1 ] = 0;
>> -			if ( ( token = strtok( optarray, "," )) == NULL ) {
>> -				token = optarray;
>> -			}
>> -			devname = strdup( token );
>> -
>>   			/* allocate a drive descriptor
>>   			 */
>> -			drivepp[ driveix ] = drive_alloc( devname, driveix );
>> +			drivepp[ driveix ] = drive_alloc( optarg, driveix );
>>   			driveix++;
>>   			break;
>>   		}
>
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs