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 <[email protected]>
---
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
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.
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)
On Sat, 2 Jul 2011 16:32:29 +0200 Luk Claes <[email protected]> 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 <[email protected]>
> ---
> 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);
> }
>
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
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
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.
On 07/03/2011 07:04 AM, NeilBrown wrote:
> On Sat, 2 Jul 2011 16:32:29 +0200 Luk Claes <[email protected]> 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 <[email protected]>
>> ---
>> 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
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