Re: Openat without die

[Eric Blake]

On 01/11/2011 10:50 AM, Bastien ROUCARIES wrote:
> 
> I have also corrected a bug openat not testing NULL path. I return EFAULT 
> like my Linux here.

I disagree with this change.  EFAULT is not mandated by POSIX; since
anything you can ever do to trigger an EFAULT (that is, any bad pointer,
not just the NULL pointer) is evidence of a bug in the program.  EFAULT
is nicer than crashing in the kernel, especially when the kernel can do
EFAULT checking for free (it already has to validate all pointers as
part of the translation between user-space and kernel-space operation).
 But since we don't ever pass bad pointers in the first place (right?
:), and we are not in a position to check for all possible bad pointers
in userspace, and our crash would be in user space where the backtrace
is useful, I argue that checking for the special case of a NULL pointer
just wastes time in our openat() emulation.

> 
> 0002-Reject-early-NULL-path-and-empty-path-in-at-function.patch

Generally, a patch titled 0002 implies an 0001; based on how you rebased
things, you may want to regenerate the patch to have a more appropriate
title on the mailing list so we don't go looking for a missing patch
1/n.  [Oh, I see, you attached patches out of order, so that 2 got
listed before 1 - in that case, sending separate emails, one per patch,
using git send-email's threading capabilities, can make things easier to
reply to.]

Also, we prefer titles that hint as to the module involved (do 'git
shortlog -30' to get a feel for the majority of recent patch subject
lines), like this:

openat: reject empty path in at functions

> 
> From 0c638872ae7d33a36c6548c720aa1333b7683510 Mon Sep 17 00:00:00 2001
> From: Bastien ROUCARIES <address@hidden>
> Date: Tue, 11 Jan 2011 18:15:44 +0100
> Subject: [PATCH 2/3] Reject early NULL path and empty path in *at function
> 
> Reject early and set errno when user pass a NULL string or empty string
> as path for *at function.
> 
> Previous version leads to a NULL deference in case of NULL string.

This is useful information for the email review, where we can compare it
to v1 of your patch, but is pointless in the commit log, where once a
final version of your patch is pushed, we no longer care how many
intermediate versions your patch went through to get to the final
version.  Therefore, it belongs...

> ---

after the --- line, so that someone using 'git am' on your message won't
preserver that comment through to the final commit.

[which reminds me - I have a bug report against 'git notes' for not
making it easier to add annotations that will automatically appear after
the --- when using get send-email/format-patch, but I'm not familiar
enough with the git code base to write the patch myself without spending
a lot of time on it, and no one else seems to have jumped in either:
http://thread.gmane.org/gmane.comp.version-control.git/163141]

>  lib/at-func.c     |   14 ++++++++++++++
>  lib/at-func2.c    |   15 ++++++++++++++-
>  lib/openat-proc.c |    7 -------
>  lib/openat.c      |   15 +++++++++++++++
>  4 files changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/at-func.c b/lib/at-func.c
> index 31a75f1..f7e2667 100644
> --- a/lib/at-func.c
> +++ b/lib/at-func.c
> @@ -71,6 +71,20 @@ AT_FUNC_NAME (int fd, char const *file 
> AT_FUNC_POST_FILE_PARAM_DECLS)
>  
>    if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
>      return CALL_FUNC (file);
> +  
> +  /* NULL string are forbidden */
> +  if (file == NULL)
> +    {
> +      errno = EFAULT;
> +      return FUNC_FAIL;
> +    }

So lose this hunk, and let a buggy program crash (you're doing the
programmer a service by crashing, since it is easier to debug a core
dump due to SEGV than it is to figure out why EFAULT was returned).

> +
> +  /* empty string */
> +  if (!*file)
> +    {
> +      errno = ENOENT;
> +      return FUNC_FAIL;
> +    }

We already had empty string checks built in to the proc_buf building
phase, and the testsuite already exercises empty strings to test that
fact, so I'm not sure why you needed to add this hunk.

> +++ b/lib/openat-proc.c
> @@ -64,13 +64,6 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, 
> char const *file)
>  {
>    static int proc_status = 0;
>  
> -  /* Make sure the caller gets ENOENT when appropriate.  */
> -  if (!*file)
> -    {
> -      buf[0] = '\0';
> -      return buf;
> -    }

Oh, you added the empty string check earlier because you are losing it
here in the openat_proc_name location.  I don't see why this code motion
is necessary, without more justification.

> 
> 0001-Avoid-xmalloc-in-openat-implementation.patch
> 
> From 38458bc47a2560f697dbb017cddcaeac32d9bb63 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bastien=20ROUCARI=C3=88S?= <address@hidden>
> Date: Tue, 11 Jan 2011 17:39:12 +0100
> Subject: [PATCH 1/3] Avoid xmalloc in openat implementation

Oh, I see - you attached your patches out of order.  Again, I would have
preferred this subject:

openat: avoid xmalloc

> 
> Do not call xmalloc but return NULL in case of error. Caller are safe
> and fallback to fchdir implementation.

Grammar on the second sentence: "Callers are safe, and fall back to
fchdir implementation."

> +++ b/lib/openat-proc.c
> @@ -28,6 +28,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <stdlib.h>

Great that you added this for malloc(), but you also need to drop
"xalloc.h" for the no-longer-used xmalloc().

>  
>  #include "dirname.h"
>  #include "intprops.h"
> @@ -42,6 +43,11 @@
>  #undef open
>  #undef close
>  
> +/* In this file we always malloc a stricly positive size. 
> +   repl_malloc(size >0) and malloc(size >0) are equivalent 
> +   Use therefore use the system malloc. */
> +#undef malloc

I disagree with this hunk.  One of the reasons that rpl_malloc (not
repl_malloc) might exist is to work around the mingw bug of not setting
errno to ENOMEM on failure (the malloc-posix module); just because
you're avoiding the portability bug of malloc(0) (which means you don't
need the malloc-gnu module) doesn't mean you're avoiding all portability
bugs, so this undef is unwise.  But even if you can convince me to keep
this hunk, s/stricly/strictly/

