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