[Cluster-devel] [PATCH] mkfs.gfs2: Check locktable more strictly for valid chars

[Cluster-devel] [PATCH] mkfs.gfs2: Check locktable more strictly for valid chars

[Andrew Price]

Previously mkfs.gfs2 allowed any printable characters to be used in the
locktable name. This causes problems when the locktable contains special
characters such as '/'. This patch limits the locktable to alphanumeric
characters plus hyphens and underscores.

Ref: bz#862847

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/man/mkfs.gfs2.8  | 3 ++-
 gfs2/mkfs/main_mkfs.c | 6 ++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/gfs2/man/mkfs.gfs2.8 b/gfs2/man/mkfs.gfs2.8
index d17e272..4613305 100644
--- a/gfs2/man/mkfs.gfs2.8
+++ b/gfs2/man/mkfs.gfs2.8
@@ -73,7 +73,8 @@ Clustername must match that in cluster.conf; only members of this
 cluster are permitted to use this file system.
 Fsname is a unique file system name used to distinguish this GFS2 file
 system from others created (1 to 16 characters).  Lock_nolock doesn't
-use this field.
+use this field. Valid \fIclustername\fRs and \fIfsname\fRs may only contain
+alphanumeric characters, hyphens (-) and underscores (_).
 .TP
 \fB-V\fP
 Print program version information, then exit.
diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 2d529d7..3bbb5b4 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -259,10 +259,8 @@ static void test_locking(char *lockproto, char *locktable)
 			exit(-1);
 		}
 		for (c = locktable; *c; c++) {
-			if (isspace(*c))
-				die( _("locktable error: contains space characters\n"));
-			if (!isprint(*c))
-				die( _("locktable error: contains unprintable characters\n"));
+			if (!isalnum(*c) && (*c != '-') && (*c != '_') && (*c != ':'))
+				die( _("locktable error: invalid character '%c'\n"), *c);
 		}
 
 		c = strstr(locktable, ":");
-- 
1.7.11.4

[Cluster-devel] [PATCH] mkfs.gfs2: Check locktable more strictly for valid chars

[Steven Whitehouse]

Hi,

Looks good. Should we also be limiting the number of : to just one I
wonder?

Steve.

On Fri, 2012-10-12 at 15:37 +0100, Andrew Price wrote:
> Previously mkfs.gfs2 allowed any printable characters to be used in the
> locktable name. This causes problems when the locktable contains special
> characters such as '/'. This patch limits the locktable to alphanumeric
> characters plus hyphens and underscores.
> 
> Ref: bz#862847
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  gfs2/man/mkfs.gfs2.8  | 3 ++-
>  gfs2/mkfs/main_mkfs.c | 6 ++----
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/gfs2/man/mkfs.gfs2.8 b/gfs2/man/mkfs.gfs2.8
> index d17e272..4613305 100644
> --- a/gfs2/man/mkfs.gfs2.8
> +++ b/gfs2/man/mkfs.gfs2.8
> @@ -73,7 +73,8 @@ Clustername must match that in cluster.conf; only members of this
>  cluster are permitted to use this file system.
>  Fsname is a unique file system name used to distinguish this GFS2 file
>  system from others created (1 to 16 characters).  Lock_nolock doesn't
> -use this field.
> +use this field. Valid \fIclustername\fRs and \fIfsname\fRs may only contain
> +alphanumeric characters, hyphens (-) and underscores (_).
>  .TP
>  \fB-V\fP
>  Print program version information, then exit.
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index 2d529d7..3bbb5b4 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -259,10 +259,8 @@ static void test_locking(char *lockproto, char *locktable)
>  			exit(-1);
>  		}
>  		for (c = locktable; *c; c++) {
> -			if (isspace(*c))
> -				die( _("locktable error: contains space characters\n"));
> -			if (!isprint(*c))
> -				die( _("locktable error: contains unprintable characters\n"));
> +			if (!isalnum(*c) && (*c != '-') && (*c != '_') && (*c != ':'))
> +				die( _("locktable error: invalid character '%c'\n"), *c);
>  		}
>  
>  		c = strstr(locktable, ":");

[Cluster-devel] [PATCH] mkfs.gfs2: Check locktable more strictly for valid chars

[Andrew Price]

On 15/10/12 10:46, Steven Whitehouse wrote:
> Hi,
>
> Looks good. Should we also be limiting the number of : to just one I
> wonder?

That's already taken care of with a check a bit further down in that 
function:

         if (strstr(c, ":"))
                 die( _("locktable error: more than one colon present\n"));

Andy

>
> Steve.
>
> On Fri, 2012-10-12 at 15:37 +0100, Andrew Price wrote:
>> Previously mkfs.gfs2 allowed any printable characters to be used in the
>> locktable name. This causes problems when the locktable contains special
>> characters such as '/'. This patch limits the locktable to alphanumeric
>> characters plus hyphens and underscores.
>>
>> Ref: bz#862847
>>
>> Signed-off-by: Andrew Price <anprice@redhat.com>
>> ---
>>   gfs2/man/mkfs.gfs2.8  | 3 ++-
>>   gfs2/mkfs/main_mkfs.c | 6 ++----
>>   2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/gfs2/man/mkfs.gfs2.8 b/gfs2/man/mkfs.gfs2.8
>> index d17e272..4613305 100644
>> --- a/gfs2/man/mkfs.gfs2.8
>> +++ b/gfs2/man/mkfs.gfs2.8
>> @@ -73,7 +73,8 @@ Clustername must match that in cluster.conf; only members of this
>>   cluster are permitted to use this file system.
>>   Fsname is a unique file system name used to distinguish this GFS2 file
>>   system from others created (1 to 16 characters).  Lock_nolock doesn't
>> -use this field.
>> +use this field. Valid \fIclustername\fRs and \fIfsname\fRs may only contain
>> +alphanumeric characters, hyphens (-) and underscores (_).
>>   .TP
>>   \fB-V\fP
>>   Print program version information, then exit.
>> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
>> index 2d529d7..3bbb5b4 100644
>> --- a/gfs2/mkfs/main_mkfs.c
>> +++ b/gfs2/mkfs/main_mkfs.c
>> @@ -259,10 +259,8 @@ static void test_locking(char *lockproto, char *locktable)
>>   			exit(-1);
>>   		}
>>   		for (c = locktable; *c; c++) {
>> -			if (isspace(*c))
>> -				die( _("locktable error: contains space characters\n"));
>> -			if (!isprint(*c))
>> -				die( _("locktable error: contains unprintable characters\n"));
>> +			if (!isalnum(*c) && (*c != '-') && (*c != '_') && (*c != ':'))
>> +				die( _("locktable error: invalid character '%c'\n"), *c);
>>   		}
>>
>>   		c = strstr(locktable, ":");
>
>