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 <roucaries.bastien+gnu...@gmail.com> > 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?= > <roucaries.bastien+gnu...@gmail.com> > 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 <roucaries.bastien+gnu...@gmail.com> > 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 ebl...@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
