Le mardi 11 janvier 2011 19:36:36, Eric Blake a écrit :
> 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.
[...]
> :), and we are not in a position to check for all possible bad pointers
Ok point taken
[...]
>
> > +
> > + /* 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.
[...]
I plan to factorize a little bit the code and testing early this code path will
simplify further factorisation. But will do
latter.
> > +++ 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.
>
> > 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.
I plan to factorize this part of code in its own function, and use a stub that
that return -1 and set errno to ENOTSUP. But wait
if errno is declared volatile it will not work. Will use #ifdef.
Point taken
> 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?).
Openat was added in 2.6.16, and nahant (REL4) is still supported (last beta
20101210) with a 2.6.9 kernel
(I have not checked kernel side patch for openat).
If we plan to release libposix could we manually enforce minimal kernel version
at config time ? It will avoid to compile a lot of
useless code on recent debian for instance (with O_CLOEXEC flag for instance).
> /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.
So /proc/self/fd is a linux only fallback ?
> 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)..
Ok so we could move proc stuff to old linux only #define
Bastien
