2009-06-03 19:44:44

by Jeff Layton

Subject: [PATCH] nfs-utils: fix endptr check in closeall (try #2)

The closeall function is broken in such a way that it almost never
closes any file descriptors. It's calling strtol on the text
representation of the file descriptor, and then checking to see if the
value of *endptr is not '\0' before trying to close the file. This check
is wrong.

When strtol returns an endptr that points to a NULL byte, that indicates
that the conversion was completely successful. I believe this check
should instead be requiring that endptr is pointing to '\0' before closing
the fd.

Also, fix up the function to check for conversion errors from strtol. If
one occurs, just skip the close on that entry.

Reported-by: Chuck Lever <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
support/nfs/closeall.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/support/nfs/closeall.c b/support/nfs/closeall.c
index cc7fb3b..5fe07de 100644
--- a/support/nfs/closeall.c
+++ b/support/nfs/closeall.c
@@ -7,6 +7,7 @@
#include <unistd.h>
#include <stdlib.h>
#include <dirent.h>
+#include <errno.h>

void
closeall(int min)
@@ -18,8 +19,9 @@ closeall(int min)

while ((d = readdir(dir)) != NULL) {
char *endp;
+ errno = 0;
long n = strtol(d->d_name, &endp, 10);
- if (*endp != '\0' && n >= min && n != dfd)
+ if (!errno && *endp == '\0' && n >= min && n != dfd)
(void) close(n);
}
closedir(dir);
--
1.6.0.6

2009-06-03 20:03:43

by Trond Myklebust

Subject: Re: [PATCH] nfs-utils: fix endptr check in closeall (try #2)

On Wed, 2009-06-03 at 15:44 -0400, Jeff Layton wrote:
> The closeall function is broken in such a way that it almost never
> closes any file descriptors. It's calling strtol on the text
> representation of the file descriptor, and then checking to see if the
> value of *endptr is not '\0' before trying to close the file. This check
> is wrong.
>
> When strtol returns an endptr that points to a NULL byte, that indicates
> that the conversion was completely successful. I believe this check
> should instead be requiring that endptr is pointing to '\0' before closing
> the fd.
>
> Also, fix up the function to check for conversion errors from strtol. If
> one occurs, just skip the close on that entry.
>
> Reported-by: Chuck Lever <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> support/nfs/closeall.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/support/nfs/closeall.c b/support/nfs/closeall.c
> index cc7fb3b..5fe07de 100644
> --- a/support/nfs/closeall.c
> +++ b/support/nfs/closeall.c
> @@ -7,6 +7,7 @@
> #include <unistd.h>
> #include <stdlib.h>
> #include <dirent.h>
> +#include <errno.h>
>
> void
> closeall(int min)
> @@ -18,8 +19,9 @@ closeall(int min)
>
> while ((d = readdir(dir)) != NULL) {
> char *endp;
> + errno = 0;
> long n = strtol(d->d_name, &endp, 10);
> - if (*endp != '\0' && n >= min && n != dfd)
> + if (!errno && *endp == '\0' && n >= min && n != dfd)
> (void) close(n);
> }
> closedir(dir);

Shouldn't you also check the case endp == d->d_name? I don't think that
actually sets errno...

Cheers
Trond

2009-06-03 20:13:30

by Jeff Layton

Subject: Re: [PATCH] nfs-utils: fix endptr check in closeall (try #2)

On Wed, 03 Jun 2009 16:03:41 -0400
Trond Myklebust <[email protected]> wrote:

> On Wed, 2009-06-03 at 15:44 -0400, Jeff Layton wrote:
> > The closeall function is broken in such a way that it almost never
> > closes any file descriptors. It's calling strtol on the text
> > representation of the file descriptor, and then checking to see if the
> > value of *endptr is not '\0' before trying to close the file. This check
> > is wrong.
> >
> > When strtol returns an endptr that points to a NULL byte, that indicates
> > that the conversion was completely successful. I believe this check
> > should instead be requiring that endptr is pointing to '\0' before closing
> > the fd.
> >
> > Also, fix up the function to check for conversion errors from strtol. If
> > one occurs, just skip the close on that entry.
> >
> > Reported-by: Chuck Lever <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > support/nfs/closeall.c | 4 +++-
> > 1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/support/nfs/closeall.c b/support/nfs/closeall.c
> > index cc7fb3b..5fe07de 100644
> > --- a/support/nfs/closeall.c
> > +++ b/support/nfs/closeall.c
> > @@ -7,6 +7,7 @@
> > #include <unistd.h>
> > #include <stdlib.h>
> > #include <dirent.h>
> > +#include <errno.h>
> >
> > void
> > closeall(int min)
> > @@ -18,8 +19,9 @@ closeall(int min)
> >
> > while ((d = readdir(dir)) != NULL) {
> > char *endp;
> > + errno = 0;
> > long n = strtol(d->d_name, &endp, 10);
> > - if (*endp != '\0' && n >= min && n != dfd)
> > + if (!errno && *endp == '\0' && n >= min && n != dfd)
> > (void) close(n);
> > }
> > closedir(dir);
>
> Shouldn't you also check the case endp == d->d_name? I don't think that
> actually sets errno...
>

Hmm...for that to be a problem, readdir would have to return a name of
"" (i.e. d->d_name would have to point to a NULL byte). Is that a
possibility here? If not, then the *endp == '\0' check should catch
that case too...

--
Jeff Layton <[email protected]>

2009-06-03 20:40:22

by Trond Myklebust

Subject: Re: [PATCH] nfs-utils: fix endptr check in closeall (try #2)

On Wed, 2009-06-03 at 16:13 -0400, Jeff Layton wrote:
> On Wed, 03 Jun 2009 16:03:41 -0400
> Trond Myklebust <[email protected]> wrote:
>
> > On Wed, 2009-06-03 at 15:44 -0400, Jeff Layton wrote:
> > > The closeall function is broken in such a way that it almost never
> > > closes any file descriptors. It's calling strtol on the text
> > > representation of the file descriptor, and then checking to see if the
> > > value of *endptr is not '\0' before trying to close the file. This check
> > > is wrong.
> > >
> > > When strtol returns an endptr that points to a NULL byte, that indicates
> > > that the conversion was completely successful. I believe this check
> > > should instead be requiring that endptr is pointing to '\0' before closing
> > > the fd.
> > >
> > > Also, fix up the function to check for conversion errors from strtol. If
> > > one occurs, just skip the close on that entry.
> > >
> > > Reported-by: Chuck Lever <[email protected]>
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > support/nfs/closeall.c | 4 +++-
> > > 1 files changed, 3 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/support/nfs/closeall.c b/support/nfs/closeall.c
> > > index cc7fb3b..5fe07de 100644
> > > --- a/support/nfs/closeall.c
> > > +++ b/support/nfs/closeall.c
> > > @@ -7,6 +7,7 @@
> > > #include <unistd.h>
> > > #include <stdlib.h>
> > > #include <dirent.h>
> > > +#include <errno.h>
> > >
> > > void
> > > closeall(int min)
> > > @@ -18,8 +19,9 @@ closeall(int min)
> > >
> > > while ((d = readdir(dir)) != NULL) {
> > > char *endp;
> > > + errno = 0;
> > > long n = strtol(d->d_name, &endp, 10);
> > > - if (*endp != '\0' && n >= min && n != dfd)
> > > + if (!errno && *endp == '\0' && n >= min && n != dfd)
> > > (void) close(n);
> > > }
> > > closedir(dir);
> >
> > Shouldn't you also check the case endp == d->d_name? I don't think that
> > actually sets errno...
> >
>
> Hmm...for that to be a problem, readdir would have to return a name of
> "" (i.e. d->d_name would have to point to a NULL byte). Is that a
> possibility here? If not, then the *endp == '\0' check should catch
> that case too...

In theory, d->d_name cannot be an empty string.

Then again, if you believe theory, it shouldn't be necessary to check
errno at all since (with exception of '.' and '..', which are caught by
the *endp=='\0' test) the readdir entries in /proc/self/fd should always
convert to positive integer values...

Trond