[PATCH v3 resend 2/2] server: Allow creating named pipes using \Device\NamedPipe\ as RootDirectory.

[Jinoh Kang]

On 1/5/22 06:58, Jacek Caban wrote:

>> diff --git a/server/named_pipe.c b/server/named_pipe.c
>> index 3e6cf09d4f2..37520789722 100644
>> --- a/server/named_pipe.c
>> +++ b/server/named_pipe.c
>> @@ -103,6 +103,13 @@ struct named_pipe_device_file
>>       struct named_pipe_device *device;      /* named pipe device */
>>   };
>>   +struct named_pipe_dir
>> +{
>> +    struct object             obj;         /* object header */
>> +    struct fd                *fd;          /* pseudo-fd for ioctls */
>> +    struct named_pipe_device *device;      /* named pipe device */
>> +};
> 
> 
> This is identical to named_pipe_device_file, maybe we could reuse that (we'd still use different ops).

It's certainly possible to reuse the struct, but I suppose making the two structs separate would help check for potential object type confusion.

>> +static WCHAR *named_pipe_dir_get_full_name( struct object *obj, data_size_t *ret_len )
>> +{
>> +    static const WCHAR backslash = '\\';
>> +    struct named_pipe_dir *dir = (struct named_pipe_dir *)obj;
>> +    data_size_t len;
>> +    char *device_name, *ret;
>> +
>> +    device_name = (char *)dir->device->obj.ops->get_full_name( &dir->device->obj, &len );
>> +    if (!device_name) return NULL;
>> +
>> +    len += sizeof(WCHAR);
>> +    ret = realloc(device_name, len);
>> +    if (!ret)
>> +    {
>> +        free(device_name);
>> +        return NULL;
>> +    }
>> +
>> +    *ret_len = len;
>> +    memcpy( ret + len - sizeof(WCHAR), &backslash, sizeof(WCHAR) );
>> +    return (WCHAR *)ret;
>> +}
> 
> 
> My first impression was that there should be a better way, but it seems that we indeed something like that. I think that it could be made a bit nicer by using WCHAR type for device_name and ret. memcpy() can be replaced with a simple assignment:
> 
> ret[len / sizeof(WCHAR) - 1] = '\\';

I actually borrowed that part from server/object.c.  Puzzled I was at first, but I guessed there
must be some legit reason as to why things were done that way, presumably for compatibility with
older compilers.  (Apparently GCC 4.x compilation problems on Bugzilla were recently acknowledged as legitimate and fixed).

If this is not (or no longer) the case, perhaps we can modify the old code too?

> 
> 
>> +
>> +static struct object *named_pipe_dir_lookup_name( struct object *obj, struct unicode_str *name,
>> +                                                  unsigned int attr, struct object *root )
>> +{
>> +    struct named_pipe_dir *dir = (struct named_pipe_dir *)obj;
>> +    if (!name || !name->len) return NULL;  /* open the directory itself */
>> +    return dir->device->obj.ops->lookup_name( &dir->device->obj, name, attr, root );
>> +}
>> +
>> +static struct object *named_pipe_dir_open_file( struct object *obj, unsigned int access,
>> +                                                unsigned int sharing, unsigned int options )
>> +{
>> +    struct named_pipe_dir *dir = (struct named_pipe_dir *)obj;
>> +    if (!dir->fd && !(dir->fd = alloc_pseudo_fd( &named_pipe_dir_fd_ops, obj, options ))) return NULL;
> 
> 
> Why do you call alloc_pseudo_fd() here instead of named_pipe_device_lookup_name, where you alloc dir object?

named_pipe_device_lookup_name lacks the options parameter.

> Also, it could use allow_fd_caching().

Thanks!

> 
> 
> Thanks,
> 
> Jacek
> 


-- 
Sincerely,
Jinoh Kang