[PATCH] Do not segfault because of kernel version

[PATCH] Do not segfault because of kernel version

[Luk Claes]

mount.nfs segfaults if kernel version number does not contain
at least 3 components delimited with a dot.

Avoid this by matching up to three unsigned integers inialised
to zero, separated by dots.

Signed-off-by: Luk Claes <luk@debian.org>
---
 utils/mount/version.h |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/utils/mount/version.h b/utils/mount/version.h
index af61a6f..2642eab 100644
--- a/utils/mount/version.h
+++ b/utils/mount/version.h
@@ -23,8 +23,7 @@
 #ifndef _NFS_UTILS_MOUNT_VERSION_H
 #define _NFS_UTILS_MOUNT_VERSION_H
 
-#include <stdlib.h>
-#include <string.h>
+#include <stdio.h>
 
 #include <sys/utsname.h>
 
@@ -37,14 +36,14 @@ static inline unsigned int MAKE_VERSION(unsigned int p, unsigned int q,
 static inline unsigned int linux_version_code(void)
 {
 	struct utsname my_utsname;
-	unsigned int p, q, r;
+	unsigned int p, q = 0, r = 0;
 
 	if (uname(&my_utsname))
 		return 0;
 
-	p = (unsigned int)atoi(strtok(my_utsname.release, "."));
-	q = (unsigned int)atoi(strtok(NULL, "."));
-	r = (unsigned int)atoi(strtok(NULL, "."));
+	if (sscanf(my_utsname.release, "%u.%u.%u", &p, &q, &r) < 1)
+		return 0;
+	
 	return MAKE_VERSION(p, q, r);
 }
 
-- 
1.7.5.4

Re: [PATCH] Do not segfault because of kernel version

[NeilBrown]

On Sat,  2 Jul 2011 16:32:29 +0200 Luk Claes <luk@debian.org> wrote:

> mount.nfs segfaults if kernel version number does not contain
> at least 3 components delimited with a dot.
> 
> Avoid this by matching up to three unsigned integers inialised
> to zero, separated by dots.
> 
> Signed-off-by: Luk Claes <luk@debian.org>
> ---
>  utils/mount/version.h |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/utils/mount/version.h b/utils/mount/version.h
> index af61a6f..2642eab 100644
> --- a/utils/mount/version.h
> +++ b/utils/mount/version.h
> @@ -23,8 +23,7 @@
>  #ifndef _NFS_UTILS_MOUNT_VERSION_H
>  #define _NFS_UTILS_MOUNT_VERSION_H
>  
> -#include <stdlib.h>
> -#include <string.h>
> +#include <stdio.h>
>  
>  #include <sys/utsname.h>
>  
> @@ -37,14 +36,14 @@ static inline unsigned int MAKE_VERSION(unsigned int p, unsigned int q,
>  static inline unsigned int linux_version_code(void)
>  {
>  	struct utsname my_utsname;
> -	unsigned int p, q, r;
> +	unsigned int p, q = 0, r = 0;
>  
>  	if (uname(&my_utsname))
>  		return 0;
>  
> -	p = (unsigned int)atoi(strtok(my_utsname.release, "."));
> -	q = (unsigned int)atoi(strtok(NULL, "."));
> -	r = (unsigned int)atoi(strtok(NULL, "."));
> +	if (sscanf(my_utsname.release, "%u.%u.%u", &p, &q, &r) < 1)
> +		return 0;
> +	

According to Linus, the error case should return a big number, not a small
number.
So if the version number is not recognised - e.g. it was written in Sanskrit
rather than arabic numberals, it is assumed to be a future version, not a
past version.

 https://lkml.org/lkml/2011/6/14/293


NeilBrown

>  	return MAKE_VERSION(p, q, r);
>  }
>  

Re: [PATCH] Do not segfault because of kernel version

[Luk Claes]

On 07/03/2011 07:04 AM, NeilBrown wrote:
> On Sat,  2 Jul 2011 16:32:29 +0200 Luk Claes <luk@debian.org> wrote:
> 
>> mount.nfs segfaults if kernel version number does not contain
>> at least 3 components delimited with a dot.
>>
>> Avoid this by matching up to three unsigned integers inialised
>> to zero, separated by dots.
>>
>> Signed-off-by: Luk Claes <luk@debian.org>
>> ---
>>  utils/mount/version.h |   11 +++++------
>>  1 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/utils/mount/version.h b/utils/mount/version.h
>> index af61a6f..2642eab 100644
>> --- a/utils/mount/version.h
>> +++ b/utils/mount/version.h
>> @@ -23,8 +23,7 @@
>>  #ifndef _NFS_UTILS_MOUNT_VERSION_H
>>  #define _NFS_UTILS_MOUNT_VERSION_H
>>  
>> -#include <stdlib.h>
>> -#include <string.h>
>> +#include <stdio.h>
>>  
>>  #include <sys/utsname.h>
>>  
>> @@ -37,14 +36,14 @@ static inline unsigned int MAKE_VERSION(unsigned int p, unsigned int q,
>>  static inline unsigned int linux_version_code(void)
>>  {
>>  	struct utsname my_utsname;
>> -	unsigned int p, q, r;
>> +	unsigned int p, q = 0, r = 0;
>>  
>>  	if (uname(&my_utsname))
>>  		return 0;
>>  
>> -	p = (unsigned int)atoi(strtok(my_utsname.release, "."));
>> -	q = (unsigned int)atoi(strtok(NULL, "."));
>> -	r = (unsigned int)atoi(strtok(NULL, "."));
>> +	if (sscanf(my_utsname.release, "%u.%u.%u", &p, &q, &r) < 1)
>> +		return 0;
>> +	
> 
> According to Linus, the error case should return a big number, not a small
> number.
> So if the version number is not recognised - e.g. it was written in Sanskrit
> rather than arabic numberals, it is assumed to be a future version, not a
> past version.
> 
>  https://lkml.org/lkml/2011/6/14/293

Ah, not a bad idea. In that case a parse error can be distinguished from
other errors. Would 'return -1' do the expected or will it give a
compiler warning/error we should avoid?

Cheers

Luk

Re: [PATCH] Do not segfault because of kernel version

[Jim Rees]

Luk Claes wrote:

  > So if the version number is not recognised - e.g. it was written in Sanskrit
  > rather than arabic numberals, it is assumed to be a future version, not a
  > past version.
  > 
  >  https://lkml.org/lkml/2011/6/14/293
  
  Ah, not a bad idea. In that case a parse error can be distinguished from
  other errors. Would 'return -1' do the expected or will it give a
  compiler warning/error we should avoid?

You can't return -1 from a function returning unsigned int.  I think you
want to return something like

MAKE_VERSION(9999, 255, 255)

Re: [PATCH] Do not segfault because of kernel version

[Luk Claes]

On 07/03/2011 03:02 PM, Jim Rees wrote:
> Luk Claes wrote:
> 
>   > So if the version number is not recognised - e.g. it was written in Sanskrit
>   > rather than arabic numberals, it is assumed to be a future version, not a
>   > past version.
>   > 
>   >  https://lkml.org/lkml/2011/6/14/293
>   
>   Ah, not a bad idea. In that case a parse error can be distinguished from
>   other errors. Would 'return -1' do the expected or will it give a
>   compiler warning/error we should avoid?
> 
> You can't return -1 from a function returning unsigned int.  I think you
> want to return something like
> 
> MAKE_VERSION(9999, 255, 255)

Would it not be better to return UINT_MAX in that case to avoid having
to change it when version 10000 would be released and to avoid overflows
that could potentially order lower?

Cheers

Luk

Re: [PATCH] Do not segfault because of kernel version

[Jim Rees]

Luk Claes wrote:

  > You can't return -1 from a function returning unsigned int.  I think you
  > want to return something like
  > 
  > MAKE_VERSION(9999, 255, 255)
  
  Would it not be better to return UINT_MAX in that case to avoid having
  to change it when version 10000 would be released and to avoid overflows
  that could potentially order lower?

Maybe.  I wanted the second and third numbers to be the max possible (255).
But of course they will be anyway if you return UINT_MAX and are running on
an architecture that represents ints in two's complement binary.  Which is
the case today, but wasn't there a port of unix to the System 36 at one
time?  Ok, that's just silly.

Yes, just return UINT_MAX.  Fix the other error return too, the one where
uname fails.  And put in a comment if you can briefly summarize Linus's
argument.

Re: [PATCH] Do not segfault because of kernel version

[Luk Claes]

On 07/03/2011 03:26 PM, Jim Rees wrote:
> Luk Claes wrote:
> 
>   > You can't return -1 from a function returning unsigned int.  I think you
>   > want to return something like
>   > 
>   > MAKE_VERSION(9999, 255, 255)
>   
>   Would it not be better to return UINT_MAX in that case to avoid having
>   to change it when version 10000 would be released and to avoid overflows
>   that could potentially order lower?
> 
> Maybe.  I wanted the second and third numbers to be the max possible (255).
> But of course they will be anyway if you return UINT_MAX and are running on
> an architecture that represents ints in two's complement binary.  Which is
> the case today, but wasn't there a port of unix to the System 36 at one
> time?  Ok, that's just silly.
> 
> Yes, just return UINT_MAX.  Fix the other error return too, the one where
> uname fails.  And put in a comment if you can briefly summarize Linus's
> argument.

I thought that a real error like uname failing should still get the
'wrong' return 0, no?

Cheers

Luk

Re: [PATCH] Do not segfault because of kernel version

[Jim Rees]

Luk Claes wrote:

  > Yes, just return UINT_MAX.  Fix the other error return too, the one where
  > uname fails.  And put in a comment if you can briefly summarize Linus's
  > argument.
  
  I thought that a real error like uname failing should still get the
  'wrong' return 0, no?

No.  As I read it, Linus argues that you should only run the backward
compatibility code path when you know you're running an older kernel.  If
you don't know, then you should assume you're running a newer kernel.

Re: [PATCH] Do not segfault because of kernel version

[Luk Claes]

On 07/03/2011 04:11 PM, Jim Rees wrote:
> Luk Claes wrote:
> 
>   > Yes, just return UINT_MAX.  Fix the other error return too, the one where
>   > uname fails.  And put in a comment if you can briefly summarize Linus's
>   > argument.
>   
>   I thought that a real error like uname failing should still get the
>   'wrong' return 0, no?
> 
> No.  As I read it, Linus argues that you should only run the backward
> compatibility code path when you know you're running an older kernel.  If
> you don't know, then you should assume you're running a newer kernel.

So, if uname fails we treat it as a newer kernel, shouldn't we treat
that as an error? So treating it as a special case instead of running
backward compatibility or as a newer kernel?

Cheers

Luk

Re: [PATCH] Do not segfault because of kernel version

[Jim Rees]

Luk Claes wrote:

  On 07/03/2011 04:11 PM, Jim Rees wrote:
  > Luk Claes wrote:
  > 
  >   > Yes, just return UINT_MAX.  Fix the other error return too, the one where
  >   > uname fails.  And put in a comment if you can briefly summarize Linus's
  >   > argument.
  >   
  >   I thought that a real error like uname failing should still get the
  >   'wrong' return 0, no?
  > 
  > No.  As I read it, Linus argues that you should only run the backward
  > compatibility code path when you know you're running an older kernel.  If
  > you don't know, then you should assume you're running a newer kernel.
  
  So, if uname fails we treat it as a newer kernel, shouldn't we treat
  that as an error?

That would seem wrong to me.  The current code does not treat it as an
error, so we're breaking something that used to work.  And failing the mount
because uname won't run seems unreasonable.  That could happen at the worst
possible time, for example on a system where the local disk has become
corrupt and you're fixing it from a rescue CD.

Re: [PATCH] Do not segfault because of kernel version

[Luk Claes]

Resending patch taking into account the remarks of Neil Brown
and Jim Rees.

Changes to previous attempt:

* Using MAX_UINT for versions that do not start with an integer
* Using MAX_UINT when uname does not work

[PATCH] Do not segfault because of kernel version

[Luk Claes]

mount.nfs segfaults if kernel version number does not contain
at least 3 components delimited with a dot.

Avoid this by matching up to three unsigned integers inialised
to zero, separated by dots.

A version that does not start with an integer is probably a future
version where the versioning evolved to another scheme.
Return UINT_MAX which is guaranteed to be higher than existing
versions. This would also make it possible to easily identify
versions that do not start with an integer.

Signed-off-by: Luk Claes <luk@debian.org>
---
 utils/mount/version.h |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/utils/mount/version.h b/utils/mount/version.h
index af61a6f..531cf68 100644
--- a/utils/mount/version.h
+++ b/utils/mount/version.h
@@ -23,8 +23,8 @@
 #ifndef _NFS_UTILS_MOUNT_VERSION_H
 #define _NFS_UTILS_MOUNT_VERSION_H
 
-#include <stdlib.h>
-#include <string.h>
+#include <stdio.h>
+#include <limits.h>
 
 #include <sys/utsname.h>
 
@@ -37,14 +37,16 @@ static inline unsigned int MAKE_VERSION(unsigned int p, unsigned int q,
 static inline unsigned int linux_version_code(void)
 {
 	struct utsname my_utsname;
-	unsigned int p, q, r;
+	unsigned int p, q = 0, r = 0;
 
+	/* UINT_MAX as backward compatibility code should not be run */
 	if (uname(&my_utsname))
-		return 0;
+		return UINT_MAX;
 
-	p = (unsigned int)atoi(strtok(my_utsname.release, "."));
-	q = (unsigned int)atoi(strtok(NULL, "."));
-	r = (unsigned int)atoi(strtok(NULL, "."));
+	/* UINT_MAX as future versions might not start with an integer */
+	if (sscanf(my_utsname.release, "%u.%u.%u", &p, &q, &r) < 1)
+		return UINT_MAX;
+	
 	return MAKE_VERSION(p, q, r);
 }
 
-- 
1.7.5.4

Re: [PATCH] Do not segfault because of kernel version

[Steve Dickson]

On 07/05/2011 01:42 AM, Luk Claes wrote:
> mount.nfs segfaults if kernel version number does not contain
> at least 3 components delimited with a dot.
> 
> Avoid this by matching up to three unsigned integers inialised
> to zero, separated by dots.
> 
> A version that does not start with an integer is probably a future
> version where the versioning evolved to another scheme.
> Return UINT_MAX which is guaranteed to be higher than existing
> versions. This would also make it possible to easily identify
> versions that do not start with an integer.
> 
> Signed-off-by: Luk Claes <luk@debian.org>
Committed....

steved.

> ---
>  utils/mount/version.h |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/utils/mount/version.h b/utils/mount/version.h
> index af61a6f..531cf68 100644
> --- a/utils/mount/version.h
> +++ b/utils/mount/version.h
> @@ -23,8 +23,8 @@
>  #ifndef _NFS_UTILS_MOUNT_VERSION_H
>  #define _NFS_UTILS_MOUNT_VERSION_H
>  
> -#include <stdlib.h>
> -#include <string.h>
> +#include <stdio.h>
> +#include <limits.h>
>  
>  #include <sys/utsname.h>
>  
> @@ -37,14 +37,16 @@ static inline unsigned int MAKE_VERSION(unsigned int p, unsigned int q,
>  static inline unsigned int linux_version_code(void)
>  {
>  	struct utsname my_utsname;
> -	unsigned int p, q, r;
> +	unsigned int p, q = 0, r = 0;
>  
> +	/* UINT_MAX as backward compatibility code should not be run */
>  	if (uname(&my_utsname))
> -		return 0;
> +		return UINT_MAX;
>  
> -	p = (unsigned int)atoi(strtok(my_utsname.release, "."));
> -	q = (unsigned int)atoi(strtok(NULL, "."));
> -	r = (unsigned int)atoi(strtok(NULL, "."));
> +	/* UINT_MAX as future versions might not start with an integer */
> +	if (sscanf(my_utsname.release, "%u.%u.%u", &p, &q, &r) < 1)
> +		return UINT_MAX;
> +	
>  	return MAKE_VERSION(p, q, r);
>  }
>