Re: Openat without die

[Bastien ROUCARIES]

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 <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.

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