Re: [PATCH v2] ntdll: resolve drive symlink for mapped filename

[Thomas Crider]

I've discovered that this patch, or at least the version from this
submission, causes Elder Scrolls Online to crash a few seconds after the
game launches.

On Wed, Sep 15, 2021 at 1:07 AM Zebediah Figura 
wrote:
> Hello Matias, thanks for the patch!
>
> A couple high-level questions occur to me:
> - Should we resolve the symlink in ntdll, or the server? Unfortunately
>
> the answer to this one isn't clear to me.
> - Should we resolve the symlink (either on the client or server side)
>
> when opening the file/view, instead of when querying it? I'm inclined to
> say that we should, so that other APIs like
> NtQueryObject(ObjectNameInformation) and
> NtQueryInformationProcess(ProcessImageFileName) work as well.
>
> I also have some comments inlined below.
>
> On 9/10/21 5:25 PM, Matias Zuniga wrote:
> > Applications expect a path starting with a drive like
> > '\Device\Harddisk1' instead of a drive letter.
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51687
> > Signed-off-by: Matias Zuniga
> > v2: Fix info.c test breakage
> >
> > dlls/ntdll/unix/virtual.c     | 97 ++++++++++++++++++++++++++++++++---
> >   dlls/psapi/tests/psapi_main.c |  5 +-
> >   2 files changed, 90 insertions(+), 12 deletions(-)
> >
> > diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c
> > index 984af2d4a21..48bfaf3cf99 100644
> > --- a/dlls/ntdll/unix/virtual.c
> > +++ b/dlls/ntdll/unix/virtual.c
> > @@ -133,6 +133,8 @@ struct file_view
> >            } \
> >       } while (0);
> >
> > +#define SYMBOLIC_LINK_QUERY 0x0001
> > -
>
> This is defined in ddk/wdm.h.
> > /* per-page protection flags */
> >   #define VPROT_READ       0x01
> >   #define VPROT_WRITE      0x02
> > @@ -4273,34 +4275,113 @@ static NTSTATUS get_working_set_ex( HANDLE
>
> process, LPCVOID addr,
> >   return STATUS_SUCCESS;
> >
> > }
> >
> > +static NTSTATUS read_nt_symlink( UNICODE_STRING *name, WCHAR *target,
>
> DWORD size )
> > +{
> > - NTSTATUS status;
> > - OBJECT_ATTRIBUTES attr;
> > - HANDLE handle;
> > -
> > - attr.Length = sizeof(attr);
> > - attr.RootDirectory = 0;
> > - attr.Attributes = OBJ_CASE_INSENSITIVE;
> > - attr.ObjectName = name;
> > - attr.SecurityDescriptor = NULL;
> > - attr.SecurityQualityOfService = NULL;
> > -
> > - if (!(status = NtOpenSymbolicLinkObject( &handle,
>
> SYMBOLIC_LINK_QUERY, &attr )))
> > - {
> > -
> >    UNICODE_STRING targetW;
> > -
> >    targetW.Buffer = target;
> > -
> >    targetW.MaximumLength = (size - 1) * sizeof(WCHAR);
> > -
> >    status = NtQuerySymbolicLinkObject( handle, &targetW, NULL );
> > -
> >    if (!status) target[targetW.Length / sizeof(WCHAR)] = 0;
> > -
> >    NtClose( handle );
> > - }
> > - return status;
> >
> > +}
> > -
> >
> > +static NTSTATUS follow_device_symlink( WCHAR *name, SIZE_T
>
> max_path_len, WCHAR *buffer,
> > -
> >                                   SIZE_T buffer_len, SIZE_T
>
> *current_path_len )
>
> This is terribly confusing; what's an input parameter, and what's an
> output? Why are you passing three different input sizes?
> > +{
> > - WCHAR *p;
> > - NTSTATUS status = STATUS_SUCCESS;
> > - SIZE_T devname_len = 6 * sizeof(WCHAR); // e.g. ??\C:
>
> Please avoid C++ comments in Wine code.
> > - UNICODE_STRING devname;
> > - SIZE_T target_len;
> > -
> > - if (*current_path_len > buffer_len) return STATUS_INVALID_PARAMETER;
>
> This kind of check is pointless; we control all the parameters; it
> should never happen.
> > -
> > - if (*current_path_len >= devname_len && buffer[devname_len /
>
> sizeof(WCHAR) - 1] == ':') {
>
> It strikes me as awkward that we're validating the colon and nothing else.
>
> For that matter, we could make this even more robust by not even
> assuming that the name is in ??\C:\ format, and just resolving the
> first symlink we can. I think it always will be at the moment, though...
> > -
> >    devname.Buffer = buffer;
> > -
> >    devname.Length = devname_len;
> > -
> > -
> >    p = buffer + (*current_path_len / sizeof(WCHAR));
> > -
> >    if (!(status = read_nt_symlink( &devname, p, (buffer_len -
>
> *current_path_len) / sizeof(WCHAR) )))
>
> You're passing the size in characters, only to convert it back into
> bytes in read_nt_symlink()...
> > -
> >    {
> > -
> >        *current_path_len -= devname_len; // skip the device name
> > -
> >        target_len = lstrlenW(p) * sizeof(WCHAR);
>
> Don't we already have the length? In fact, I don't even think the server
> guarantees us a null-terminated name.
> > -
> >        *current_path_len += target_len;
> > -
> > -
> >        if (*current_path_len <= max_path_len)
> > -
> >        {
> > -
> >            memcpy( name, p, target_len );
> > -
> >            p = buffer + devname_len / sizeof(WCHAR);
> > -
> >            memcpy( name + target_len / sizeof(WCHAR), p,
>
> *current_path_len - target_len);
> > -
> >        }
> > -
> >        else status = STATUS_BUFFER_OVERFLOW;
> > -
> >    }
> > - }
> > - else if (*current_path_len <= max_path_len) {
> > -
> >    memcpy( name, buffer, *current_path_len );
> > - }
> > - else status = STATUS_BUFFER_OVERFLOW;
> > -
> > - return status;
> >
> > +}
> > - static NTSTATUS get_memory_section_name( HANDLE process, LPCVOID addr,
> >                                          MEMORY_SECTION_NAME *info,
>
> SIZE_T len, SIZE_T *ret_len )
> > {
> > - SIZE_T current_path_len, max_path_len = 0;
> > - // buffer to hold the path + 6 chars devname (e.g. ??\C:)
> > - SIZE_T buffer_len = (MAX_PATH + 6) * sizeof(WCHAR);
>
> We don't want to rely on MAX_PATH here.
> > -
> >
> > WCHAR *buffer = NULL;
> >  NTSTATUS status;
> >
> > if (!info) return STATUS_ACCESS_VIOLATION;
> > -
> >
> > if (!(buffer = malloc( buffer_len ))) return STATUS_NO_MEMORY;
> > -
> >
> > if (len > sizeof(*info) + sizeof(WCHAR))
> > -
> >
> > {
> > -
> >    max_path_len = len - sizeof(*info) - sizeof(WCHAR); // dont
>
> count null char
> > -
> >
> > }
> >
> > SERVER_START_REQ( get_mapping_filename )
> >  {
> >      req->process = wine_server_obj_handle( process );
> >      req->addr = wine_server_client_ptr( addr );
> > -
> >    if (len > sizeof(*info) + sizeof(WCHAR))
> > -
> >        wine_server_set_reply( req, info + 1, len - sizeof(*info) -
>
> sizeof(WCHAR) );
> > -
> >    wine_server_set_reply( req, buffer, MAX_PATH );
> >     status = wine_server_call( req );
> > -
> >    if (!status || status == STATUS_BUFFER_OVERFLOW)
> > -
> >    if (!status)
> >     {
> > -
> >        if (ret_len) *ret_len = sizeof(*info) + reply->len +
>
> sizeof(WCHAR);
> > -
> >        if (len < sizeof(*info)) status =
>
> STATUS_INFO_LENGTH_MISMATCH;
> > -
> >        current_path_len = reply->len;
> > -
> >        status = follow_device_symlink( (WCHAR *)(info + 1),
>
> max_path_len, buffer, buffer_len, &current_path_len);
> > -
> >        if (len < sizeof(*info))
> > -
> >        {
> > -
> >            status = STATUS_INFO_LENGTH_MISMATCH;
> > -
> >        }
> > -
> > -
> >        if (ret_len) *ret_len = sizeof(*info) + current_path_len +
>
> sizeof(WCHAR);
> >           if (!status)
> >           {
> >               info->SectionFileName.Buffer = (WCHAR *)(info + 1);
> > -
> >            info->SectionFileName.Length = reply->len;
> > -
> >            info->SectionFileName.MaximumLength = reply->len +
>
> sizeof(WCHAR);
> > -
> >            info->SectionFileName.Buffer[reply->len /
>
> sizeof(WCHAR)] = 0;
> > -
> >            info->SectionFileName.Length = current_path_len;
> > -
> >            info->SectionFileName.MaximumLength = current_path_len
> - sizeof(WCHAR);
> > -
> >            info->SectionFileName.Buffer[current_path_len /
>
> sizeof(WCHAR)] = 0;
> >           }
> >       }
> >   }
> >   SERVER_END_REQ;
> > - free(buffer);
> >  return status;
> > }
> >
> > diff --git a/dlls/psapi/tests/psapi_main.c
>
> b/dlls/psapi/tests/psapi_main.c
> > index c4a740310a4..d6fb8e69384 100644
> > --- a/dlls/psapi/tests/psapi_main.c
> > +++ b/dlls/psapi/tests/psapi_main.c
> > @@ -471,7 +471,6 @@ static void test_GetMappedFileName(void)
> >       ret = GetMappedFileNameA(GetCurrentProcess(), base, map_name,
>
> sizeof(map_name));
> >   ok(ret, "GetMappedFileName error %d\n", GetLastError());
> >   ok(ret > strlen(device_name), "map_name should be longer than
>
> device_name\n");
> > - todo_wine
> >  ok(memcmp(map_name, device_name, strlen(device_name)) == 0, "map
>
> name does not start with a device name: %s\n", map_name);
> >   SetLastError(0xdeadbeef);
> >
> > @@ -482,7 +481,6 @@ static void test_GetMappedFileName(void)
> >       {
> >           ok(memcmp(map_nameW, nt_map_name, lstrlenW(map_nameW)) == 0,
>
> "map name does not start with a device name: %s\n", map_name);
> >       WideCharToMultiByte(CP_ACP, 0, map_nameW, -1, map_name,
>
> MAX_PATH, NULL, NULL);
> > -
> >    todo_wine
> >     ok(memcmp(map_name, device_name, strlen(device_name)) == 0,
>
> "map name does not start with a device name: %s\n", map_name);
> >   }
> >
> > @@ -490,7 +488,6 @@ static void test_GetMappedFileName(void)
> >       ret = GetMappedFileNameA(GetCurrentProcess(), base + 0x2000,
>
> map_name, sizeof(map_name));
> >   ok(ret, "GetMappedFileName error %d\n", GetLastError());
> >   ok(ret > strlen(device_name), "map_name should be longer than
>
> device_name\n");
> > - todo_wine
> >  ok(memcmp(map_name, device_name, strlen(device_name)) == 0, "map
>
> name does not start with a device name: %s\n", map_name);
> >   SetLastError(0xdeadbeef);
> >
> > @@ -566,7 +563,7 @@ static void test_GetProcessImageFileName(void)
> >       {
> >           /* Windows returns 2*strlen-1 */
> >           ok(ret >= strlen(szImgPath), "szImgPath="%s" ret=%d\n",
>
> szImgPath, ret);
> > -
> >    todo_wine ok(!strcmp(szImgPath, szMapPath), "szImgPath=\"%s\"
>
> szMapPath="%s"\n", szImgPath, szMapPath);
> > -
> >    ok(!strcmp(szImgPath, szMapPath), "szImgPath=\"%s\"
>
> szMapPath="%s"\n", szImgPath, szMapPath);
> >   }
> >
> >   SetLastError(0xdeadbeef);