[PATCH v8 0/2] MR8459: urlmon: Add PersistentZoneIdentifier semi-stub

[Mike Kozelkov (＠augenzi)]

Implementation PersistentZoneIdentifer object without any writing and reading zone information. OS Windows uses NTFS alternative data stream Zone.Identifer to write and read zone information.

--
  v8: urlmon: Add PersistentZoneIdentifier test cases
      urlmon: Add PersistentZoneIdentifier semi-stubs
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459

[PATCH v8 1/2] urlmon: Add PersistentZoneIdentifier semi-stubs

[Mike Kozelkov]

From: Mike Kozelkov <augenzi(a)etersoft.ru>

---
 dlls/urlmon/zone_id.c | 150 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 133 insertions(+), 17 deletions(-)

diff --git a/dlls/urlmon/zone_id.c b/dlls/urlmon/zone_id.c
index 43712fadbf7..abb2be4747d 100644
--- a/dlls/urlmon/zone_id.c
+++ b/dlls/urlmon/zone_id.c
@@ -27,6 +27,12 @@ typedef struct {
     IPersistFile    IPersistFile_iface;
     IZoneIdentifier IZoneIdentifier_iface;
 
+    LPWSTR save_file_name;
+    LPWSTR load_file_name;
+
+    URLZONE zone;
+    BOOL is_zone_removed;
+
     IUnknown        *outer;
 
     LONG ref;
@@ -103,6 +109,8 @@ static ULONG WINAPI PZIUnk_Release(IUnknown *iface)
 
     if (!ref)
     {
+        free(This->save_file_name);
+        free(This->load_file_name);
         free(This);
         URLMON_UnlockModule();
     }
@@ -147,16 +155,21 @@ static HRESULT WINAPI PZIPersistFile_GetClassID(IPersistFile *iface, CLSID *clsi
 {
     PersistentZoneIdentifier *This = impl_from_IPersistFile(iface);
 
-    FIXME("(%p, %p) not implemented\n", This, clsid);
+    TRACE("(%p, %p)\n", This, clsid);
 
-    return E_NOTIMPL;
+    *clsid = CLSID_PersistentZoneIdentifier;
+
+    return S_OK;
 }
 
 static HRESULT WINAPI PZIPersistFile_GetCurFile(IPersistFile *iface, LPOLESTR *file_name)
 {
     PersistentZoneIdentifier *This = impl_from_IPersistFile(iface);
 
-    FIXME("(%p, %p) not implemented\n", This, file_name);
+    /* GetCurFile isn't implemented in Windows*/
+    TRACE("(%p, %p)\n", This, file_name);
+
+    *file_name = NULL;
 
     return E_NOTIMPL;
 }
@@ -165,36 +178,98 @@ static HRESULT WINAPI PZIPersistFile_IsDirty(IPersistFile *iface)
 {
     PersistentZoneIdentifier *This = impl_from_IPersistFile(iface);
 
-    FIXME("(%p) not implemented\n", This);
+    TRACE("(%p)\n", This);
 
-    return E_NOTIMPL;
+    return S_OK;
 }
 
 static HRESULT WINAPI PZIPersistFile_Load(IPersistFile *iface, LPCOLESTR file_name, DWORD mode)
 {
     PersistentZoneIdentifier *This = impl_from_IPersistFile(iface);
+    HANDLE hfile;
 
-    FIXME("(%p, %s, 0x%08lx) not implemented\n", This, debugstr_w(file_name), mode);
+    FIXME("(%p, %s, 0x%08lx) semi-stub\n", This, debugstr_w(file_name), mode);
 
-    return E_NOTIMPL;
+    if (!file_name)
+    {
+        if (!This->load_file_name)
+            return E_INVALIDARG;
+
+        file_name = This->load_file_name;
+    }
+
+    /* FIXME: Read zone information from Zone.Identifier NTFS alternate data stream */
+
+    hfile = CreateFileW(file_name, GENERIC_READ,
+        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING,
+        FILE_ATTRIBUTE_NORMAL, NULL);
+    if (hfile == INVALID_HANDLE_VALUE)
+        return HRESULT_FROM_WIN32(GetLastError());
+
+    CloseHandle(hfile);
+
+    if (lstrcmpW(This->load_file_name, file_name))
+    {
+        if (This->load_file_name)
+            free(This->load_file_name);
+
+        This->load_file_name = wcsdup(file_name);
+    }
+
+    return S_OK;
 }
 
 static HRESULT WINAPI PZIPersistFile_Save(IPersistFile *iface, LPCOLESTR file_name, BOOL remember)
 {
     PersistentZoneIdentifier *This = impl_from_IPersistFile(iface);
+    WCHAR *new_file_name;
+    HANDLE hfile;
 
-    FIXME("(%p, %s, %d) not implemented\n", This, debugstr_w(file_name), remember);
+    FIXME("(%p, %s, %d) semi-stub\n", This, debugstr_w(file_name), remember);
 
-    return E_NOTIMPL;
+    if (!file_name)
+    {
+        if (!This->save_file_name)
+            return E_INVALIDARG;
+
+        file_name = This->save_file_name;
+        remember = FALSE;
+    }
+
+    /* FIXME: Write zone information to Zone.Identifier NTFS alternate data stream */
+
+    hfile = CreateFileW(file_name, GENERIC_READ | GENERIC_WRITE,
+        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_ALWAYS,
+        FILE_ATTRIBUTE_NORMAL, NULL);
+    if (hfile == INVALID_HANDLE_VALUE)
+        return HRESULT_FROM_WIN32(GetLastError());
+
+    CloseHandle(hfile);
+
+    if (remember)
+    {
+        new_file_name = wcsdup(file_name);
+        if (!new_file_name)
+            return E_OUTOFMEMORY;
+
+        if (This->save_file_name)
+            free(This->save_file_name);
+
+        This->save_file_name = new_file_name;
+    }
+
+    return S_OK;
 }
 
 static HRESULT WINAPI PZIPersistFile_SaveCompleted(IPersistFile* iface, LPCOLESTR pszFileName)
 {
     PersistentZoneIdentifier *This = impl_from_IPersistFile(iface);
 
-    FIXME("(%p, %p) not implemented\n", This, pszFileName);
+    FIXME("(%p, %p) semi-stub \n", This, pszFileName);
 
-    return E_NOTIMPL;
+    /* FIXME: Add a notification to the object that it can write to its file */
+
+    return S_OK;
 }
 
 static const IPersistFileVtbl PZIPersistFileVtbl = {
@@ -236,31 +311,66 @@ static ULONG WINAPI PZIZoneId_Release(IZoneIdentifier *iface)
     return IUnknown_Release(This->outer);
 }
 
+
+static BOOL is_trusted_zone(URLZONE zone)
+{
+    switch (zone)
+    {
+    case URLZONE_LOCAL_MACHINE:
+    case URLZONE_INTRANET:
+    case URLZONE_TRUSTED:
+    case URLZONE_INTERNET:
+    case URLZONE_UNTRUSTED:
+        return TRUE;
+    default:
+        return FALSE;
+    }
+}
+
+
 static HRESULT WINAPI PZIZoneId_GetId(IZoneIdentifier* iface, DWORD* pdwZone)
 {
     PersistentZoneIdentifier *This = impl_from_IZoneIdentifier(iface);
 
-    FIXME("(%p, %p) not implemented\n", This, pdwZone);
+    TRACE("(%p, %p)\n", This, pdwZone);
 
-    return E_NOTIMPL;
+    if (This->is_zone_removed)
+    {
+        *pdwZone = URLZONE_UNTRUSTED;
+        return HRESULT_FROM_WIN32(ERROR_NOT_FOUND);
+    } else if (!is_trusted_zone(This->zone))
+    {
+        *pdwZone = URLZONE_UNTRUSTED;
+        return E_ACCESSDENIED;
+    } else
+    {
+        *pdwZone = This->zone;
+        return S_OK;
+    }
 }
 
 static HRESULT WINAPI PZIZoneId_Remove(IZoneIdentifier* iface)
 {
     PersistentZoneIdentifier *This = impl_from_IZoneIdentifier(iface);
 
-    FIXME("(%p) not implemented\n", This);
+    TRACE("(%p)\n", This);
 
-    return E_NOTIMPL;
+    This->zone = URLZONE_LOCAL_MACHINE;
+    This->is_zone_removed = TRUE;
+
+    return S_OK;
 }
 
 static HRESULT WINAPI PZIZoneId_SetId(IZoneIdentifier* iface, DWORD dwZone)
 {
     PersistentZoneIdentifier *This = impl_from_IZoneIdentifier(iface);
 
-    FIXME("(%p, 0x%08lx) not implemented\n", This, dwZone);
+    TRACE("(%p, 0x%08lx)\n", This, dwZone);
 
-    return E_NOTIMPL;
+    This->zone = dwZone;
+    This->is_zone_removed = FALSE;
+
+    return is_trusted_zone(This->zone) ? S_OK : E_INVALIDARG;
 }
 
 static const IZoneIdentifierVtbl PZIZoneIdVtbl = {
@@ -290,6 +400,12 @@ HRESULT PersistentZoneIdentifier_Construct(IUnknown *outer, LPVOID *ppobj)
     ret->IPersistFile_iface.lpVtbl = &PZIPersistFileVtbl;
     ret->IZoneIdentifier_iface.lpVtbl = &PZIZoneIdVtbl;
 
+    ret->save_file_name = NULL;
+    ret->load_file_name = NULL;
+
+    ret->zone = URLZONE_INVALID;
+    ret->is_zone_removed = TRUE;
+
     ret->ref = 1;
     ret->outer = outer ? outer : &ret->IUnknown_inner;
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/8459

[PATCH v8 2/2] urlmon: Add PersistentZoneIdentifier test cases

[Mike Kozelkov]

From: Mike Kozelkov <augenzi(a)etersoft.ru>

---
 dlls/urlmon/tests/sec_mgr.c | 466 ++++++++++++++++++++++++++++++++++++
 1 file changed, 466 insertions(+)

diff --git a/dlls/urlmon/tests/sec_mgr.c b/dlls/urlmon/tests/sec_mgr.c
index e98a7d19a83..eab15f797a2 100644
--- a/dlls/urlmon/tests/sec_mgr.c
+++ b/dlls/urlmon/tests/sec_mgr.c
@@ -35,6 +35,8 @@
 
 #define URLZONE_CUSTOM  URLZONE_USER_MIN+1
 #define URLZONE_CUSTOM2 URLZONE_CUSTOM+1
+#define URLZONE_CUSTOM3 URLZONE_UNTRUSTED + 1
+#define URLZONE_CUSTOM4 URLZONE_USER_MAX + 1
 
 #define DEFINE_EXPECT(func) \
     static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
@@ -1951,6 +1953,468 @@ static void test_CoInternetIsFeatureZoneElevationEnabled(void)
     }
 }
 
+static void test_uninitialized_zone_identifier(IPersistFile *persist_file, IZoneIdentifier *zone_id)
+{
+    HRESULT hres;
+    DWORD zone;
+    CLSID clsid;
+    LPWSTR file_name;
+
+    hres = IZoneIdentifier_GetId(zone_id, &zone);
+    ok(hres == HRESULT_FROM_WIN32(ERROR_NOT_FOUND),
+        "Unexpected GetId result: 0x%08lx, expected result: 0x%08lx\n",
+        hres, HRESULT_FROM_WIN32(ERROR_NOT_FOUND));
+    ok(zone == URLZONE_UNTRUSTED,
+        "Unexpected GetId default zone: 0x%08lx, expected zone: 0x%08x\n", zone, URLZONE_UNTRUSTED);
+
+    hres = IPersistFile_GetClassID(persist_file, &clsid);
+    ok(hres == S_OK,
+        "Unexpected GetClassId result: 0x%08lx, expected result: 0x%08lx\n",
+        hres, S_OK);
+    ok(IsEqualCLSID(&clsid, &CLSID_PersistentZoneIdentifier),
+        "Unexpected GetClassId id: %s, expected class id: %s\n",
+        debugstr_guid(&clsid), debugstr_guid(&CLSID_PersistentZoneIdentifier));
+
+    hres = IPersistFile_GetCurFile(persist_file, &file_name);
+    ok(hres == E_NOTIMPL,
+        "Unexpected GetCurFile result: 0x%08lx, expected result: 0x%08lx\n", hres, E_NOTIMPL);
+    ok(!lstrcmpW(file_name, NULL),
+        "Unexpected GetCurFile file name: %s, expected file name: %s\n",
+        debugstr_w(file_name), debugstr_w(NULL));
+    if (hres == S_OK || hres == S_FALSE)
+        free(file_name);
+
+    hres = IPersistFile_IsDirty(persist_file);
+    ok(hres == S_OK,
+        "Unexpected IsDirty result: 0x%08lx, expected result: 0x%08lx\n", hres, S_OK);
+
+    hres = IZoneIdentifier_SetId(zone_id, URLZONE_INTERNET);
+    ok(hres == S_OK,
+        "Unexpected SetId result: 0x%08lx, expected result: 0x%08lx\n", hres, S_OK);
+
+    hres = IPersistFile_IsDirty(persist_file);
+    ok(hres == S_OK,
+        "Unexpected IsDirty result after SetId: 0x%08lx, expected result: 0x%08lx\n", hres, S_OK);
+
+    hres = IPersistFile_Save(persist_file, NULL, FALSE);
+    ok(hres == E_INVALIDARG,
+        "Unexpected Save result: 0x%08lx, expected result: 0x%08lx\n", hres, E_INVALIDARG);
+
+    hres = IPersistFile_Load(persist_file, NULL, STGM_READ);
+    ok(hres == E_INVALIDARG,
+        "Unexpected Load result: 0x%08lx, expected result: 0x%08lx\n", hres, E_INVALIDARG);
+}
+
+typedef struct _zone_id_op_test {
+    URLZONE id;
+    HRESULT hres;
+} zone_id_op_test;
+
+typedef struct _zone_id_test {
+    zone_id_op_test set;
+    zone_id_op_test get;
+} zone_id_test;
+
+static const zone_id_test zone_id_tests[] = {
+    {
+        { URLZONE_INVALID, E_INVALIDARG },
+        { URLZONE_UNTRUSTED, E_ACCESSDENIED }
+    },
+    {
+        { URLZONE_LOCAL_MACHINE, S_OK },
+        { URLZONE_LOCAL_MACHINE, S_OK }
+    },
+    {
+        { URLZONE_INTRANET, S_OK },
+        { URLZONE_INTRANET, S_OK }
+    },
+    {
+        { URLZONE_TRUSTED, S_OK },
+        { URLZONE_TRUSTED, S_OK }
+    },
+    {
+        { URLZONE_INTERNET, S_OK },
+        { URLZONE_INTERNET, S_OK }
+    },
+    {
+        { URLZONE_UNTRUSTED, S_OK },
+        { URLZONE_UNTRUSTED, S_OK }
+    },
+    {
+        { URLZONE_CUSTOM3, E_INVALIDARG },
+        { URLZONE_UNTRUSTED, E_ACCESSDENIED }
+    },
+    {
+        { URLZONE_USER_MIN, E_INVALIDARG },
+        { URLZONE_UNTRUSTED, E_ACCESSDENIED }
+    },
+    {
+        { URLZONE_USER_MAX, E_INVALIDARG },
+        { URLZONE_UNTRUSTED, E_ACCESSDENIED }
+    },
+    {
+        { URLZONE_CUSTOM4, E_INVALIDARG },
+        { URLZONE_UNTRUSTED, E_ACCESSDENIED }
+    }
+};
+
+static void test_IZoneIdentifier_iface(IZoneIdentifier *zone_id)
+{
+    zone_id_op_test set;
+    zone_id_op_test get;
+    HRESULT hres;
+    DWORD i, zone;
+
+    for(i = 0; i < ARRAY_SIZE(zone_id_tests); ++i)
+    {
+        set = zone_id_tests[i].set;
+        get = zone_id_tests[i].get;
+
+        hres = IZoneIdentifier_SetId(zone_id, set.id);
+        ok(hres == set.hres,
+           "%lu) Unexpected SetId result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, set.hres);
+
+        hres = IZoneIdentifier_GetId(zone_id, &zone);
+        ok(hres == get.hres,
+            "%lu) Unexpected GetId id result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, get.hres);
+        ok(zone == get.id,
+            "%lu) Unexpected GetId zone: 0x%08lx, expected zone: 0x%08x\n",
+            i, zone, get.id);
+    }
+
+    hres = IZoneIdentifier_Remove(zone_id);
+    ok(hres == S_OK,
+        "Unexpected Remove result: 0x%08lx, expected result: 0x%08lx\n",
+        hres, S_OK);
+
+    hres = IZoneIdentifier_GetId(zone_id, &zone);
+    ok(hres == HRESULT_FROM_WIN32(ERROR_NOT_FOUND),
+        "Unexpected GetId result after Remove: 0x%08lx, expected result: 0x%08lx\n",
+        hres, HRESULT_FROM_WIN32(ERROR_NOT_FOUND));
+    ok(zone == URLZONE_UNTRUSTED,
+        "Unexpected GetId zone after Remove: 0x%08lx, expected zone: 0x%08x\n",
+        zone, URLZONE_UNTRUSTED);
+
+}
+
+typedef struct _get_cur_file_test {
+    LPCWSTR name;
+    HRESULT hres;
+} get_cur_file_test;
+
+typedef struct load_file_test {
+    LPCWSTR name;
+    DWORD mode;
+    HRESULT hres;
+} load_file_test;
+
+typedef struct _save_file_test {
+    LPCWSTR name;
+    BOOL remember;
+    HRESULT hres;
+} save_file_test;
+
+typedef struct _persist_file_test {
+    zone_id_op_test set;
+    save_file_test save;
+    get_cur_file_test cur_file;
+    load_file_test load;
+    zone_id_op_test get;
+} persist_file_test;
+
+static const persist_file_test persist_file_tests[] = {
+    {
+        { URLZONE_UNTRUSTED, S_OK },
+        { NULL, FALSE, E_INVALIDARG },
+        { NULL, E_NOTIMPL },
+        { NULL, 0, E_INVALIDARG },
+        { URLZONE_UNTRUSTED, __HRESULT_FROM_WIN32(ERROR_NOT_FOUND) }
+    },
+    {
+        { URLZONE_INTERNET, S_OK },
+        { L"00000000-0000-0000-0000-000000000000", FALSE, S_OK },
+        { NULL, E_NOTIMPL },
+        { L"00000000-0000-0000-0000-000000000000", STGM_READWRITE | STGM_SHARE_DENY_NONE, S_OK },
+        { URLZONE_INTERNET, S_OK }
+    },
+    {
+        { URLZONE_LOCAL_MACHINE, S_OK },
+        { L"00000000-0000-0000-0000-000000000000", TRUE, S_OK },
+        { NULL, E_NOTIMPL },
+        { L"00000000-0000-0000-0000-000000000000", STGM_READWRITE | STGM_SHARE_DENY_NONE, S_OK },
+        { URLZONE_LOCAL_MACHINE, S_OK }
+    },
+    {
+        { URLZONE_TRUSTED, S_OK },
+        { NULL, FALSE, S_OK },
+        { NULL, E_NOTIMPL },
+        { L"00000000-0000-0000-0000-000000000000", STGM_READWRITE | STGM_SHARE_DENY_NONE, S_OK },
+        { URLZONE_TRUSTED, S_OK }
+    },
+    {
+        { URLZONE_INTRANET, S_OK },
+        { NULL, TRUE, S_OK },
+        { NULL, E_NOTIMPL },
+        { L"00000000-0000-0000-0000-000000000000", STGM_READWRITE | STGM_SHARE_DENY_NONE, S_OK },
+        { URLZONE_INTRANET, S_OK }
+    },
+    {
+        { URLZONE_INTERNET, S_OK },
+        { L"11111111-1111-1111-1111-111111111111", FALSE, S_OK },
+        { NULL, E_NOTIMPL },
+        { L"11111111-1111-1111-1111-111111111111", STGM_READWRITE | STGM_SHARE_DENY_NONE, S_OK },
+        { URLZONE_INTERNET, S_OK }
+    },
+    {
+        { URLZONE_LOCAL_MACHINE, S_OK },
+        { L"00000000-0000-0000-0000-000000000000", TRUE, S_OK },
+        { NULL, E_NOTIMPL },
+        { NULL, STGM_READ | STGM_SHARE_EXCLUSIVE, __HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) },
+        { URLZONE_UNTRUSTED, __HRESULT_FROM_WIN32(ERROR_NOT_FOUND) }
+    },
+    {
+        { URLZONE_LOCAL_MACHINE, S_OK },
+        { L"00000000-0000-0000-0000-000000000000", TRUE, S_OK },
+        { NULL, E_NOTIMPL },
+        { L"00000000-0000-0000-0000-000000000000", STGM_READ | STGM_SHARE_EXCLUSIVE, S_OK },
+        { URLZONE_LOCAL_MACHINE, S_OK }
+    },
+};
+
+static void remove_dir(const WCHAR* dir_path)
+{
+    WCHAR files_mask[MAX_PATH];
+    WCHAR file_path[MAX_PATH];
+    HANDLE hfiles;
+    WIN32_FIND_DATAW file_data;
+
+    wsprintfW(files_mask, L"%s\\*-*-*-*", dir_path);
+
+    hfiles = FindFirstFileW(files_mask, &file_data);
+    if (hfiles != INVALID_HANDLE_VALUE)
+        do {
+            wsprintfW(file_path, L"%s\\%s", dir_path, file_data.cFileName);
+            DeleteFileW(file_path);
+        } while(FindNextFileW(hfiles, &file_data));
+
+    RemoveDirectoryW(dir_path);
+}
+
+static void test_IPersistFile_iface(IPersistFile *persist_file_save, IZoneIdentifier* zone_id_save,
+    IPersistFile *persist_file_load, IZoneIdentifier *zone_id_load)
+{
+    HRESULT hres;
+    WCHAR tmp_dir[MAX_PATH];
+    WCHAR sub_dir [] = L"PersistentZoneIdentifier";
+    WCHAR tmp_file_path[MAX_PATH];
+    WCHAR *file_path;
+    HANDLE hfile;
+    DWORD i, zone;
+    LPWSTR file_name;
+    persist_file_test test;
+
+    hres = IZoneIdentifier_Remove(zone_id_save);
+    ok(hres == S_OK,
+        "Unexpected Remove result: 0x%08lx, expected result: 0x%08lx\n", hres, S_OK);
+
+    if (!GetTempPathW(sizeof(tmp_dir), tmp_dir))
+    {
+        skip("GetTempPathW failed with: 0x%08lx\n", GetLastError());
+        return;
+    }
+
+    lstrcatW(tmp_dir, sub_dir);
+
+    if (!CreateDirectoryW(tmp_dir, NULL))
+    {
+        skip("CreateDirectoryW failed with: 0x%08lx\n", GetLastError());
+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(persist_file_tests); ++i) {
+        file_name = NULL;
+        file_path = NULL;
+
+        test = persist_file_tests[i];
+        hres = IZoneIdentifier_SetId(zone_id_save, test.set.id);
+        ok(hres == test.set.hres,
+            "%lu) Unexpected SetId result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, test.set.hres);
+
+        hres = IPersistFile_IsDirty(persist_file_save);
+        ok(hres == S_OK,
+            "%lu) Unexpected IsDirty result before Save: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, S_OK);
+
+        if (test.save.name)
+        {
+            wsprintfW(tmp_file_path, L"%s\\%s", tmp_dir, test.save.name);
+            file_path = tmp_file_path;
+        }
+        else
+            file_path = NULL;
+
+        if (file_path)
+        {
+            hfile = CreateFileW(file_path, GENERIC_READ | GENERIC_WRITE,
+                FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+            if (hfile == INVALID_HANDLE_VALUE)
+            {
+                skip("%lu) CreateFileW failed for %s, error: 0x%08lx\n",
+                    i, debugstr_w(file_path), GetLastError());
+                continue;
+            }
+            CloseHandle(hfile);
+        }
+
+        hres = IPersistFile_Save(persist_file_save, file_path, test.save.remember);
+        ok(hres == test.save.hres,
+            "%lu) Unexpected Save result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, test.save.hres);
+
+        hres = IPersistFile_IsDirty(persist_file_save);
+        ok(hres == S_OK,
+            "%lu) Unexpected IsDirty result after Save: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, S_OK);
+
+        if (test.cur_file.name && test.cur_file.hres == S_OK)
+        {
+            wsprintfW(tmp_file_path, L"%s\\%s", tmp_dir, test.cur_file.name);
+            file_path = tmp_file_path;
+        }
+        else
+            file_path = NULL;
+
+        hres = IPersistFile_GetCurFile(persist_file_save, &file_name);
+        ok(hres == test.cur_file.hres,
+            "%lu) Unexpected GetCurFile result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, test.cur_file.hres);
+        ok(!lstrcmpW(file_name, file_path),
+            "%lu) Unexpected GetCurFile file name: %s, expected file name: %s\n",
+            i, debugstr_w(file_name), debugstr_w(file_path));
+
+        if (hres == S_OK || hres == S_FALSE)
+            free(file_name);
+
+        hres = IZoneIdentifier_Remove(zone_id_load);
+        ok(hres == S_OK,
+            "%lu) Unexpected Remove result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, S_OK);
+
+        hres = IPersistFile_IsDirty(persist_file_load);
+        ok(hres == S_OK,
+            "%lu) Unexpected IsDirty result before Load: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, S_OK);
+
+        if (test.load.name)
+        {
+            wsprintfW(tmp_file_path, L"%s\\%s", tmp_dir, test.load.name);
+            file_path = tmp_file_path;
+        }
+        else
+            file_path = NULL;
+
+        hres = IPersistFile_Load(persist_file_load, file_path, test.load.mode);
+        ok(hres == test.load.hres,
+            "%lu) Unexpected Load result: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, test.load.hres);
+
+        hres = IPersistFile_IsDirty(persist_file_load);
+        ok(hres == S_OK,
+            "%lu) Unexpected IsDirty result after Load: 0x%08lx, expected result: 0x%08lx\n",
+            i, hres, S_OK);
+
+        hres = IZoneIdentifier_GetId(zone_id_load, &zone);
+
+        /* GetId checks after expected Load falure shouldn't be marked as todo_wine */
+        todo_wine_if (test.get.hres != __HRESULT_FROM_WIN32(ERROR_NOT_FOUND))
+        {
+            ok(hres == test.get.hres,
+                "%lu) Unexpected GetId result after Load: 0x%08lx, expected result: 0x%08lx\n",
+                i, hres, test.get.hres);
+            ok(zone == test.get.id,
+                "%lu) Unexpected GetId zone after Load: 0x%08lx, expected zone: 0x%08x\n",
+                i, zone, test.get.id);
+        }
+
+        if (test.save.name)
+        {
+            wsprintfW(tmp_file_path, L"%s\\%s", tmp_dir, test.save.name);
+            file_path = tmp_file_path;
+        }
+        else
+            file_path = NULL;
+
+        if (file_path)
+            DeleteFileW(file_path);
+
+    }
+
+    remove_dir(tmp_dir);
+
+}
+
+static void test_PersistentZoneIdentifier(void)
+{
+    IUnknown *unk;
+    IPersistFile *persist_file, *persist_file2;
+    IZoneIdentifier *zone_id, *zone_id2;
+    HRESULT hres;
+
+    trace("Testing uninitialized state of PersistentZoneIdetifier...\n");
+
+    hres = CoCreateInstance(&CLSID_PersistentZoneIdentifier, NULL,
+        CLSCTX_INPROC_SERVER, &IID_IUnknown, (void**)&unk);
+    ok(hres == S_OK, "Failed to obtain IUnknown iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IZoneIdentifier, (void**)&zone_id);
+    ok(hres == S_OK, "Failed to obtain IZoneIdentifier iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IPersistFile, (void**)&persist_file);
+    ok(hres == S_OK, "Failed to obtain IPersistFile iface: 0x%08lx\n", hres);
+
+    test_uninitialized_zone_identifier(persist_file, zone_id);
+
+
+    trace("Testing IZoneIdentifier interface of PersistentZoneIdetifier...\n");
+
+    hres = CoCreateInstance(&CLSID_PersistentZoneIdentifier, NULL,
+        CLSCTX_INPROC_SERVER, &IID_IUnknown, (void**)&unk);
+    ok(hres == S_OK, "Failed to obtain IUnknown iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IZoneIdentifier, (void**)&zone_id);
+    ok(hres == S_OK, "Failed to obtain IZoneIdentifier: 0x%08lx\n", hres);
+
+    test_IZoneIdentifier_iface(zone_id);
+
+
+    trace("Testing IPersistFile interface of PersistentZoneIdetifier...\n");
+
+    hres = CoCreateInstance(&CLSID_PersistentZoneIdentifier, NULL,
+        CLSCTX_INPROC_SERVER, &IID_IUnknown, (void**)&unk);
+    ok(hres == S_OK, "Failed to obtain first IUnknown iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IZoneIdentifier, (void**)&zone_id);
+    ok(hres == S_OK, "Failed to obtain first IZoneIdentifier iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IPersistFile, (void**)&persist_file);
+    ok(hres == S_OK, "Failed to obtain first IPersistFile iface: 0x%08lx\n", hres);
+
+    hres = CoCreateInstance(&CLSID_PersistentZoneIdentifier, NULL,
+        CLSCTX_INPROC_SERVER, &IID_IUnknown, (void**)&unk);
+    ok(hres == S_OK, "Failed to obtain second IUnknown iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IZoneIdentifier, (void**)&zone_id2);
+    ok(hres == S_OK, "Failed to obtain second IZoneIdentifier iface: 0x%08lx\n", hres);
+
+    hres = IUnknown_QueryInterface(unk, &IID_IPersistFile, (void**)&persist_file2);
+    ok(hres == S_OK, "Failed to obtain second IPersistFile iface: 0x%08lx\n", hres);
+
+    test_IPersistFile_iface(persist_file, zone_id, persist_file2, zone_id2);
+}
+
 START_TEST(sec_mgr)
 {
     HMODULE hurlmon;
@@ -2005,6 +2469,8 @@ START_TEST(sec_mgr)
     test_InternetSecurityMarshalling();
     test_CoInternetIsFeatureZoneElevationEnabled();
 
+    test_PersistentZoneIdentifier();
+
     unregister_protocols();
     OleUninitialize();
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8459

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +
> +    remove_dir(tmp_dir);
> +
> +}
> +
> +static void test_PersistentZoneIdentifier(void)
> +{
> +    IUnknown *unk;
> +    IPersistFile *persist_file, *persist_file2;
> +    IZoneIdentifier *zone_id, *zone_id2;
> +    HRESULT hres;
> +
> +    trace("Testing uninitialized state of PersistentZoneIdetifier...\n");
> +
> +    hres = CoCreateInstance(&CLSID_PersistentZoneIdentifier, NULL,
> +        CLSCTX_INPROC_SERVER, &IID_IUnknown, (void**)&unk);
> `unk` is leaked, it needs a corresponding `Release` call. The same is true for all interfaces returned by `QueryInterface`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109914

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +        hres, HRESULT_FROM_WIN32(ERROR_NOT_FOUND));
> +    ok(zone == URLZONE_UNTRUSTED,
> +        "Unexpected GetId default zone: 0x%08lx, expected zone: 0x%08x\n", zone, URLZONE_UNTRUSTED);
> +
> +    hres = IPersistFile_GetClassID(persist_file, &clsid);
> +    ok(hres == S_OK,
> +        "Unexpected GetClassId result: 0x%08lx, expected result: 0x%08lx\n",
> +        hres, S_OK);
> +    ok(IsEqualCLSID(&clsid, &CLSID_PersistentZoneIdentifier),
> +        "Unexpected GetClassId id: %s, expected class id: %s\n",
> +        debugstr_guid(&clsid), debugstr_guid(&CLSID_PersistentZoneIdentifier));
> +
> +    hres = IPersistFile_GetCurFile(persist_file, &file_name);
> +    ok(hres == E_NOTIMPL,
> +        "Unexpected GetCurFile result: 0x%08lx, expected result: 0x%08lx\n", hres, E_NOTIMPL);
> +    ok(!lstrcmpW(file_name, NULL),
> This could be just `!file_name`, there is no need for `lstrcmpW` to check for NULL.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109915

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +            "%lu) Unexpected IsDirty result before Save: 0x%08lx, expected result: 0x%08lx\n",
> +            i, hres, S_OK);
> +
> +        if (test.save.name)
> +        {
> +            wsprintfW(tmp_file_path, L"%s\\%s", tmp_dir, test.save.name);
> +            file_path = tmp_file_path;
> +        }
> +        else
> +            file_path = NULL;
> +
> +        if (file_path)
> +        {
> +            hfile = CreateFileW(file_path, GENERIC_READ | GENERIC_WRITE,
> +                FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> +            if (hfile == INVALID_HANDLE_VALUE)
> This could be `ok()` too.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109920

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +    {
> +        skip("GetTempPathW failed with: 0x%08lx\n", GetLastError());
> +        return;
> +    }
> +
> +    lstrcatW(tmp_dir, sub_dir);
> +
> +    if (!CreateDirectoryW(tmp_dir, NULL))
> +    {
> +        skip("CreateDirectoryW failed with: 0x%08lx\n", GetLastError());
> +        return;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(persist_file_tests); ++i) {
> +        file_name = NULL;
> +        file_path = NULL;
> You initialize `file_path` later, there is no need to initialize it here. Also, I think it would be easier to follow if you moved `file_name` initialization closer to its usage.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109919

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +            wsprintfW(tmp_file_path, L"%s\\%s", tmp_dir, test.cur_file.name);
> +            file_path = tmp_file_path;
> +        }
> +        else
> +            file_path = NULL;
> +
> +        hres = IPersistFile_GetCurFile(persist_file_save, &file_name);
> +        ok(hres == test.cur_file.hres,
> +            "%lu) Unexpected GetCurFile result: 0x%08lx, expected result: 0x%08lx\n",
> +            i, hres, test.cur_file.hres);
> +        ok(!lstrcmpW(file_name, file_path),
> +            "%lu) Unexpected GetCurFile file name: %s, expected file name: %s\n",
> +            i, debugstr_w(file_name), debugstr_w(file_path));
> +
> +        if (hres == S_OK || hres == S_FALSE)
> +            free(file_name);
> This should use `CoTaskMemFree()`, but given that we don't expect `GetCurFile` to succeed, you may just remove it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109921

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +        "Unexpected GetId default zone: 0x%08lx, expected zone: 0x%08x\n", zone, URLZONE_UNTRUSTED);
> +
> +    hres = IPersistFile_GetClassID(persist_file, &clsid);
> +    ok(hres == S_OK,
> +        "Unexpected GetClassId result: 0x%08lx, expected result: 0x%08lx\n",
> +        hres, S_OK);
> +    ok(IsEqualCLSID(&clsid, &CLSID_PersistentZoneIdentifier),
> +        "Unexpected GetClassId id: %s, expected class id: %s\n",
> +        debugstr_guid(&clsid), debugstr_guid(&CLSID_PersistentZoneIdentifier));
> +
> +    hres = IPersistFile_GetCurFile(persist_file, &file_name);
> +    ok(hres == E_NOTIMPL,
> +        "Unexpected GetCurFile result: 0x%08lx, expected result: 0x%08lx\n", hres, E_NOTIMPL);
> +    ok(!lstrcmpW(file_name, NULL),
> +        "Unexpected GetCurFile file name: %s, expected file name: %s\n",
> +        debugstr_w(file_name), debugstr_w(NULL));
> You may just change the error string to "... expected NULL".

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109916

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +        hres, S_OK);
> +
> +    hres = IZoneIdentifier_GetId(zone_id, &zone);
> +    ok(hres == HRESULT_FROM_WIN32(ERROR_NOT_FOUND),
> +        "Unexpected GetId result after Remove: 0x%08lx, expected result: 0x%08lx\n",
> +        hres, HRESULT_FROM_WIN32(ERROR_NOT_FOUND));
> +    ok(zone == URLZONE_UNTRUSTED,
> +        "Unexpected GetId zone after Remove: 0x%08lx, expected zone: 0x%08x\n",
> +        zone, URLZONE_UNTRUSTED);
> +
> +}
> +
> +typedef struct _get_cur_file_test {
> +    LPCWSTR name;
> +    HRESULT hres;
> +} get_cur_file_test;
> It's similar to `IsDirty`, if it always returns `E_NOTIMPL` there is no need to specify that for each test, you could just change the test to always check for `E_NOTIMPL`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109917

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/tests/sec_mgr.c:
> +    WCHAR tmp_file_path[MAX_PATH];
> +    WCHAR *file_path;
> +    HANDLE hfile;
> +    DWORD i, zone;
> +    LPWSTR file_name;
> +    persist_file_test test;
> +
> +    hres = IZoneIdentifier_Remove(zone_id_save);
> +    ok(hres == S_OK,
> +        "Unexpected Remove result: 0x%08lx, expected result: 0x%08lx\n", hres, S_OK);
> +
> +    if (!GetTempPathW(sizeof(tmp_dir), tmp_dir))
> +    {
> +        skip("GetTempPathW failed with: 0x%08lx\n", GetLastError());
> +        return;
> +    }
> `GetTempPathW` is not expected to fail. You may verify that with `ok()`, but there is no need for `skip()`. It's similar for `CreateDirectoryW`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109918

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/zone_id.c:
> return IUnknown_Release(This->outer);
>  }
>
> +
> +static BOOL is_trusted_zone(URLZONE zone)

The name is a bit misleading given that `URLZONE_UNTRUSTED` returns true. Maybe something like `is_known_zone()`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109924

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/zone_id.c:
> +
> +    hfile = CreateFileW(file_name, GENERIC_READ | GENERIC_WRITE,
> +        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_ALWAYS,
> +        FILE_ATTRIBUTE_NORMAL, NULL);
> +    if (hfile == INVALID_HANDLE_VALUE)
> +        return HRESULT_FROM_WIN32(GetLastError());
> +
> +    CloseHandle(hfile);
> +
> +    if (remember)
> +    {
> +        new_file_name = wcsdup(file_name);
> +        if (!new_file_name)
> +            return E_OUTOFMEMORY;
> +
> +        if (This->save_file_name)
> Same here, no need for the NULL check.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109923

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/urlmon/zone_id.c:
> +    }
> +
> +    /* FIXME: Read zone information from Zone.Identifier NTFS alternate data stream */
> +
> +    hfile = CreateFileW(file_name, GENERIC_READ,
> +        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING,
> +        FILE_ATTRIBUTE_NORMAL, NULL);
> +    if (hfile == INVALID_HANDLE_VALUE)
> +        return HRESULT_FROM_WIN32(GetLastError());
> +
> +    CloseHandle(hfile);
> +
> +    if (lstrcmpW(This->load_file_name, file_name))
> +    {
> +        if (This->load_file_name)
> +            free(This->load_file_name);
> There is no need for the NULL check prior to `free()` call.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109922

[Mike Kozelkov (＠augenzi)]

On Tue Jul 15 10:02:09 2025 +0000, Jacek Caban wrote:
> This could be `ok()` too.
> If `CreateFileW()` call failed, should we skip test case iteration?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_109947

[Jacek Caban (＠jacek)]

On Tue Jul 15 15:31:38 2025 +0000, Mike Kozelkov wrote:
> If `CreateFileW()` call failed, should we skip test case iteration? I
> think it should be skipped because all sequence Save->Load->GetId will fail.
> Unlike the implementation, tests generally don’t need to handle every theoretical failure case. Whether one test fails or several, the result is the same, it indicates something that needs fixing.

The problem with using `skip()` is that it allows issues to go unnoticed. It prints a message and leaves a trace, but CI still treats it as a success. `skip()` is useful when detecting configurations that genuinely prevent a test from running, but if we can’t create a temporary file, that seems more like a broken environment than a valid configuration from the test’s perspective.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8459#note_110053