[PATCH 1/6] msi: Make MsiGetProperty() RPC-compatible.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/msi/custom.c    | 11 ++++++++++
 dlls/msi/package.c   | 59 ++++++++++++++++++----------------------------------
 dlls/msi/winemsi.idl |  2 +-
 3 files changed, 32 insertions(+), 40 deletions(-)

diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c
index e2ed5a4..83b1687 100644
--- a/dlls/msi/custom.c
+++ b/dlls/msi/custom.c
@@ -34,6 +34,7 @@
 
 #include "msipriv.h"
 #include "winemsi.h"
+#include "wine/heap.h"
 #include "wine/debug.h"
 #include "wine/unicode.h"
 #include "wine/exception.h"
@@ -64,6 +65,16 @@ static CRITICAL_SECTION msi_custom_action_cs = { &msi_custom_action_cs_debug, -1
 
 static struct list msi_pending_custom_actions = LIST_INIT( msi_pending_custom_actions );
 
+void  __RPC_FAR * __RPC_USER MIDL_user_allocate(SIZE_T len)
+{
+    return heap_alloc(len);
+}
+
+void __RPC_USER MIDL_user_free(void __RPC_FAR * ptr)
+{
+    heap_free(ptr);
+}
+
 UINT msi_schedule_action( MSIPACKAGE *package, UINT script, const WCHAR *action )
 {
     UINT count;
diff --git a/dlls/msi/package.c b/dlls/msi/package.c
index 4e1ac2f..86975e0 100644
--- a/dlls/msi/package.c
+++ b/dlls/msi/package.c
@@ -2396,52 +2396,23 @@ static UINT MSI_GetProperty( MSIHANDLE handle, LPCWSTR name,
     package = msihandle2msiinfo( handle, MSIHANDLETYPE_PACKAGE );
     if (!package)
     {
-        HRESULT hr;
         LPWSTR value = NULL;
         MSIHANDLE remote;
-        BSTR bname;
 
         if (!(remote = msi_get_remote(handle)))
             return ERROR_INVALID_HANDLE;
 
-        bname = SysAllocString( name );
-        if (!bname)
-            return ERROR_OUTOFMEMORY;
+        r = remote_GetProperty(remote, name, &value);
+        if (r != ERROR_SUCCESS)
+            return r;
 
-        hr = remote_GetProperty(remote, bname, NULL, &len);
-        if (FAILED(hr))
-            goto done;
-
-        len++;
-        value = msi_alloc(len * sizeof(WCHAR));
-        if (!value)
-        {
-            r = ERROR_OUTOFMEMORY;
-            goto done;
-        }
-
-        hr = remote_GetProperty(remote, bname, value, &len);
-        if (FAILED(hr))
-            goto done;
-
-        r = msi_strcpy_to_awstring( value, len, szValueBuf, pchValueBuf );
+        r = msi_strcpy_to_awstring(value, -1, szValueBuf, pchValueBuf);
 
         /* Bug required by Adobe installers */
-        if (!szValueBuf->unicode && !szValueBuf->str.a)
+        if (pchValueBuf && !szValueBuf->unicode && !szValueBuf->str.a)
             *pchValueBuf *= sizeof(WCHAR);
 
-done:
-        SysFreeString(bname);
-        msi_free(value);
-
-        if (FAILED(hr))
-        {
-            if (HRESULT_FACILITY(hr) == FACILITY_WIN32)
-                return HRESULT_CODE(hr);
-
-            return ERROR_FUNCTION_FAILED;
-        }
-
+        midl_user_free(value);
         return r;
     }
 
@@ -2498,11 +2469,21 @@ HRESULT __cdecl remote_GetActiveDatabase(MSIHANDLE hinst, MSIHANDLE *handle)
     return S_OK;
 }
 
-HRESULT __cdecl remote_GetProperty(MSIHANDLE hinst, BSTR property, BSTR value, DWORD *size)
+UINT __cdecl remote_GetProperty(MSIHANDLE hinst, LPCWSTR property, LPWSTR *value)
 {
-    UINT r = MsiGetPropertyW(hinst, property, value, size);
-    if (r != ERROR_SUCCESS) return HRESULT_FROM_WIN32(r);
-    return S_OK;
+    WCHAR empty[1];
+    DWORD size = 0;
+    UINT r;
+
+    r = MsiGetPropertyW(hinst, property, empty, &size);
+    if (r == ERROR_MORE_DATA)
+    {
+        *value = midl_user_allocate(++size * sizeof(WCHAR));
+        if (!*value)
+            return ERROR_OUTOFMEMORY;
+        r = MsiGetPropertyW(hinst, property, *value, &size);
+    }
+    return r;
 }
 
 HRESULT __cdecl remote_SetProperty(MSIHANDLE hinst, BSTR property, BSTR value)
diff --git a/dlls/msi/winemsi.idl b/dlls/msi/winemsi.idl
index 706eb99..5e84ea8 100644
--- a/dlls/msi/winemsi.idl
+++ b/dlls/msi/winemsi.idl
@@ -40,7 +40,7 @@ interface IWineMsiRemote
     HRESULT remote_DatabaseOpenView( [in] MSIHANDLE db, [in] LPCWSTR query, [out] MSIHANDLE *view );
 
     HRESULT remote_GetActiveDatabase( [in] MSIHANDLE hinst, [out] MSIHANDLE *handle );
-    HRESULT remote_GetProperty( [in] MSIHANDLE hinst, [in] BSTR property, [out, size_is(*size)] BSTR value, [in, out] DWORD *size );
+    UINT remote_GetProperty( [in] MSIHANDLE hinst, [in, string] LPCWSTR property, [out, string] LPWSTR *value );
     HRESULT remote_SetProperty( [in] MSIHANDLE hinst, [in] BSTR property, [in] BSTR value );
     HRESULT remote_ProcessMessage( [in] MSIHANDLE hinst, [in] INSTALLMESSAGE message, [in] MSIHANDLE record );
     HRESULT remote_DoAction( [in] MSIHANDLE hinst, [in] BSTR action );
-- 
2.7.4

[PATCH 2/6] msi: Make MsiSetProperty() RPC-compatible.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/msi/package.c   | 31 +++----------------------------
 dlls/msi/winemsi.idl |  2 +-
 2 files changed, 4 insertions(+), 29 deletions(-)

diff --git a/dlls/msi/package.c b/dlls/msi/package.c
index 86975e0..fa498d1 100644
--- a/dlls/msi/package.c
+++ b/dlls/msi/package.c
@@ -2201,35 +2201,11 @@ UINT WINAPI MsiSetPropertyW( MSIHANDLE hInstall, LPCWSTR szName, LPCWSTR szValue
     if( !package )
     {
         MSIHANDLE remote;
-        HRESULT hr;
-        BSTR name = NULL, value = NULL;
 
         if (!(remote = msi_get_remote(hInstall)))
             return ERROR_INVALID_HANDLE;
 
-        name = SysAllocString( szName );
-        value = SysAllocString( szValue );
-        if ((!name && szName) || (!value && szValue))
-        {
-            SysFreeString( name );
-            SysFreeString( value );
-            return ERROR_OUTOFMEMORY;
-        }
-
-        hr = remote_SetProperty(remote, name, value);
-
-        SysFreeString( name );
-        SysFreeString( value );
-
-        if (FAILED(hr))
-        {
-            if (HRESULT_FACILITY(hr) == FACILITY_WIN32)
-                return HRESULT_CODE(hr);
-
-            return ERROR_FUNCTION_FAILED;
-        }
-
-        return ERROR_SUCCESS;
+        return remote_SetProperty(remote, szName, szValue);
     }
 
     ret = msi_set_property( package->db, szName, szValue, -1 );
@@ -2486,10 +2462,9 @@ UINT __cdecl remote_GetProperty(MSIHANDLE hinst, LPCWSTR property, LPWSTR *value
     return r;
 }
 
-HRESULT __cdecl remote_SetProperty(MSIHANDLE hinst, BSTR property, BSTR value)
+UINT __cdecl remote_SetProperty(MSIHANDLE hinst, LPCWSTR property, LPCWSTR value)
 {
-    UINT r = MsiSetPropertyW(hinst, property, value);
-    return HRESULT_FROM_WIN32(r);
+    return MsiSetPropertyW(hinst, property, value);
 }
 
 HRESULT __cdecl remote_ProcessMessage(MSIHANDLE hinst, INSTALLMESSAGE message, MSIHANDLE record)
diff --git a/dlls/msi/winemsi.idl b/dlls/msi/winemsi.idl
index 5e84ea8..efd3971 100644
--- a/dlls/msi/winemsi.idl
+++ b/dlls/msi/winemsi.idl
@@ -41,7 +41,7 @@ interface IWineMsiRemote
 
     HRESULT remote_GetActiveDatabase( [in] MSIHANDLE hinst, [out] MSIHANDLE *handle );
     UINT remote_GetProperty( [in] MSIHANDLE hinst, [in, string] LPCWSTR property, [out, string] LPWSTR *value );
-    HRESULT remote_SetProperty( [in] MSIHANDLE hinst, [in] BSTR property, [in] BSTR value );
+    UINT remote_SetProperty( [in] MSIHANDLE hinst, [in, string, unique] LPCWSTR property, [in, string, unique] LPCWSTR value );
     HRESULT remote_ProcessMessage( [in] MSIHANDLE hinst, [in] INSTALLMESSAGE message, [in] MSIHANDLE record );
     HRESULT remote_DoAction( [in] MSIHANDLE hinst, [in] BSTR action );
     HRESULT remote_Sequence( [in] MSIHANDLE hinst, [in] BSTR table, [in] int sequence );
-- 
2.7.4

[PATCH 2/6] msi: Make MsiSetProperty() RPC-compatible.

[Hans Leidekker]

Signed-off-by: Hans Leidekker <hans(a)codeweavers.com>

[PATCH 3/6] msi/tests: Clean up property tests.

[Zebediah Figura]

Remove many redundant tests, and print relevant values on failure.

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/msi/tests/package.c | 220 +++++++++++++++--------------------------------
 1 file changed, 68 insertions(+), 152 deletions(-)

diff --git a/dlls/msi/tests/package.c b/dlls/msi/tests/package.c
index 2022953..6414891 100644
--- a/dlls/msi/tests/package.c
+++ b/dlls/msi/tests/package.c
@@ -2138,16 +2138,14 @@ static void test_condition(void)
     DeleteFileA(msifile);
 }
 
-static BOOL check_prop_empty( MSIHANDLE hpkg, const char * prop)
+static void check_prop(MSIHANDLE hpkg, const char *prop, const char *expect)
 {
-    UINT r;
-    DWORD sz;
-    char buffer[2];
-
-    sz = sizeof buffer;
-    strcpy(buffer,"x");
-    r = MsiGetPropertyA( hpkg, prop, buffer, &sz );
-    return r == ERROR_SUCCESS && buffer[0] == 0 && sz == 0;
+    char buffer[20] = "x";
+    DWORD sz = sizeof(buffer);
+    UINT r = MsiGetPropertyA(hpkg, prop, buffer, &sz);
+    ok(!r, "'%s': got %u\n", prop, r);
+    ok(sz == lstrlenA(buffer), "'%s': expected %u, got %u\n", prop, lstrlenA(buffer), sz);
+    ok(!strcmp(buffer, expect), "expected '%s', got '%s'\n", expect, buffer);
 }
 
 static void test_props(void)
@@ -2173,150 +2171,120 @@ static void test_props(void)
 
     /* test invalid values */
     r = MsiGetPropertyA( 0, NULL, NULL, NULL );
-    ok( r == ERROR_INVALID_PARAMETER, "wrong return val\n");
+    ok(r == ERROR_INVALID_PARAMETER, "got %u\n", r);
 
     r = MsiGetPropertyA( hpkg, NULL, NULL, NULL );
-    ok( r == ERROR_INVALID_PARAMETER, "wrong return val\n");
+    ok(r == ERROR_INVALID_PARAMETER, "got %u\n", r);
 
     r = MsiGetPropertyA( hpkg, "boo", NULL, NULL );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
+    ok(!r, "got %u\n", r);
 
     r = MsiGetPropertyA( hpkg, "boo", buffer, NULL );
-    ok( r == ERROR_INVALID_PARAMETER, "wrong return val\n");
+    ok(r == ERROR_INVALID_PARAMETER, "got %u\n", r);
 
     /* test retrieving an empty/nonexistent property */
     sz = sizeof buffer;
     r = MsiGetPropertyA( hpkg, "boo", NULL, &sz );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( sz == 0, "wrong size returned\n");
+    ok(!r, "got %u\n", r);
+    ok(sz == 0, "got size %d\n", sz);
 
-    check_prop_empty( hpkg, "boo");
     sz = 0;
     strcpy(buffer,"x");
     r = MsiGetPropertyA( hpkg, "boo", buffer, &sz );
-    ok( r == ERROR_MORE_DATA, "wrong return val\n");
-    ok( !strcmp(buffer,"x"), "buffer was changed\n");
-    ok( sz == 0, "wrong size returned\n");
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!strcmp(buffer,"x"), "got \"%s\"\n", buffer);
+    ok(sz == 0, "got size %u\n", sz);
 
     sz = 1;
     strcpy(buffer,"x");
     r = MsiGetPropertyA( hpkg, "boo", buffer, &sz );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( buffer[0] == 0, "buffer was not changed\n");
-    ok( sz == 0, "wrong size returned\n");
+    ok(!r, "got %u\n", r);
+    ok(!buffer[0], "got \"%s\"\n", buffer);
+    ok(sz == 0, "got size %u\n", sz);
 
     /* set the property to something */
     r = MsiSetPropertyA( 0, NULL, NULL );
-    ok( r == ERROR_INVALID_HANDLE, "wrong return val\n");
+    ok(r == ERROR_INVALID_HANDLE, "got %u\n", r);
 
     r = MsiSetPropertyA( hpkg, NULL, NULL );
-    ok( r == ERROR_INVALID_PARAMETER, "wrong return val\n");
+    ok(r == ERROR_INVALID_PARAMETER, "got %u\n", r);
 
     r = MsiSetPropertyA( hpkg, "", NULL );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
+    ok(!r, "got %u\n", r);
 
-    /* try set and get some illegal property identifiers */
     r = MsiSetPropertyA( hpkg, "", "asdf" );
-    ok( r == ERROR_FUNCTION_FAILED, "wrong return val\n");
+    ok(r == ERROR_FUNCTION_FAILED, "got %u\n", r);
 
     r = MsiSetPropertyA( hpkg, "=", "asdf" );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
+    ok(!r, "got %u\n", r);
+    check_prop(hpkg, "=", "asdf");
 
     r = MsiSetPropertyA( hpkg, " ", "asdf" );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
+    ok(!r, "got %u\n", r);
+    check_prop(hpkg, " ", "asdf");
 
     r = MsiSetPropertyA( hpkg, "'", "asdf" );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-
-    sz = sizeof buffer;
-    buffer[0]=0;
-    r = MsiGetPropertyA( hpkg, "'", buffer, &sz );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( !strcmp(buffer,"asdf"), "buffer was not changed\n");
+    ok(!r, "got %u\n", r);
+    check_prop(hpkg, "'", "asdf");
 
     /* set empty values */
     r = MsiSetPropertyA( hpkg, "boo", NULL );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( check_prop_empty( hpkg, "boo"), "prop wasn't empty\n");
+    ok(!r, "got %u\n", r);
+    check_prop(hpkg, "boo", "");
 
     r = MsiSetPropertyA( hpkg, "boo", "" );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( check_prop_empty( hpkg, "boo"), "prop wasn't empty\n");
+    ok(!r, "got %u\n", r);
+    check_prop(hpkg, "boo", "");
 
     /* set a non-empty value */
     r = MsiSetPropertyA( hpkg, "boo", "xyz" );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
+    ok(!r, "got %u\n", r);
+    check_prop(hpkg, "boo", "xyz");
+
+    r = MsiGetPropertyA(hpkg, "boo", NULL, NULL);
+    ok(!r, "got %u\n", r);
+
+    r = MsiGetPropertyA(hpkg, "boo", buffer, NULL);
+    ok(r == ERROR_INVALID_PARAMETER, "got %u\n", r);
+
+    sz = 0;
+    r = MsiGetPropertyA(hpkg, "boo", NULL, &sz);
+    ok(!r, "got %u\n", r);
+    ok(sz == 3, "got size %u\n", sz);
+
+    sz = 0;
+    strcpy(buffer, "q");
+    r = MsiGetPropertyA(hpkg, "boo", buffer, &sz);
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!strcmp(buffer, "q"), "got \"%s\"", buffer);
+    ok(sz == 3, "got size %u\n", sz);
 
     sz = 1;
     strcpy(buffer,"x");
     r = MsiGetPropertyA( hpkg, "boo", buffer, &sz );
