Re: RE : Re: openat-proc

[Eric Blake]

On 05/09/2011 01:10 PM, Eric Blake wrote:
> On 05/09/2011 12:06 PM, Bastien ROUCARIES wrote:
>> Sorry for top post (android)
>>
>> Proposed algo:
>> Ofd=open(/proc,o_search)
>> follow link /proc/self/fd/ofd/../proc
>> If bug will resolve to
>> /proc/self/fd/proc that does not exist
>> If not bug suceed (because we have already opened /proc
> 
> I still don't see quite what you are proposing, but you have made me
> re-read the openat-proc.c code and I think we definitely have a bug there.

Then again, after reading the code yet again, I think it's okay.

> Existing behavior is that there are two types of /proc:
> 
> Solaris and Cygwin - buggy, because /proc/self/fd/dir/.. resolves to
> /proc/self/fd rather than to the parent directory of dir.
> 
> Linux - resolves to the desired parent directory of dir.
> 
> Then again, it looks like you have found a real bug in openat-proc.c.
> Rather than comparing the inodes of '/proc/self/fd' and
> '/proc/self/fd/dir/..', it is comparing the inodes of '/proc/self' and
> '/proc/self/fd/..'.  On both Solaris and Cygwin, this comparison
> succeeds rather than fails, so it is not filtering out the buggy
> platforms as desired.

Rather, it is comparing the inodes of /proc/self and /proc/self/fd/n/..,
where fd n is /proc/self/fd.  If the bug is present, then
/proc/self/fd/n/.. resolves to /proc/self/fd rather than the intended
/proc/self.  I was mis-reading the code.

I suppose we could avoid the open() and just use two stat() calls, by
using the passed in fd for n rather than a directory we opened
ourselves; but this means that we would have to have inconclusive
results if the passed-in fd is not a directory.  Thus, successful
determination of the bug uses fewer syscalls (two stat(), vs. open(),
two stat(), and close()), but failure in determination (due to an
invalid fd argument) can result in many more syscalls (since the
proc_status cache variable remains unset longer than necessary).

> There's another aspect to that file - right now, it is doing
> open("/proc/self/fd",O_SEARCH) to determine if /proc is even mounted.
> But that step can be strictly skipped if we merely rely on comparing the
> two stat() calls to detect whether the '..' bug is present.

That is, I wrote this patch, but don't think it is worth applying:

diff --git c/lib/openat-proc.c w/lib/openat-proc.c
index 4a470c5..1f84d7d 100644
--- c/lib/openat-proc.c
+++ w/lib/openat-proc.c
@@ -73,23 +73,22 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int
fd, char const *file)
          to ".." after symbolic link expansion, so avoid /proc/self/fd
          if it mishandles "..".  Solaris 10 has openat, but this
          problem is exhibited on code that built on Solaris 8 and
-         running on Solaris 10.  */
-
-      int proc_self_fd = open ("/proc/self/fd", O_SEARCH);
-      if (proc_self_fd < 0)
-        proc_status = -1;
+         running on Solaris 10.  This also serves to bypass wasted
+         attempts if /proc is not mounted.  */
+      struct stat proc_self_fd_st;
+      if (stat ("/proc/self/fd", &proc_self_fd_st) < 0)
+        proc_status = -1; /* Looks like /proc is not mounted.  */
       else
         {
-          struct stat proc_self_fd_dotdot_st;
-          struct stat proc_self_st;
+          struct stat proc_self_fd_n_dotdot_st;
           char dotdot_buf[PROC_SELF_FD_NAME_SIZE_BOUND (sizeof ".." - 1)];
-          sprintf (dotdot_buf, PROC_SELF_FD_FORMAT, proc_self_fd, "..");
-          proc_status =
-            ((stat (dotdot_buf, &proc_self_fd_dotdot_st) == 0
-              && stat ("/proc/self", &proc_self_st) == 0
-              && SAME_INODE (proc_self_fd_dotdot_st, proc_self_st))
-             ? 1 : -1);
-          close (proc_self_fd);
+          sprintf (dotdot_buf, PROC_SELF_FD_FORMAT, fd, "..");
+          if (stat (dotdot_buf, &proc_self_fd_n_dotdot_st) == 0)
+            {
+              proc_status = SAME_INODE (proc_self_fd_n_dotdot_st,
+                                        proc_self_fd_st) ? -1 : 1;
+            }
+          /* Inconclusive if proc_self_fd was not an open directory fd.  */
         }
     }



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

signature.asc
Description: OpenPGP digital signature