[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/c64aa0006e4a33d755a57a693cd81dc1ed95fa9d/server/registry.c#L521 [^reg2]: https://gitlab.winehq.org/wine/wine/-/blob/c64aa0006e4a33d755a57a693cd81dc1ed95fa9d/server/registry.c#L531
-- v3: server: Allow creating named pipes using \Device\NamedPipe\ as RootDirectory.
From: Jinoh Kang jinoh.kang.kr@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; }
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.
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.
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?
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?
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.
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.
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.
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:
- the root directory becomes a persistent object, and
- 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.
presumably using OBJ_OPENIF to distinguish the two.
I'm not sure about that.
- 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.
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.
- 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`.
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.
- 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
- 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.
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Zebediah Figura (＠zfigura)