rmdir: improvement for emptiness check

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

rmdir: improvement for emptiness check

Simon-2
Dear list,

when deleting a directory, cygwin checks if the directory is empty.
When doing so, it skipped every second file found in that directory
(note the repetition of the line "pfni = ...NextEntryOffset"). This is
a problem when, e.g., there are two files in that directory and the
first one is in a PENDING_DELETE state. The second one will not be
tested, so the directory is considered empty.

This is not an urgent patch, but fixing this should lower the
probability of an accidentally, temporarily "deleted" directory (i.e.
1. think it is empty; 2. move to recycle bin; 3. check again; 4.
notice the error and move back to its old location).

NB.: The whole move-to-bin strategy is broken and maybe even
unfounded. I don't know why cygwin is trying to move an empty
directory to a recycle.bin folder. The inherent race condition seems
avoidable to me. Is there a discussion regarding that behaviour?

Simon

--- a/winsup/cygwin/syscalls.cc 2017-07-19 10:42:02.000000000 +0200
+++ b/winsup/cygwin/syscalls.cc 2017-08-06 14:41:48.000000000 +0200
@@ -586,14 +586,14 @@
     {
       while (pfni->NextEntryOffset)
        {
+         pfni = (PFILE_NAMES_INFORMATION) ((caddr_t) pfni + pfni->NextEntryOffset);
+         /* skipping first two entries: "." and ".." */
          if (++cnt > 2)
            {
              UNICODE_STRING fname;
              OBJECT_ATTRIBUTES attr;
              FILE_BASIC_INFORMATION fbi;

-             pfni = (PFILE_NAMES_INFORMATION)
-                    ((caddr_t) pfni + pfni->NextEntryOffset);
              RtlInitCountedUnicodeString(&fname, pfni->FileName,
                                          pfni->FileNameLength);
              InitializeObjectAttributes (&attr, &fname, 0, dir, NULL);
@@ -627,7 +627,6 @@
                  return STATUS_DIRECTORY_NOT_EMPTY;
                }
            }
-         pfni = (PFILE_NAMES_INFORMATION) ((caddr_t) pfni + pfni->NextEntryOffset);
        }
     }
   while (NT_SUCCESS (NtQueryDirectoryFile (dir, NULL, NULL, 0, &io, pfni,

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: rmdir: improvement for emptiness check

Corinna Vinschen-2
Hi Simon,

On Aug  6 15:57, Simon wrote:

> Dear list,
>
> when deleting a directory, cygwin checks if the directory is empty.
> When doing so, it skipped every second file found in that directory
> (note the repetition of the line "pfni = ...NextEntryOffset"). This is
> a problem when, e.g., there are two files in that directory and the
> first one is in a PENDING_DELETE state. The second one will not be
> tested, so the directory is considered empty.
>
> This is not an urgent patch, but fixing this should lower the
> probability of an accidentally, temporarily "deleted" directory (i.e.
> 1. think it is empty; 2. move to recycle bin; 3. check again; 4.
> notice the error and move back to its old location).
Good catch!  Can you create a `git format-patch' style patch
with a nice log message, please?

> NB.: The whole move-to-bin strategy is broken and maybe even
> unfounded. I don't know why cygwin is trying to move an empty
> directory to a recycle.bin folder. The inherent race condition seems
> avoidable to me. Is there a discussion regarding that behaviour?

  $ mkdir dir
  $ cd dir
  $ rmdir ../dir


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Loading...