-    ok( r == ERROR_MORE_DATA, "wrong return val\n");
-    ok( buffer[0] == 0, "buffer was not changed\n");
-    ok( sz == 3, "wrong size returned\n");
-
-    sz = 4;
-    strcpy(buffer,"x");
-    r = MsiGetPropertyA( hpkg, "boo", buffer, &sz );
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( !strcmp(buffer,"xyz"), "buffer was not changed\n");
-    ok( sz == 3, "wrong size returned\n");
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!buffer[0], "got \"%s\"\n", buffer);
+    ok(sz == 3, "got size %u\n", sz);
 
     sz = 3;
     strcpy(buffer,"x");
     r = MsiGetPropertyA( hpkg, "boo", buffer, &sz );
-    ok( r == ERROR_MORE_DATA, "wrong return val\n");
-    ok( !strcmp(buffer,"xy"), "buffer was not changed\n");
-    ok( sz == 3, "wrong size returned\n");
-
-    r = MsiSetPropertyA(hpkg, "SourceDir", "foo");
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!strcmp(buffer,"xy"), "got \"%s\"\n", buffer);
+    ok(sz == 3, "got size %u\n", sz);
 
     sz = 4;
-    r = MsiGetPropertyA(hpkg, "SOURCEDIR", buffer, &sz);
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( !strcmp(buffer,""), "buffer wrong\n");
-    ok( sz == 0, "wrong size returned\n");
+    strcpy(buffer,"x");
+    r = MsiGetPropertyA( hpkg, "boo", buffer, &sz );
+    ok(!r, "got %u\n", r);
+    ok(!strcmp(buffer,"xyz"), "got \"%s\"\n", buffer);
+    ok(sz == 3, "got size %u\n", sz);
 
