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@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 nametarget_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);