[PATCH 3/7] server: Fail to set delete disposition on a non-empty directory.
Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/ntdll/tests/file.c | 3 --- server/fd.c | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index 56a438b973..67a211c803 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -3038,17 +3038,14 @@ todo_wine CloseHandle( handle2 ); fdi.DoDeleteFile = TRUE; res = pNtSetInformationFile( handle, &io, &fdi, sizeof fdi, FileDispositionInformation ); - todo_wine ok( res == STATUS_DIRECTORY_NOT_EMPTY, "unexpected FileDispositionInformation result (expected STATUS_DIRECTORY_NOT_EMPTY, got %x)\n", res ); fileDeleted = DeleteFileA( buffer ); ok( fileDeleted, "File should have been deleted\n" ); buffer[dirpos] = '\0'; CloseHandle( handle ); fileDeleted = GetFileAttributesA( buffer ) == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND; - todo_wine ok( !fileDeleted, "Directory shouldn't have been deleted\n" ); fileDeleted = RemoveDirectoryA( buffer ); -todo_wine ok( fileDeleted, "Directory should have been deleted\n" ); } diff --git a/server/fd.c b/server/fd.c index fa174f597e..3184926d18 100644 --- a/server/fd.c +++ b/server/fd.c @@ -31,6 +31,9 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#ifdef HAVE_DIRENT_H +# include <dirent.h> +#endif #ifdef HAVE_POLL_H #include <poll.h> #endif @@ -2277,6 +2280,22 @@ static struct fd *get_handle_fd_obj( struct process *process, obj_handle_t handl return fd; } +static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + } + + return count <= 2; +} + /* set disposition for the fd */ static void set_fd_disposition( struct fd *fd, int unlink ) { @@ -2314,6 +2333,13 @@ static void set_fd_disposition( struct fd *fd, int unlink ) return; } + /* can't unlink a non-empty directory */ + if (unlink && S_ISDIR( st.st_mode ) && !is_directory_empty( fd )) + { + set_error( STATUS_DIRECTORY_NOT_EMPTY ); + return; + } + fd->closed->unlink = unlink || (fd->options & FILE_DELETE_ON_CLOSE); } -- 2.17.1
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
What would you suggest to use instead? I'd be glad to consider any other ways to detect a non-empty directory. -- Dmitry.
November 7, 2018 12:30 PM, "Dmitry Timoshkov" <dmitry(a)baikal.ru> wrote:
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
What would you suggest to use instead? I'd be glad to consider any other ways to detect a non-empty directory.
fstat(2) the directory file and check that st_nlink is greater than 2. Chip
"Chip Davis" <cdavis(a)codeweavers.com> wrote:
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
What would you suggest to use instead? I'd be glad to consider any other ways to detect a non-empty directory.
fstat(2) the directory file and check that st_nlink is greater than 2.
Thanks for the suggestion, unfortunately this approach doesn't seem to work for me under Linux, I always get st_nlink = 2 using the following test app: #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <sys/stat.h> int main(void) { char cwd[1024]; struct stat st; if (!getcwd(cwd, sizeof(cwd))) printf("getcwd failed\n"); else printf("cwd: %s\n", cwd); stat(cwd, &st); printf("st.st_nlink: %ld\n", st.st_nlink); return 0; } -- Dmitry.
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check? -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead.
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else? -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead.
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing? -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
+static int is_directory_empty( struct fd *fd ) +{ + DIR *dir; + int count = 0; + + if ((dir = fdopendir( fd->unix_fd ))) + { + while (readdir( dir ) != NULL && count <= 2) + count++; + + closedir( dir ); + }
This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead.
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing?
It's the task scheduler service that suffers from this. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
> +static int is_directory_empty( struct fd *fd ) > +{ > + DIR *dir; > + int count = 0; > + > + if ((dir = fdopendir( fd->unix_fd ))) > + { > + while (readdir( dir ) != NULL && count <= 2) > + count++; > + > + closedir( dir ); > + }
This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead.
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing?
It's the task scheduler service that suffers from this.
Where does it depend on the directory still existing? -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
> > +static int is_directory_empty( struct fd *fd ) > > +{ > > + DIR *dir; > > + int count = 0; > > + > > + if ((dir = fdopendir( fd->unix_fd ))) > > + { > > + while (readdir( dir ) != NULL && count <= 2) > > + count++; > > + > > + closedir( dir ); > > + } > > This won't work, closedir() is going to close the file descriptor.
Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead.
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing?
It's the task scheduler service that suffers from this.
Where does it depend on the directory still existing?
The scheduler service opens a directory for monitoring, and if that dir gets removed the things go haywire. I have an installer that calls RemoveDirectory() for the Tasks and Fonts directories, probably that happens inadvertently but still. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
>> > +static int is_directory_empty( struct fd *fd ) >> > +{ >> > + DIR *dir; >> > + int count = 0; >> > + >> > + if ((dir = fdopendir( fd->unix_fd ))) >> > + { >> > + while (readdir( dir ) != NULL && count <= 2) >> > + count++; >> > + >> > + closedir( dir ); >> > + } >> >> This won't work, closedir() is going to close the file descriptor. > > Would it be acceptable to duplicate the fd before the check?
That's what you'd have to do, yes. But honestly I'm not sure that this check is a good idea in the first place. I wonder if we shouldn't remove the directory right away instead.
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing?
It's the task scheduler service that suffers from this.
Where does it depend on the directory still existing?
The scheduler service opens a directory for monitoring, and if that dir gets removed the things go haywire. I have an installer that calls RemoveDirectory() for the Tasks and Fonts directories, probably that happens inadvertently but still.
It sounds like the scheduler service should be fixed. The directory could also get removed from outside of Wine, it should be able to cope with that. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
Setting a disposition is not supposed to remove a directory or a file, just mark it for the removal until the last handle to it gets closed, or do you mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing?
It's the task scheduler service that suffers from this.
Where does it depend on the directory still existing?
The scheduler service opens a directory for monitoring, and if that dir gets removed the things go haywire. I have an installer that calls RemoveDirectory() for the Tasks and Fonts directories, probably that happens inadvertently but still.
It sounds like the scheduler service should be fixed. The directory could also get removed from outside of Wine, it should be able to cope with that.
The scheduler service handles file related errors just fine, the source of the problem is that the directories shouldn't be removed in the first place at all. Of course I can resend the remaining patches without this one if it makes things easier for you, and there's no things you'd like to see changed in them. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
> Setting a disposition is not supposed to remove a directory or a file, just > mark it for the removal until the last handle to it gets closed, or do you > mean something else?
I know it's not supposed to, but I'm not sure it's a good enough reason to add this check and make RemoveDirectory less reliable. Do you have an app that depends on the directory still existing?
It's the task scheduler service that suffers from this.
Where does it depend on the directory still existing?
The scheduler service opens a directory for monitoring, and if that dir gets removed the things go haywire. I have an installer that calls RemoveDirectory() for the Tasks and Fonts directories, probably that happens inadvertently but still.
It sounds like the scheduler service should be fixed. The directory could also get removed from outside of Wine, it should be able to cope with that.
The scheduler service handles file related errors just fine, the source of the problem is that the directories shouldn't be removed in the first place at all. Of course I can resend the remaining patches without this one if it makes things easier for you, and there's no things you'd like to see changed in them.
Yes please, the other patches look good. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Chip Davis -
Dmitry Timoshkov