-    sz = 4;
-    r = MsiGetPropertyA(hpkg, "SOMERANDOMNAME", buffer, &sz);
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( !strcmp(buffer,""), "buffer wrong\n");
-    ok( sz == 0, "wrong size returned\n");
+    /* properties are case-sensitive */
+    check_prop(hpkg, "BOO", "");
 
-    sz = 4;
-    r = MsiGetPropertyA(hpkg, "SourceDir", buffer, &sz);
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-    ok( !strcmp(buffer,"foo"), "buffer wrong\n");
-    ok( sz == 3, "wrong size returned\n");
-
-    r = MsiSetPropertyA(hpkg, "MetadataCompName", "Photoshop.dll");
-    ok( r == ERROR_SUCCESS, "wrong return val\n");
-
-    sz = 0;
-    r = MsiGetPropertyA(hpkg, "MetadataCompName", NULL, &sz );
-    ok( r == ERROR_SUCCESS, "return wrong\n");
-    ok( sz == 13, "size wrong (%d)\n", sz);
-
-    sz = 13;
-    r = MsiGetPropertyA(hpkg, "MetadataCompName", buffer, &sz );
-    ok( r == ERROR_MORE_DATA, "return wrong\n");
-    ok( !strcmp(buffer,"Photoshop.dl"), "buffer wrong\n");
-
-    r = MsiSetPropertyA(hpkg, "property", "value");
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-
-    sz = 6;
-    r = MsiGetPropertyA(hpkg, "property", buffer, &sz);
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-    ok( !strcmp(buffer, "value"), "Expected value, got %s\n", buffer);
-
-    r = MsiSetPropertyA(hpkg, "property", NULL);
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-
-    sz = 6;
-    r = MsiGetPropertyA(hpkg, "property", buffer, &sz);
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-    ok(!buffer[0], "Expected empty string, got %s\n", buffer);
+    /* properties set in Property table should work */
+    check_prop(hpkg, "MetadataCompName", "Photoshop.dll");
 
     MsiCloseHandle( hpkg );
     DeleteFileA(msifile);
@@ -3779,57 +3747,6 @@ static void test_states(void)
     DeleteFileA(msifile4);
 }
 