> @@ -98,7 +104,9 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, 
> char const *file)
>    else
>      {
>        size_t bufsize = PROC_SELF_FD_NAME_SIZE_BOUND (strlen (file));
> -      char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf : xmalloc 
> (bufsize));
> +      char *result = (bufsize < OPENAT_BUFFER_SIZE ? buf : malloc (bufsize));
> +      if (NULL == result)
> +     return NULL;

You used TAB instead of space, which is inconsistent with our style, but
other than that, this hunk is okay.

You're also missing changes to the modules files, to replace xmalloc
with malloc-posix as a required prerequisite module.

> 
> 0003-Bail-out-early-in-case-of-ENOMEM-in-openat_proc_name.patch
> 
> From 8897b4ea3b14cce62493c9f439c841a3f131a6ae Mon Sep 17 00:00:00 2001
> From: Bastien ROUCARIES <address@hidden>
> Date: Tue, 11 Jan 2011 18:32:19 +0100
> Subject: [PATCH 3/3] Bail out early in case of ENOMEM in openat_proc_name
> 
> Return early in caller functions of openat_proc_name in case of unexpected 
> error

openat: return early when error detected

> @@ -89,6 +89,10 @@ AT_FUNC_NAME (int fd, char const *file 
> AT_FUNC_POST_FILE_PARAM_DECLS)
>    {
>      char proc_buf[OPENAT_BUFFER_SIZE];
>      char *proc_file = openat_proc_name (proc_buf, fd, file);
> +
> +    if (!proc_file && errno != ENOTSUP)
> +      return FUNC_FAIL;

This needs more work in openat_proc_name to guarantee a sane errno value
on all possible return paths - right now, it can fail if proc_status ==
-1, but with an unknown errno value due to close(proc_self_fd)
clobbering it on the first time through, and with errno unchanged from
its arbitrary contents in the caller on all subsequent times through.

And, rather than checking != ENOTSUP, it might be safer to check ==
ENOMEM, so that you are minimizing the impact of your change.  The whole
point of patch 3 appears to be avoiding the risk of the fchdir()
fallback on the rare systems where *at is missing and /proc/self/fd/
works, and in the corner case where trying to use /proc/self/fd failed
due to tight memory constraints, but as written, it avoids the fchdir()
fallback even for non-memory related cases, where the fchdir() may have
been appropriate.

Meanwhile, are there any systems left that would even benefit from this
early exit patch?

/proc/self/fd/ works on Linux, but Linux already added all the *at
functions, so your fallback is only useful for kernel versions back when
this file was first written (are kernels that old still in active
enterprise use, or have RHEL and other stable-release distros moved on
to something that supports *at?).

/proc/self/fd/ exists but is broken on cygwin 1.5 and 1.7, and on
Solaris 10, so those platforms already use the fchdir() fallback.
Meanwhile, cygwin 1.7 has all the *at functions, and while Solaris 10
only has a subset of *at functions, my understanding is that the rest
are being added for Solaris 11.

Finally, most other systems lack /proc/self/fd in the first place, so
this early exit will never be triggered (assuming you patched
openat_proc_name to set errno to ENOTSUP when the cache states that
/proc/self/fd is missing or broken)..

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature