[PATCH v2 0/1] MR498: server: Allow creating named pipes using \Device\NamedPipe\ as RootDirectory. (#52105)
On Windows, \Device\NamedPipe\ is the root directory of the named pipe file system (NPFS), and can be used as RootDirectory to skip its path when accessing the NPFS namespace. This introduces a regression that makes it possible to use \Device\NamedPipe (note the lack of trailing backslash) as RootDirectory as well, since Wine does not distinguish between \Device\NamedPipe (the device itself) and \Device\NamedPipe\ (the root directory of the filesystem). (Previous iteration of this patch did distinguish between the two, but the method in which it was implemented was deemed hacky.) -- v2: server: Allow creating named pipes using \Device\NamedPipe\ as RootDirectory. https://gitlab.winehq.org/wine/wine/-/merge_requests/498
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Separate the named pipe root directory from the named pipe device file. Open the root directory instead of the device file if the path ends with backslash. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52105 Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/tests/om.c | 2 +- dlls/ntdll/tests/pipe.c | 6 +- server/named_pipe.c | 133 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 130 insertions(+), 11 deletions(-) diff --git a/dlls/ntdll/tests/om.c b/dlls/ntdll/tests/om.c index 62659fc8cb4..a2ae3e4d181 100644 --- a/dlls/ntdll/tests/om.c +++ b/dlls/ntdll/tests/om.c @@ -1960,7 +1960,7 @@ static void test_query_object(void) handle = CreateFileA( "\\\\.\\pipe\\", 0, 0, NULL, OPEN_EXISTING, 0, 0 ); ok( handle != INVALID_HANDLE_VALUE, "CreateFile failed (%lu)\n", GetLastError() ); - test_object_name( handle, L"\\Device\\NamedPipe\\", TRUE ); + test_object_name( handle, L"\\Device\\NamedPipe\\", FALSE ); test_object_type( handle, L"File" ); test_file_info( handle ); diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 697774dfa09..e25ecf66ffe 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -2766,7 +2766,7 @@ static void test_empty_name(void) pRtlInitUnicodeString(&name, L"nonexistent_pipe"); status = wait_pipe(hdirectory, &name, &zero_timeout); - todo_wine ok(status == STATUS_ILLEGAL_FUNCTION, "unexpected status for FSCTL_PIPE_WAIT on \\Device\\NamedPipe: %#lx\n", status); + ok(status == STATUS_ILLEGAL_FUNCTION, "unexpected status for FSCTL_PIPE_WAIT on \\Device\\NamedPipe: %#lx\n", status); subtest_empty_name_pipe_operations(hdirectory); @@ -2911,11 +2911,11 @@ static void test_empty_name(void) timeout.QuadPart = -(LONG64)10000000; status = pNtCreateNamedPipeFile(&hpipe, GENERIC_READ|GENERIC_WRITE, &attr, &io, FILE_SHARE_READ|FILE_SHARE_WRITE, FILE_CREATE, FILE_PIPE_FULL_DUPLEX, 0, 0, 0, 1, 256, 256, &timeout); - todo_wine ok(!status, "unexpected failure from NtCreateNamedPipeFile: %#lx\n", status); + ok(!status, "unexpected failure from NtCreateNamedPipeFile: %#lx\n", status); handle = CreateFileA("\\\\.\\pipe\\test3\\pipe", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0 ); - todo_wine ok(handle != INVALID_HANDLE_VALUE, "Failed to open NamedPipe (%lu)\n", GetLastError()); + ok(handle != INVALID_HANDLE_VALUE, "Failed to open NamedPipe (%lu)\n", GetLastError()); CloseHandle(handle); CloseHandle(hpipe); diff --git a/server/named_pipe.c b/server/named_pipe.c index f3404a33c3b..3ef87c20145 100644 --- a/server/named_pipe.c +++ b/server/named_pipe.c @@ -99,6 +99,7 @@ struct named_pipe_device struct named_pipe_device_file { struct object obj; /* object header */ + int is_rootdir; /* is root directory file? */ struct fd *fd; /* pseudo-fd for ioctls */ struct named_pipe_device *device; /* named pipe device */ }; @@ -277,6 +278,12 @@ static const struct object_ops named_pipe_device_ops = static void named_pipe_device_file_dump( struct object *obj, int verbose ); static struct fd *named_pipe_device_file_get_fd( struct object *obj ); static WCHAR *named_pipe_device_file_get_full_name( struct object *obj, data_size_t *len ); +static struct object *named_pipe_device_file_lookup_name( struct object *obj, struct unicode_str *name, + unsigned int attr, struct object *root ); +static struct object *named_pipe_device_file_open_file( struct object *obj, unsigned int access, + unsigned int sharing, unsigned int options ); +static enum server_fd_type named_pipe_rootdir_get_fd_type( struct fd *fd ); +static void named_pipe_rootdir_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ); static void named_pipe_device_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ); static enum server_fd_type named_pipe_device_file_get_fd_type( struct fd *fd ); static void named_pipe_device_file_destroy( struct object *obj ); @@ -296,10 +303,10 @@ static const struct object_ops named_pipe_device_file_ops = default_get_sd, /* get_sd */ default_set_sd, /* set_sd */ named_pipe_device_file_get_full_name, /* get_full_name */ - no_lookup_name, /* lookup_name */ + named_pipe_device_file_lookup_name, /* lookup_name */ no_link_name, /* link_name */ NULL, /* unlink_name */ - no_open_file, /* open_file */ + named_pipe_device_file_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ no_close_handle, /* close_handle */ named_pipe_device_file_destroy /* destroy */ @@ -321,6 +328,22 @@ static const struct fd_ops named_pipe_device_fd_ops = default_fd_reselect_async /* reselect_async */ }; +static const struct fd_ops named_pipe_rootdir_fd_ops = +{ + default_fd_get_poll_events, /* get_poll_events */ + default_poll_event, /* poll_event */ + named_pipe_rootdir_get_fd_type, /* get_fd_type */ + no_fd_read, /* read */ + no_fd_write, /* write */ + no_fd_flush, /* flush */ + default_fd_get_file_info, /* get_file_info */ + no_fd_get_volume_info, /* get_volume_info */ + named_pipe_rootdir_ioctl, /* ioctl */ + default_fd_cancel_async, /* cancel_async */ + default_fd_queue_async, /* queue_async */ + default_fd_reselect_async /* reselect_async */ +}; + static void named_pipe_dump( struct object *obj, int verbose ) { fputs( "Named pipe\n", stderr ); @@ -502,6 +525,20 @@ static struct object *named_pipe_device_lookup_name( struct object *obj, struct if (!name) return NULL; /* open the device itself */ + if (name->str && !name->len) + { + /* root directory: last component is empty ("\\Device\\NamedPipe\\") */ + struct named_pipe_device_file *dir = alloc_object( &named_pipe_device_file_ops ); + + if (!dir) return NULL; + + dir->is_rootdir = 1; + dir->fd = NULL; /* defer alloc_pseudo_fd() until after we have options */ + dir->device = (struct named_pipe_device *)grab_object( obj ); + + return &dir->obj; + } + if ((found = find_object( device->pipes, name, attr | OBJ_CASE_INSENSITIVE ))) name->len = 0; @@ -514,6 +551,7 @@ static struct object *named_pipe_device_open_file( struct object *obj, unsigned struct named_pipe_device_file *file; if (!(file = alloc_object( &named_pipe_device_file_ops ))) return NULL; + file->is_rootdir = 0; file->device = (struct named_pipe_device *)grab_object( obj ); if (!(file->fd = alloc_pseudo_fd( &named_pipe_device_fd_ops, obj, options ))) { @@ -553,7 +591,8 @@ static void named_pipe_device_file_dump( struct object *obj, int verbose ) { struct named_pipe_device_file *file = (struct named_pipe_device_file *)obj; - fprintf( stderr, "File on named pipe device %p\n", file->device ); + fprintf( stderr, "%s on named pipe device %p\n", + file->is_rootdir ? "Root directory" : "File", file->device ); } static struct fd *named_pipe_device_file_get_fd( struct object *obj ) @@ -562,10 +601,32 @@ static struct fd *named_pipe_device_file_get_fd( struct object *obj ) return (struct fd *)grab_object( file->fd ); } -static WCHAR *named_pipe_device_file_get_full_name( struct object *obj, data_size_t *len ) +static WCHAR *named_pipe_device_file_get_full_name( struct object *obj, data_size_t *ret_len ) { struct named_pipe_device_file *file = (struct named_pipe_device_file *)obj; - return file->device->obj.ops->get_full_name( &file->device->obj, len ); + WCHAR *device_name; + data_size_t len; + + device_name = file->device->obj.ops->get_full_name( &file->device->obj, &len ); + if (!device_name) return NULL; + + if (file->is_rootdir) + { + WCHAR *newbuf; + + len += sizeof(WCHAR); + if (!(newbuf = realloc(device_name, len))) + { + free(device_name); + return NULL; + } + + device_name = newbuf; + *(WCHAR *)((char *)device_name + len - sizeof(WCHAR)) = '\\'; + } + + *ret_len = len; + return device_name; } static enum server_fd_type named_pipe_device_file_get_fd_type( struct fd *fd ) @@ -573,6 +634,39 @@ static enum server_fd_type named_pipe_device_file_get_fd_type( struct fd *fd ) return FD_TYPE_DEVICE; } +static struct object *named_pipe_device_file_lookup_name( struct object *obj, struct unicode_str *name, + unsigned int attr, struct object *root ) +{ + struct named_pipe_device_file *file = (struct named_pipe_device_file *)obj; + + if (!file->is_rootdir) + return no_lookup_name( obj, name, attr, root ); + + if (!file->fd) + { + /* not yet opened; complete the object name lookup */ + return NULL; + } + + return file->device->obj.ops->lookup_name( &file->device->obj, name, attr, root ); +} + +static struct object *named_pipe_device_file_open_file( struct object *obj, unsigned int access, + unsigned int sharing, unsigned int options ) +{ + struct named_pipe_device_file *file = (struct named_pipe_device_file *)obj; + + if (!file->is_rootdir || file->fd) + return no_open_file( obj, access, sharing, options ); + + if (!(file->fd = alloc_pseudo_fd( &named_pipe_rootdir_fd_ops, obj, options ))) + return NULL; + + allow_fd_caching( file->fd ); + + return grab_object( obj ); +} + static void named_pipe_device_file_destroy( struct object *obj ) { struct named_pipe_device_file *file = (struct named_pipe_device_file*)obj; @@ -1321,14 +1415,20 @@ static struct pipe_end *create_pipe_client( struct named_pipe *pipe, data_size_t static int named_pipe_link_name( struct object *obj, struct object_name *name, struct object *parent ) { - struct named_pipe_device *dev = (struct named_pipe_device *)parent; + if (parent->ops == &named_pipe_device_file_ops) + { + struct named_pipe_device_file *file = (struct named_pipe_device_file *)parent; + + if (file->is_rootdir) + parent = &((struct named_pipe_device_file *)parent)->device->obj; + } if (parent->ops != &named_pipe_device_ops) { set_error( STATUS_OBJECT_NAME_INVALID ); return 0; } - namespace_add( dev->pipes, name ); + namespace_add( ((struct named_pipe_device *)parent)->pipes, name ); name->parent = grab_object( parent ); return 1; } @@ -1370,6 +1470,25 @@ static struct object *named_pipe_open_file( struct object *obj, unsigned int acc } static void named_pipe_device_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) +{ + switch (code) + { + case FSCTL_PIPE_WAIT: + set_error( STATUS_ILLEGAL_FUNCTION ); + break; + + default: + default_fd_ioctl( fd, code, async ); + } +} + +static enum server_fd_type named_pipe_rootdir_get_fd_type( struct fd *fd ) +{ + /* TODO actually implement NtQueryDirectoryFile */ + return FD_TYPE_DIR; +} + +static void named_pipe_rootdir_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) { struct named_pipe_device *device = get_fd_user( fd ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/498
**Note**: `if (name->str && !name->len)` may look hacky, but it's used to indicate trailing `\\`[^bks] and existing code already does it too. - `server/directory.c` checks `name->str`[^dir] to test if this is the last component. This is evident by the fact that changing it to `name->len` will lead to major regression. - `server/registry.c` has similar lookup logic.[^reg1][^reg2] [^bks]: https://gitlab.winehq.org/wine/wine/-/blob/c64aa0006e4a33d755a57a693cd81dc1e... [^dir]: https://gitlab.winehq.org/wine/wine/-/blob/c64aa0006e4a33d755a57a693cd81dc1e... [^reg1]: <https://gitlab.winehq.org/wine/wine/-/blob/c64aa0006e4a33d755a57a693cd81dc1e...> [^reg2]: <https://gitlab.winehq.org/wine/wine/-/blob/c64aa0006e4a33d755a57a693cd81dc1e...> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_52710
Are there enough differences between \Device\NamedPipe and \Device\NamedPipe\ to warrant the separate fd_ops? Does it actually make sense to use FD_TYPE_DIR here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_52768
Are there enough differences between \\Device\\NamedPipe and \\Device\\NamedPipe\\
Not sure about that. I don't see enough tests to judge. I could be wrong, though.
to warrant the separate fd_ops?
They can be merged. The only think stopping the merge is the fact that `get_fd_user(fd)` points to the device *itself*, not the file object that contains the `is_rootdir` flag.
Does it actually make sense to use FD_TYPE_DIR here?
Putting anything here doesn't make a difference (hence TODO). In fact, removing `get_fd_type` entirely makes sense, too. Both `ntdll:om` and `ntdll:pipe` work just fine with this patch: ```diff diff --git a/server/named_pipe.c b/server/named_pipe.c index 3ef87c20145..3628c1d9200 100644 --- a/server/named_pipe.c +++ b/server/named_pipe.c @@ -316,7 +316,7 @@ static const struct fd_ops named_pipe_device_fd_ops = { default_fd_get_poll_events, /* get_poll_events */ default_poll_event, /* poll_event */ - named_pipe_device_file_get_fd_type, /* get_fd_type */ + NULL, /* get_fd_type */ no_fd_read, /* read */ no_fd_write, /* write */ no_fd_flush, /* flush */ @@ -332,7 +332,7 @@ static const struct fd_ops named_pipe_rootdir_fd_ops = { default_fd_get_poll_events, /* get_poll_events */ default_poll_event, /* poll_event */ - named_pipe_rootdir_get_fd_type, /* get_fd_type */ + NULL, /* get_fd_type */ no_fd_read, /* read */ no_fd_write, /* write */ no_fd_flush, /* flush */ ``` In fact, there is a precedence: https://gitlab.winehq.org/wine/wine/-/blob/7c45c7c5ebb59237d568a4e5b38626422... Shall I go ahead and set both of them to NULL? `\Device\NamedPipe\` doesn't look like a "device" to me anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_52873
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=140059 Your paranoid android. === debian11 (build log) === error: patch failed: server/named_pipe.c:332 Task: Patch failed to apply === debian11b (build log) === error: patch failed: server/named_pipe.c:332 Task: Patch failed to apply
participants (4)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin -
Zebediah Figura (＠zfigura)