-static void test_getproperty(void)
-{
-    MSIHANDLE hPackage = 0;
-    char prop[100];
-    static CHAR empty[] = "";
-    DWORD size;
-    UINT r;
-
-    r = package_from_db(create_package_db(), &hPackage);
-    if (r == ERROR_INSTALL_PACKAGE_REJECTED)
-    {
-        skip("Not enough rights to perform tests\n");
-        DeleteFileA(msifile);
-        return;
-    }
-    ok( r == ERROR_SUCCESS, "Failed to create package %u\n", r );
-
-    /* set the property */
-    r = MsiSetPropertyA(hPackage, "Name", "Value");
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-
-    /* retrieve the size, NULL pointer */
-    size = 0;
-    r = MsiGetPropertyA(hPackage, "Name", NULL, &size);
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-    ok( size == 5, "Expected 5, got %d\n", size);
-
-    /* retrieve the size, empty string */
-    size = 0;
-    r = MsiGetPropertyA(hPackage, "Name", empty, &size);
-    ok( r == ERROR_MORE_DATA, "Expected ERROR_MORE_DATA, got %d\n", r);
-    ok( size == 5, "Expected 5, got %d\n", size);
-
-    /* don't change size */
-    r = MsiGetPropertyA(hPackage, "Name", prop, &size);
-    ok( r == ERROR_MORE_DATA, "Expected ERROR_MORE_DATA, got %d\n", r);
-    ok( size == 5, "Expected 5, got %d\n", size);
-    ok( !lstrcmpA(prop, "Valu"), "Expected Valu, got %s\n", prop);
-
-    /* increase the size by 1 */
-    size++;
-    r = MsiGetPropertyA(hPackage, "Name", prop, &size);
-    ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", r);
-    ok( size == 5, "Expected 5, got %d\n", size);
-    ok( !lstrcmpA(prop, "Value"), "Expected Value, got %s\n", prop);
-
-    r = MsiCloseHandle( hPackage);
-    ok( r == ERROR_SUCCESS , "Failed to close package\n" );
-    DeleteFileA(msifile);
-}
-
 static void test_removefiles(void)
 {
     MSIHANDLE hpkg;
@@ -9723,7 +9640,6 @@ START_TEST(package)
     test_formatrecord2();
     test_formatrecord_tables();
     test_states();
-    test_getproperty();
     test_removefiles();
     test_appsearch();
     test_appsearch_complocator();
-- 
2.7.4

[PATCH 3/6] msi/tests: Clean up property tests.

[Hans Leidekker]

Signed-off-by: Hans Leidekker <hans(a)codeweavers.com>

[PATCH 4/6] msi: Also null-terminate WCHAR strings.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/msi/install.c       |  4 ++++
 dlls/msi/tests/package.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/dlls/msi/install.c b/dlls/msi/install.c
index 65840e2..814addc 100644
--- a/dlls/msi/install.c
+++ b/dlls/msi/install.c
@@ -180,7 +180,11 @@ UINT msi_strcpy_to_awstring( const WCHAR *str, int len, awstring *awbuf, DWORD *
     if (len < 0) len = strlenW( str );
  
     if (awbuf->unicode && awbuf->str.w)
+    {
         memcpy( awbuf->str.w, str, min(len + 1, *sz) * sizeof(WCHAR) );
+        if (*sz && len >= *sz)
+            awbuf->str.w[*sz - 1] = 0;
+    }
     else
     {
         int lenA = WideCharToMultiByte( CP_ACP, 0, str, len + 1, NULL, 0, NULL, NULL );
diff --git a/dlls/msi/tests/package.c b/dlls/msi/tests/package.c
index 6414891..b85a3a8 100644
--- a/dlls/msi/tests/package.c
+++ b/dlls/msi/tests/package.c
@@ -2150,10 +2150,14 @@ static void check_prop(MSIHANDLE hpkg, const char *prop, const char *expect)
 
 static void test_props(void)
 {
+    static const WCHAR booW[] = {'b','o','o',0};
+    static const WCHAR xyzW[] = {'x','y','z',0};
+    static const WCHAR xyW[] = {'x','y',0};
     MSIHANDLE hpkg, hdb;
     UINT r;
     DWORD sz;
     char buffer[0x100];
+    WCHAR bufferW[10];
 
     hdb = create_package_db();
 
@@ -2280,6 +2284,39 @@ static void test_props(void)
     ok(!strcmp(buffer,"xyz"), "got \"%s\"\n", buffer);
     ok(sz == 3, "got size %u\n", sz);
 
+    sz = 0;
+    r = MsiGetPropertyW(hpkg, booW, NULL, &sz);
+    ok(!r, "got %u\n", r);
+    ok(sz == 3, "got size %u\n", sz);
+
+    sz = 0;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hpkg, booW, bufferW, &sz);
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!lstrcmpW(bufferW, booW), "got %s\n", wine_dbgstr_w(bufferW));
+    ok(sz == 3, "got size %u\n", sz);
+
+    sz = 1;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hpkg, booW, bufferW, &sz );
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!bufferW[0], "got %s\n", wine_dbgstr_w(bufferW));
+    ok(sz == 3, "got size %u\n", sz);
+
+    sz = 3;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hpkg, booW, bufferW, &sz );
+    ok(r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(!lstrcmpW(bufferW, xyW), "got %s\n", wine_dbgstr_w(bufferW));
+    ok(sz == 3, "got size %u\n", sz);
+
+    sz = 4;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hpkg, booW, bufferW, &sz );
+    ok(!r, "got %u\n", r);
+    ok(!lstrcmpW(bufferW, xyzW), "got %s\n", wine_dbgstr_w(bufferW));
+    ok(sz == 3, "got size %u\n", sz);
+
     /* properties are case-sensitive */
     check_prop(hpkg, "BOO", "");
 
-- 
2.7.4

[PATCH 4/6] msi: Also null-terminate WCHAR strings.

[Hans Leidekker]

Signed-off-by: Hans Leidekker <hans(a)codeweavers.com>

[PATCH 5/6] msi/tests: Test properties from custom actions.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/msi/tests/custom.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 180 insertions(+)

diff --git a/dlls/msi/tests/custom.c b/dlls/msi/tests/custom.c
index feb8061..58c0d63 100644
--- a/dlls/msi/tests/custom.c
+++ b/dlls/msi/tests/custom.c
@@ -51,6 +51,184 @@ static void ok_(MSIHANDLE hinst, int todo, const char *file, int line, int condi
 #define ok(hinst, condition, ...)           ok_(hinst, 0, __FILE__, __LINE__, condition, __VA_ARGS__)
 #define todo_wine_ok(hinst, condition, ...) ok_(hinst, 1, __FILE__, __LINE__, condition, __VA_ARGS__)
 
+static const char *dbgstr_w(WCHAR *str)
+{
+    static char buffer[300], *p;
+
+    if (!str) return "(null)";
+
+    p = buffer;
+    *p++ = 'L';
+    *p++ = '"';
+    while ((*p++ = *str++));
+    *p++ = '"';
+    *p++ = 0;
+
+    return buffer;
+}
+
+static void check_prop(MSIHANDLE hinst, const char *prop, const char *expect)
+{
+    char buffer[10] = "x";
+    DWORD sz = sizeof(buffer);
+    UINT r = MsiGetPropertyA(hinst, prop, buffer, &sz);
+    ok(hinst, !r, "'%s': got %u\n", prop, r);
+    ok(hinst, sz == strlen(buffer), "'%s': expected %u, got %u\n", prop, strlen(buffer), sz);
+    ok(hinst, !strcmp(buffer, expect), "expected '%s', got '%s'\n", expect, buffer);
+}
+
+static void test_props(MSIHANDLE hinst)
+{
+    static const WCHAR booW[] = {'b','o','o',0};
+    static const WCHAR xyzW[] = {'x','y','z',0};
+    static const WCHAR xyW[] = {'x','y',0};
+    char buffer[10];
+    WCHAR bufferW[10];
+    DWORD sz;
+    UINT r;
+
+    /* test invalid values */
+    r = MsiGetPropertyA(hinst, NULL, NULL, NULL);
+    ok(hinst, r == ERROR_INVALID_PARAMETER, "got %u\n", r);
+
+    r = MsiGetPropertyA(hinst, "boo", NULL, NULL);
+    ok(hinst, !r, "got %u\n", r);
+
+    r = MsiGetPropertyA(hinst, "boo", buffer, NULL );
+    ok(hinst, r == ERROR_INVALID_PARAMETER, "got %u\n", r);
+
+    sz = 0;
+    r = MsiGetPropertyA(hinst, "boo", NULL, &sz);
+    ok(hinst, !r, "got %u\n", r);
+    ok(hinst, sz == 0, "got size %u\n", sz);
+
+    sz = 0;
+    strcpy(buffer,"x");
+    r = MsiGetPropertyA(hinst, "boo", buffer, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !strcmp(buffer, "x"), "got \"%s\"\n", buffer);
+    ok(hinst, sz == 0, "got size %u\n", sz);
+
+    sz = 1;
+    strcpy(buffer,"x");
+    r = MsiGetPropertyA(hinst, "boo", buffer, &sz);
+    ok(hinst, !r, "got %u\n", r);
+    ok(hinst, !buffer[0], "got \"%s\"\n", buffer);
+    ok(hinst, sz == 0, "got size %u\n", sz);
+
+    /* set the property to something */
+    r = MsiSetPropertyA(hinst, NULL, NULL);
+    ok(hinst, r == ERROR_INVALID_PARAMETER, "got %u\n", r);
+
+    r = MsiSetPropertyA(hinst, "", NULL);
+    ok(hinst, !r, "got %u\n", r);
+
+    r = MsiSetPropertyA(hinst, "", "asdf");
+    ok(hinst, r == ERROR_FUNCTION_FAILED, "got %u\n", r);
+
+    r = MsiSetPropertyA(hinst, "=", "asdf");
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, "=", "asdf");
+
+    r = MsiSetPropertyA(hinst, " ", "asdf");
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, " ", "asdf");
+
+    r = MsiSetPropertyA(hinst, "'", "asdf");
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, "'", "asdf");
+
+    r = MsiSetPropertyA(hinst, "boo", NULL);
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, "boo", "");
+
+    r = MsiSetPropertyA(hinst, "boo", "");
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, "boo", "");
+
+    r = MsiSetPropertyA(hinst, "boo", "xyz");
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, "boo", "xyz");
+
+    r = MsiGetPropertyA(hinst, "boo", NULL, NULL);
+    ok(hinst, !r, "got %u\n", r);
+
+    r = MsiGetPropertyA(hinst, "boo", buffer, NULL );
+    ok(hinst, r == ERROR_INVALID_PARAMETER, "got %u\n", r);
+
+    /* Returned size is in bytes, not chars, but only for custom actions.
+     * Seems to be a casualty of RPC... */
+
+    sz = 0;
+    r = MsiGetPropertyA(hinst, "boo", NULL, &sz);
+    ok(hinst, !r, "got %u\n", r);
+    ok(hinst, sz == 6, "got size %u\n", sz);
+
+    sz = 0;
+    strcpy(buffer,"q");
+    r = MsiGetPropertyA(hinst, "boo", buffer, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !strcmp(buffer, "q"), "got \"%s\"\n", buffer);
+    todo_wine_ok(hinst, sz == 6, "got size %u\n", sz);
+
+    sz = 1;
+    strcpy(buffer,"x");
+    r = MsiGetPropertyA(hinst, "boo", buffer, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !buffer[0], "got \"%s\"\n", buffer);
+    todo_wine_ok(hinst, sz == 6, "got size %u\n", sz);
+
+    sz = 3;
+    strcpy(buffer,"x");
+    r = MsiGetPropertyA(hinst, "boo", buffer, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !strcmp(buffer, "xy"), "got \"%s\"\n", buffer);
+    todo_wine_ok(hinst, sz == 6, "got size %u\n", sz);
+
+    sz = 4;
+    strcpy(buffer,"x");
+    r = MsiGetPropertyA(hinst, "boo", buffer, &sz);
+    ok(hinst, !r, "got %u\n", r);
+    ok(hinst, !strcmp(buffer, "xyz"), "got \"%s\"\n", buffer);
+    ok(hinst, sz == 3, "got size %u\n", sz);
+
+    sz = 0;
+    r = MsiGetPropertyW(hinst, booW, NULL, &sz);
+    ok(hinst, !r, "got %u\n", r);
+    ok(hinst, sz == 3, "got size %u\n", sz);
+
+    sz = 0;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hinst, booW, bufferW, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !lstrcmpW(bufferW, booW), "got %s\n", dbgstr_w(bufferW));
+    ok(hinst, sz == 3, "got size %u\n", sz);
+
+    sz = 1;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hinst, booW, bufferW, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !bufferW[0], "got %s\n", dbgstr_w(bufferW));
+    ok(hinst, sz == 3, "got size %u\n", sz);
+
+    sz = 3;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hinst, booW, bufferW, &sz);
+    ok(hinst, r == ERROR_MORE_DATA, "got %u\n", r);
+    ok(hinst, !lstrcmpW(bufferW, xyW), "got %s\n", dbgstr_w(bufferW));
+    ok(hinst, sz == 3, "got size %u\n", sz);
+
+    sz = 4;
+    lstrcpyW(bufferW, booW);
+    r = MsiGetPropertyW(hinst, booW, bufferW, &sz);
+    ok(hinst, !r, "got %u\n", r);
+    ok(hinst, !lstrcmpW(bufferW, xyzW), "got %s\n", dbgstr_w(bufferW));
+    ok(hinst, sz == 3, "got size %u\n", sz);
+
+    r = MsiSetPropertyA(hinst, "boo", NULL);
+    ok(hinst, !r, "got %u\n", r);
+    check_prop(hinst, "boo", "");
+}
 
 /* Main test. Anything that doesn't depend on a specific install configuration
  * or have undesired side effects should go here. */
@@ -70,6 +248,8 @@ UINT WINAPI main_test(MSIHANDLE hinst)
     res = MsiGetDatabaseState(hinst);
     todo_wine_ok(hinst, res == MSIDBSTATE_ERROR, "expected MSIDBSTATE_ERROR, got %u\n", res);
 
+    test_props(hinst);
+
     return ERROR_SUCCESS;
 }
 
-- 
2.7.4

[PATCH 5/6] msi/tests: Test properties from custom actions.

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?
Full results can be found at
https://testbot.winehq.org/JobDetails.pl?Key=37720

Your paranoid android.


=== w7pro64 (64 bit custom) ===
The previous 1 run(s) terminated abnormally

[PATCH 5/6] msi/tests: Test properties from custom actions.

[Hans Leidekker]

Signed-off-by: Hans Leidekker <hans(a)codeweavers.com>

[PATCH 6/6] msi: Make MsiProcessMessage() RPC-compatible.

[Zebediah Figura]

Instead of passing a remote MSIHANDLE and creating a set of remote_Record*()
methods, we marshal the whole record as a wire struct. We do this for two
reasons: firstly, because chances are whoever is reading the record is going
to want to read the whole thing, so it's much less taxing on IPC to just pass
the whole record once; and secondly, because records can be created on the
client side or returned from the server side, and we don't want to have to
write a lot of extra code to deal with both possibilities.

The wire_record struct is designed so that we can simply pass the relevant
part of an MSIRECORD to the server.

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/msi/msipriv.h   |  2 ++
 dlls/msi/package.c   | 41 +++++++++++++++++++----------------------
 dlls/msi/record.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 dlls/msi/winemsi.idl | 23 ++++++++++++++++++++++-
 4 files changed, 87 insertions(+), 23 deletions(-)

diff --git a/dlls/msi/msipriv.h b/dlls/msi/msipriv.h
index b2b3e7c..00edaa1 100644
--- a/dlls/msi/msipriv.h
+++ b/dlls/msi/msipriv.h
@@ -39,6 +39,7 @@
 #include "wine/debug.h"
 
 #include "msiserver.h"
+#include "winemsi.h"
 
 static const BOOL is_64bit = sizeof(void *) > sizeof(int);
 BOOL is_wow64 DECLSPEC_HIDDEN;
@@ -832,6 +833,7 @@ extern BOOL MSI_RecordsAreFieldsEqual(MSIRECORD *a, MSIRECORD *b, UINT field) DE
 extern UINT msi_record_set_string(MSIRECORD *, UINT, const WCHAR *, int) DECLSPEC_HIDDEN;
 extern const WCHAR *msi_record_get_string(const MSIRECORD *, UINT, int *) DECLSPEC_HIDDEN;
 extern void dump_record(MSIRECORD *) DECLSPEC_HIDDEN;
+extern UINT unmarshal_record(const struct wire_record *in, MSIHANDLE *out) DECLSPEC_HIDDEN;
 
 /* stream internals */
 extern void enum_stream_names( IStorage *stg ) DECLSPEC_HIDDEN;
diff --git a/dlls/msi/package.c b/dlls/msi/package.c
index fa498d1..1aa9fb8 100644
--- a/dlls/msi/package.c
+++ b/dlls/msi/package.c
@@ -2051,39 +2051,27 @@ INT WINAPI MsiProcessMessage( MSIHANDLE hInstall, INSTALLMESSAGE eMessageType,
         MsiRecordGetInteger(hRecord, 1) != 2)
         return -1;
 
+    record = msihandle2msiinfo(hRecord, MSIHANDLETYPE_RECORD);
+    if (!record)
+        return ERROR_INVALID_HANDLE;
+
     package = msihandle2msiinfo( hInstall, MSIHANDLETYPE_PACKAGE );
     if( !package )
     {
         MSIHANDLE remote;
-        HRESULT hr;
 
         if (!(remote = msi_get_remote(hInstall)))
             return ERROR_INVALID_HANDLE;
 
-        hr = remote_ProcessMessage(remote, eMessageType, hRecord);
+        ret = remote_ProcessMessage(remote, eMessageType, (struct wire_record *)&record->count);
 
-        if (FAILED(hr))
-        {
-            if (HRESULT_FACILITY(hr) == FACILITY_WIN32)
-                return HRESULT_CODE(hr);
-
-            return ERROR_FUNCTION_FAILED;
-        }
-
-        return ERROR_SUCCESS;
+        msiobj_release(&record->hdr);
+        return ret;
     }
 
-    record = msihandle2msiinfo( hRecord, MSIHANDLETYPE_RECORD );
-    if( !record )
-        goto out;
-
     ret = MSI_ProcessMessage( package, eMessageType, record );
 
-out:
     msiobj_release( &package->hdr );
-    if( record )
-        msiobj_release( &record->hdr );
-
     return ret;
 }
 
@@ -2467,10 +2455,19 @@ UINT __cdecl remote_SetProperty(MSIHANDLE hinst, LPCWSTR property, LPCWSTR value
     return MsiSetPropertyW(hinst, property, value);
 }
 
-HRESULT __cdecl remote_ProcessMessage(MSIHANDLE hinst, INSTALLMESSAGE message, MSIHANDLE record)
+int __cdecl remote_ProcessMessage(MSIHANDLE hinst, INSTALLMESSAGE message, struct wire_record *remote_rec)
 {
-    UINT r = MsiProcessMessage(hinst, message, record);
-    return HRESULT_FROM_WIN32(r);
+    MSIHANDLE rec;
+    int ret;
+    UINT r;
+
+    if ((r = unmarshal_record(remote_rec, &rec)))
+        return r;
+
+    ret = MsiProcessMessage(hinst, message, rec);
+
+    MsiCloseHandle(rec);
+    return ret;
 }
 
 HRESULT __cdecl remote_DoAction(MSIHANDLE hinst, BSTR action)
diff --git a/dlls/msi/record.c b/dlls/msi/record.c
index de45191..e7f9e13 100644
--- a/dlls/msi/record.c
+++ b/dlls/msi/record.c
@@ -1102,3 +1102,47 @@ void dump_record(MSIRECORD *rec)
     }
     TRACE("]\n");
 }
+
+UINT unmarshal_record(const struct wire_record *in, MSIHANDLE *out)
+{
+    MSIRECORD *rec;
+    unsigned int i;
+    UINT r;
+
+    rec = MSI_CreateRecord(in->count);
+    if (!rec) return ERROR_OUTOFMEMORY;
+
+    for (i = 0; i <= in->count; i++)
+    {
+        switch (in->fields[i].type)
+        {
+        case MSIFIELD_NULL:
+            break;
+        case MSIFIELD_INT:
+        case MSIFIELD_INTPTR:
+            r = MSI_RecordSetInteger(rec, i, in->fields[i].u.iVal);
+            break;
+        case MSIFIELD_WSTR:
+            r = MSI_RecordSetStringW(rec, i, in->fields[i].u.szwVal);
+            break;
+        case MSIFIELD_STREAM:
+            r = MSI_RecordSetIStream(rec, i, in->fields[i].u.stream);
+            break;
+        default:
+            ERR("invalid field type %d\n", in->fields[i].type);
+            break;
+        }
+
+        if (r)
+        {
+            msiobj_release(&rec->hdr);
+            return r;
+        }
+    }
+
+    *out = alloc_msihandle(&rec->hdr);
+    if (!*out) return ERROR_OUTOFMEMORY;
+
+    msiobj_release(&rec->hdr);
+    return ERROR_SUCCESS;
+}
diff --git a/dlls/msi/winemsi.idl b/dlls/msi/winemsi.idl
index efd3971..6490ad3 100644
--- a/dlls/msi/winemsi.idl
+++ b/dlls/msi/winemsi.idl
@@ -26,9 +26,30 @@ typedef int MSICONDITION;
 typedef int MSIRUNMODE;
 typedef int INSTALLSTATE;
 
+#define MSIFIELD_NULL   0
+#define MSIFIELD_INT    1
+#define MSIFIELD_WSTR   3
+#define MSIFIELD_STREAM 4
+#define MSIFIELD_INTPTR 5
 cpp_quote("#endif")
 cpp_quote("#include \"msiquery.h\"")
 
+struct wire_field {
+    unsigned int type;
+    [switch_is(type)] union {
+        [case(MSIFIELD_NULL)] ;
+        [case(MSIFIELD_INT, MSIFIELD_INTPTR)] int iVal;
+        [case(MSIFIELD_WSTR), string] LPWSTR szwVal;
+        [case(MSIFIELD_STREAM)] IStream *stream;
+    } u;
+    int len;
+};
+
+struct wire_record {
+    unsigned int count;
+    [size_is(count+1)] struct wire_field fields[];
+};
+
 [
     uuid(56D58B64-8780-4c22-A8BC-8B0B29E4A9F8)
 ]
@@ -42,7 +63,7 @@ interface IWineMsiRemote
     HRESULT remote_GetActiveDatabase( [in] MSIHANDLE hinst, [out] MSIHANDLE *handle );
     UINT remote_GetProperty( [in] MSIHANDLE hinst, [in, string] LPCWSTR property, [out, string] LPWSTR *value );
     UINT remote_SetProperty( [in] MSIHANDLE hinst, [in, string, unique] LPCWSTR property, [in, string, unique] LPCWSTR value );
-    HRESULT remote_ProcessMessage( [in] MSIHANDLE hinst, [in] INSTALLMESSAGE message, [in] MSIHANDLE record );
+    int remote_ProcessMessage( [in] MSIHANDLE hinst, [in] INSTALLMESSAGE message, [in] struct wire_record *record );
     HRESULT remote_DoAction( [in] MSIHANDLE hinst, [in] BSTR action );
     HRESULT remote_Sequence( [in] MSIHANDLE hinst, [in] BSTR table, [in] int sequence );
     HRESULT remote_GetTargetPath( [in] MSIHANDLE hinst, [in] BSTR folder, [out, size_is(*size)] BSTR value, [in, out] DWORD *size );
-- 
2.7.4

[PATCH 6/6] msi: Make MsiProcessMessage() RPC-compatible.

[Hans Leidekker]

On Mon, 2018-04-16 at 20:20 -0500, Zebediah Figura wrote:
> +UINT unmarshal_record(const struct wire_record *in, MSIHANDLE *out)
> +{
> +    MSIRECORD *rec;
> +    unsigned int i;
> +    UINT r;
> +
> +    rec = MSI_CreateRecord(in->count);
> +    if (!rec) return ERROR_OUTOFMEMORY;
> +
> +    for (i = 0; i <= in->count; i++)
> +    {
> +        switch (in->fields[i].type)
> +        {
> +        case MSIFIELD_NULL:
> +            break;
> +        case MSIFIELD_INT:
> +        case MSIFIELD_INTPTR:
> +            r = MSI_RecordSetInteger(rec, i, in->fields[i].u.iVal);
> +            break;

MSIFIELD_INTPTR is used to store a pointer in MsiViewFetch. We should probably
get rid of that but while we have it I think it would be better to print a
message here and return an error.

[PATCH 6/6] msi: Make MsiProcessMessage() RPC-compatible.

[Zebediah Figura]

On 17/04/18 09:17, Hans Leidekker wrote:
> On Mon, 2018-04-16 at 20:20 -0500, Zebediah Figura wrote:
> > +UINT unmarshal_record(const struct wire_record *in, MSIHANDLE *out)
> > +{
> > +    MSIRECORD *rec;
> > +    unsigned int i;
> > +    UINT r;
> > +
> > +    rec = MSI_CreateRecord(in->count);
> > +    if (!rec) return ERROR_OUTOFMEMORY;
> > +
> > +    for (i = 0; i <= in->count; i++)
> > +    {
> > +        switch (in->fields[i].type)
> > +        {
> > +        case MSIFIELD_NULL:
> > +            break;
> > +        case MSIFIELD_INT:
> > +        case MSIFIELD_INTPTR:
> > +            r = MSI_RecordSetInteger(rec, i, in->fields[i].u.iVal);
> > +            break;
>
> MSIFIELD_INTPTR is used to store a pointer in MsiViewFetch. We should probably
> get rid of that but while we have it I think it would be better to print a
> message here and return an error.

Unless I'm misunderstanding the code, it's necessary for MsiViewModify
to work, and I have tests showing that function works across custom actions.

[PATCH 6/6] msi: Make MsiProcessMessage() RPC-compatible.

[Hans Leidekker]

On Tue, 2018-04-17 at 09:28 -0500, Zebediah Figura wrote:
> On 17/04/18 09:17, Hans Leidekker wrote:
> > On Mon, 2018-04-16 at 20:20 -0500, Zebediah Figura wrote:
> > > +UINT unmarshal_record(const struct wire_record *in, MSIHANDLE *out)
> > > +{
> > > +    MSIRECORD *rec;
> > > +    unsigned int i;
> > > +    UINT r;
> > > +
> > > +    rec = MSI_CreateRecord(in->count);
> > > +    if (!rec) return ERROR_OUTOFMEMORY;
> > > +
> > > +    for (i = 0; i <= in->count; i++)
> > > +    {
> > > +        switch (in->fields[i].type)
> > > +        {
> > > +        case MSIFIELD_NULL:
> > > +            break;
> > > +        case MSIFIELD_INT:
> > > +        case MSIFIELD_INTPTR:
> > > +            r = MSI_RecordSetInteger(rec, i, in->fields[i].u.iVal);
> > > +            break;
> >
> > MSIFIELD_INTPTR is used to store a pointer in MsiViewFetch. We should probably
> > get rid of that but while we have it I think it would be better to print a
> > message here and return an error.
>
> Unless I'm misunderstanding the code, it's necessary for MsiViewModify
> to work, and I have tests showing that function works across custom actions.

The pointer would get truncated in a 64-bit process if you store it like that.
Since this field type is only used to validate the record in MsiViewModify I
think it would be better to get rid of it and find some other way to validate
the record.

[PATCH 6/6] msi: Make MsiProcessMessage() RPC-compatible.

[Zebediah Figura]

On 17/04/18 10:27, Hans Leidekker wrote:
> On Tue, 2018-04-17 at 09:28 -0500, Zebediah Figura wrote:
> > On 17/04/18 09:17, Hans Leidekker wrote:
> > > On Mon, 2018-04-16 at 20:20 -0500, Zebediah Figura wrote:
> > > > +UINT unmarshal_record(const struct wire_record *in, MSIHANDLE *out)
> > > > +{
> > > > +    MSIRECORD *rec;
> > > > +    unsigned int i;
> > > > +    UINT r;
> > > > +
> > > > +    rec = MSI_CreateRecord(in->count);
> > > > +    if (!rec) return ERROR_OUTOFMEMORY;
> > > > +
> > > > +    for (i = 0; i <= in->count; i++)
> > > > +    {
> > > > +        switch (in->fields[i].type)
> > > > +        {
> > > > +        case MSIFIELD_NULL:
> > > > +            break;
> > > > +        case MSIFIELD_INT:
> > > > +        case MSIFIELD_INTPTR:
> > > > +            r = MSI_RecordSetInteger(rec, i, in->fields[i].u.iVal);
> > > > +            break;
> > >
> > > MSIFIELD_INTPTR is used to store a pointer in MsiViewFetch. We should probably
> > > get rid of that but while we have it I think it would be better to print a
> > > message here and return an error.
> >
> > Unless I'm misunderstanding the code, it's necessary for MsiViewModify
> > to work, and I have tests showing that function works across custom actions.
>
> The pointer would get truncated in a 64-bit process if you store it like that.
> Since this field type is only used to validate the record in MsiViewModify I
> think it would be better to get rid of it and find some other way to validate
> the record.

Alright, I'll look into fixing this. Thanks for the review.

[Dmitry Timoshkov]

Zebediah Figura <z.figura12(a)gmail.com> wrote:
> +    r = MsiGetPropertyW(hinst, property, empty, &size);
> +    if (r == ERROR_MORE_DATA)
> +    {
> +        *value = midl_user_allocate(++size * sizeof(WCHAR));

While this construct works as expected could you please avoid this kind
of convoluted thing to make it look less tricky?
> +        if (!*value)
> +            return ERROR_OUTOFMEMORY;
> +        r = MsiGetPropertyW(hinst, property, *value, &size);
> +    }
> +    return r;
>  }

-- 
Dmitry.

[Zebediah Figura]

On 16/04/18 21:20, Dmitry Timoshkov wrote:
> Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > +    r = MsiGetPropertyW(hinst, property, empty, &size);
> > +    if (r == ERROR_MORE_DATA)
> > +    {
> > +        *value = midl_user_allocate(++size * sizeof(WCHAR));
>
> While this construct works as expected could you please avoid this kind
> of convoluted thing to make it look less tricky?

Thanks for the comment. I'm not sure I really see how use of a
preincrement operator is that convoluted, but if it's felt strongly
about I'll change it.

[Dmitry Timoshkov]

Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > +    r = MsiGetPropertyW(hinst, property, empty, &size);
> > > +    if (r == ERROR_MORE_DATA)
> > > +    {
> > > +        *value = midl_user_allocate(++size * sizeof(WCHAR));
> >
> > While this construct works as expected could you please avoid this kind
> > of convoluted thing to make it look less tricky?
>
> Thanks for the comment. I'm not sure I really see how use of a
> preincrement operator is that convoluted, but if it's felt strongly
> about I'll change it.

Probably I should have said not obvious instead, sorry for the "strong"
incantation :)

-- 
Dmitry.

[Hans Leidekker]

On Mon, 2018-04-16 at 20:20 -0500, Zebediah Figura wrote:
> +++ b/dlls/msi/package.c
> @@ -2396,52 +2396,23 @@ static UINT MSI_GetProperty( MSIHANDLE handle, LPCWSTR name,
>      package = msihandle2msiinfo( handle, MSIHANDLETYPE_PACKAGE );
>      if (!package)
>      {
> -        HRESULT hr;
>          LPWSTR value = NULL;
>          MSIHANDLE remote;
> -        BSTR bname;
>  
>          if (!(remote = msi_get_remote(handle)))
>              return ERROR_INVALID_HANDLE;
>  
> -        bname = SysAllocString( name );
> -        if (!bname)
> -            return ERROR_OUTOFMEMORY;
> +        r = remote_GetProperty(remote, name, &value);
> +        if (r != ERROR_SUCCESS)
> +            return r;
>  
> -        hr = remote_GetProperty(remote, bname, NULL, &len);
> -        if (FAILED(hr))
> -            goto done;
> -
> -        len++;
> -        value = msi_alloc(len * sizeof(WCHAR));
> -        if (!value)
> -        {
> -            r = ERROR_OUTOFMEMORY;
> -            goto done;
> -        }
> -
> -        hr = remote_GetProperty(remote, bname, value, &len);
> -        if (FAILED(hr))
> -            goto done;
> -
> -        r = msi_strcpy_to_awstring( value, len, szValueBuf, pchValueBuf );
> +        r = msi_strcpy_to_awstring(value, -1, szValueBuf, pchValueBuf);

Property values can contain embedded nulls. You can't set such values
through MsiSetProperty but MsiGetProperty returns the full value and size
if the property is set by other means. If the remote case behaves the same
you'll have to keep the length parameter.

[Zebediah Figura]

On 17/04/18 07:20, Hans Leidekker wrote:
> On Mon, 2018-04-16 at 20:20 -0500, Zebediah Figura wrote:
> > +++ b/dlls/msi/package.c
> > @@ -2396,52 +2396,23 @@ static UINT MSI_GetProperty( MSIHANDLE handle, LPCWSTR name,
> >      package = msihandle2msiinfo( handle, MSIHANDLETYPE_PACKAGE );
> >      if (!package)
> >      {
> > -        HRESULT hr;
> >          LPWSTR value = NULL;
> >          MSIHANDLE remote;
> > -        BSTR bname;
> >  
> >          if (!(remote = msi_get_remote(handle)))
> >              return ERROR_INVALID_HANDLE;
> >  
> > -        bname = SysAllocString( name );
> > -        if (!bname)
> > -            return ERROR_OUTOFMEMORY;
> > +        r = remote_GetProperty(remote, name, &value);
> > +        if (r != ERROR_SUCCESS)
> > +            return r;
> >  
> > -        hr = remote_GetProperty(remote, bname, NULL, &len);
> > -        if (FAILED(hr))
> > -            goto done;
> > -
> > -        len++;
> > -        value = msi_alloc(len * sizeof(WCHAR));
> > -        if (!value)
> > -        {
> > -            r = ERROR_OUTOFMEMORY;
> > -            goto done;
> > -        }
> > -
> > -        hr = remote_GetProperty(remote, bname, value, &len);
> > -        if (FAILED(hr))
> > -            goto done;
> > -
> > -        r = msi_strcpy_to_awstring( value, len, szValueBuf, pchValueBuf );
> > +        r = msi_strcpy_to_awstring(value, -1, szValueBuf, pchValueBuf);
>
> Property values can contain embedded nulls. You can't set such values
> through MsiSetProperty but MsiGetProperty returns the full value and size
> if the property is set by other means. If the remote case behaves the same
> you'll have to keep the length parameter.

It seems that the remote case returns the correct length but truncates
the value to the first null. I'll send an updated patch.