| Message ID | 1309617149-3993-1-git-send-email-luk@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(-)