[PATCH v2] ntdll: resolve drive symlink for mapped filename
Zebediah Figura
zfigura at codeweavers.com
Wed Sep 15 00:07:15 CDT 2021
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 <matias.nicolas.zc at gmail.com>
> ---
> 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, ¤t_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);
>
More information about the wine-devel
mailing list