[PATCH v3 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. --- **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...> -- v3: 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 Fixes: 2600ecd4edfdb71097105c74312f83845305a4f2 --- dlls/ntdll/tests/om.c | 2 +- dlls/ntdll/tests/pipe.c | 4 +- server/named_pipe.c | 96 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 92 insertions(+), 10 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..2f1f7c52518 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -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..294b9e92e0f 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,10 @@ 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 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 +301,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 */ @@ -502,6 +507,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 +533,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 +573,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 +583,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 +616,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_device_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 +1397,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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/498
On Mon Nov 20 13:46:57 2023 +0000, Jinoh Kang wrote:
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 --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. Removed special logic for `is_rootdir = 1` case entirely. The `FSCTL_PIPE_WAIT` test is now left todo.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_52882
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.
In particular I notice they have the same (nontrivial) behaviour for most ioctls, cf. subtest_empty_name_pipe_operations(). Perhaps more generally, if they have the same object_ops it feels mildly odd for them to have different fd_ops.
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.
That can be changed, though, right?
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.
inotify is an unusual case because it's only used internally, and the vtable actually hasn't been updated when new functions are added, which is debatably a good thing. Regardless, get_fd_type() is never called for a pseudo-fd, so it's probably fine to leave it NULL. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_52917
On Tue Nov 21 15:13:30 2023 +0000, Zebediah Figura wrote:
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. In particular I notice they have the same (nontrivial) behaviour for most ioctls, cf. subtest_empty_name_pipe_operations(). Perhaps more generally, if they have the same object_ops it feels mildly odd for them to have different fd_ops.
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. That can be changed, though, right? 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. inotify is an unusual case because it's only used internally, and the vtable actually hasn't been updated when new functions are added, which is debatably a good thing. Regardless, get_fd_type() is never called for a pseudo-fd, so it's probably fine to leave it NULL. I pushed a smaller patch that removes rootdir special casing in ioctl entirely.
This patch is enough to make Cygwin happy. I'm planning to fix the rest in anotber series; do you have any concern with this approach? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53100
Zebediah Figura (@zfigura) commented about server/named_pipe.c:
if (!name) return NULL; /* open the device itself */
+ if (name->str && !name->len)
This part makes sense, esp. given precedent, although why is "if (name->str)" alone not enough here, like the other cases? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53166
Zebediah Figura (@zfigura) commented about server/named_pipe.c:
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;
This on the other hand bothers me. Maybe my intuition as to what's sensible design is off, but creating an object from lookup_name() feels wrong, especially given that we have to defer creating the fd. I guess the right solution here is to combine lookup_name() and open_file(), presumably using OBJ_OPENIF to distinguish the two. Or possibly the right solution is to create (ahead of time) a separate named pipe "device" that represents the root directory, but I'm not thrilled about that idea; it feels less architecturally clear to me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53167
On Tue Nov 21 21:38:54 2023 +0000, Zebediah Figura wrote:
This part makes sense, esp. given precedent, although why is "if (name->str)" alone not enough here, like the other cases? Because `name->str` is not NULL for ordinary named pipes too.
There is an option to delegate `find_object` to the root dir obj. In that case, I think it makes sense to change the design so that: 1. the root directory becomes a persistent object, and 2. optionally, the named pipe namespace is moved from the device to the root directory. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53220
This on the other hand bothers me. Maybe my intuition as to what's sensible design is off, but creating an object from lookup_name() feels wrong, especially given that we have to defer creating the fd.
I generally concur. There is precedent too, though: in `console_lookup_name` and `console_server_lookup_name`.
I guess the right solution here is to combine lookup_name() and open_file(),
How do you see it implemented? Shall I combine them in `struct object_ops`, and adjust implementors appropriately?
presumably using OBJ_OPENIF to distinguish the two.
I'm not sure about that. 1. Interposing different semantics to `attr` is in-band signaling. - Note that native ignores `OBJ_OPENIF` entirely IIRC (you can't create a file normally there). If we want to use it for internal purposes, we would want to *special case* NPFS and then filter that flag out. - Also, `open_file` doesn't accept `attr`. Or perhaps, did you mean for me to extend it? 2. If `OBJ_OPENIF` were simply instead a magic value for a new internal parameter, then it would be unnecessary indirection. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53225
On Tue Nov 21 23:45:47 2023 +0000, Jinoh Kang wrote:
Because `name->str` is not NULL for ordinary named pipes too. There is an option to delegate `find_object` to the root dir obj. In that case, I think it makes sense to change the design so that: 1. the root directory becomes a persistent object, and 2. optionally, the named pipe namespace is moved from the device to the root directory. Sorry, never mind, ignore that comment please, I can't read, or think apparently.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53228
presumably using OBJ_OPENIF to distinguish the two.
I'm not sure about that.
1. Interposing different semantics to `attr` is in-band signaling. * Note that native ignores `OBJ_OPENIF` entirely IIRC (you can't create a file normally there). If we want to use it for internal purposes, we would want to *special case* NPFS and then filter that flag out.
I misremember, it's not OBJ_OPENIF that does this exactly, it's FILE_CREATE / FILE_OPEN_IF. That'd have to be plumbed.
* Also, `open_file` doesn't accept `attr`. Or perhaps, did you mean for me to extend it?
Combining lookup_name() and open_file() would imply passing attr, yes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53233
On Wed Nov 22 00:22:36 2023 +0000, Zebediah Figura wrote:
presumably using OBJ_OPENIF to distinguish the two.
I'm not sure about that.
1. Interposing different semantics to `attr` is in-band signaling. * Note that native ignores `OBJ_OPENIF` entirely IIRC (you can't create a file normally there). If we want to use it for internal purposes, we would want to *special case* NPFS and then filter that flag out. I misremember, it's not OBJ_OPENIF that does this exactly, it's FILE_CREATE / FILE_OPEN_IF. That'd have to be plumbed. * Also, `open_file` doesn't accept `attr`. Or perhaps, did you mean for me to extend it? Combining lookup_name() and open_file() would imply passing attr, yes. I still don't get what `FILE_CREATE` vs `FILE_OPEN_IF` solves. Am I supposed to distinguish between the presence and absense of trailing `\` with them? Note that you cannot "create" a named pipe via `NtCreateFile`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53298
On Wed Nov 22 11:00:39 2023 +0000, Jinoh Kang wrote:
~~I still don't get what `FILE_CREATE` vs `FILE_OPEN_IF` solves. Am I supposed to distinguish between the presence and absense of trailing `\` with them? Note that you cannot "create" a named pipe via `NtCreateFile`.~~ Please disregard the last message. Sorry for the noise!
You're telling me to fuse `lookup_name` and `open_file` together, so that it's more natural to summon a file object while looking up an object path. However: 1. It's even more complex than just fixing `open_file()` so that it receives a file name. On the other hand, your fusing proposal touches two methods instead of just one. - If we still want to do global refactoring, we can do it in a separate PR. Also see the point below about server/console.c 2. There are objects (e.g., `registry`) that have a complex `lookup_name` implementation with *several* early returns. Fusing them will require either duplicating "open_file" code at each return point or factoring out so that we still have separate lookup and open-file. - It's worth mentioning that, on native, object lookup and file opening is externally completely distinct stage (see `IRP_MJ_CREATE`). After all, the kludgy "create the file object in midst of lookup" thing already has precedence in server/console.c. Introducing one more instance doesn't seem like regression of style, although I could be wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53308
1. It's even more complex than just fixing `open_file()` so that it receives a file name. In other words, your fusing proposal touches two methods instead of just one. * If we still want to do global refactoring, we can do it in a separate PR. Also see the point below about server/console.c 1. There are objects (e.g., `registry`) that have a complex `lookup_name` implementation with *several* early returns. Fusing them will require either duplicating "open_file" code at each return point or factoring out so that we still have separate lookup and open-file.
Yes, you'd have to do some nontrivial refactoring. That doesn't mean it's the wrong thing to do (although it'd be nice to have confirmation that it's the right thing to do).
* It's worth mentioning that, on native, object lookup and file opening is externally completely distinct stage (see `IRP_MJ_CREATE`).
N...ot really, though? IRP_MJ_CREATE only applies to devices, whereas we use this infrastructure for several different object types. Also, IRP_MJ_CREATE does get passed the whole path, and FILE_OPEN vs FILE_CREATE, so I'd say that they're not distinct stages.
After all, the kludgy "create the file object in midst of lookup" thing already has precedent in server/console.c. Introducing one more instance doesn't seem like regression of style, although I could be wrong.
If it has precedent then perhaps this patch is fine, although perhaps we should also not bother delaying the create_pseudo_fd() call like the console does. It's not what I would do, but that's not saying much. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/498#note_53356
participants (3)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Zebediah Figura (＠zfigura)