[RESEND]MSI:ACTION_AppSearchReg() should return backslash terminated paths - Solves bug 13838

[James Hawkins]

On Mon, Jul 7, 2008 at 10:04 AM, Massimo Del Fedele <max at veneto.com> wrote:
> I'll repost that one... I'd like some feedback if it looks wrong.
>

This isn't going to be committed without a test.

> To maintain consistence between global MSI path properties (as
> CommonAppDataFolder) which are returned with a trailing backslash, and
> local MSI path properties (as LOCALAPPDATAFOLDER) which by now where
> returned without the trailing backslash.

Properties in MSI are just that...properties.  Folders loaded from the
Folder table just happen to be stored as properties.  The AppSearch
action looks for a path (in this case) and sets a user-defined
property to the path of the directory.  You're making a connection
between these two completely separated sets of functionality that
doesn't exist.

> This solves autocad's installer bug 13838.
>

Ok, we've had tons of patches submitted, some committed, that fix one
app and break tons of other apps.  This is exactly why this has to be
backed up by a test.

I'll review your patch for you anyway:

@@ -368,6 +368,17 @@ static UINT ACTION_AppSearchReg(MSIPACKAGE
*package, LPWSTR *appValue, MSISIGNAT
     switch (type & 0x0f)
     {
     case msidbLocatorTypeDirectory:
+	/* directories should be returned with a trailing backslash
+           so check for it and append if necessary */

Leave the comment out.

+        if((LPWSTR)value[sz-1] != L'\\')
+        {

Two things wrong.  That first part is casting the result of
value[sz-1], which is a BYTE (integer), to LPWSTR (pointer).  Second,
you can't use L in Wine.

+            *appValue = (LPWSTR)value;
+            value = msi_alloc(sz+1);

You forgot to check if you're out of memory.

+            lstrcpyW((LPWSTR)value, *appValue);
+            lstrcatW(value, L"\\");

Again, can't use L in Wine.

+            msi_free(*appValue);
+            *appValue = NULL;
+        }
         rc = ACTION_SearchDirectory(package, sig, (LPWSTR)value, 0, appValue);
         break;
     case msidbLocatorTypeFileName:

Now let's look at the whole thing:

            *appValue = (LPWSTR)value;
            value = msi_alloc(sz+1);
            lstrcpyW((LPWSTR)value, *appValue);
            lstrcatW(value, L"\\");
            msi_free(*appValue);
            *appValue = NULL;

You're using *appValue as a temporary pointer, which is ugly.
msi_realloc is your friend.

Check out the warnings you get when you compile this, which is the
first sign that something is wrong:

appsearch.c: In function 'ACTION_AppSearchReg':
appsearch.c:373: warning: cast to pointer from integer of different size
appsearch.c:373: warning: comparison between pointer and integer
appsearch.c:378: warning: passing argument 1 of 'lstrcatW' from
incompatible pointer type
appsearch.c:378: warning: passing argument 2 of 'lstrcatW' from
incompatible pointer type

Taking one more step back:

@@ -368,6 +368,17 @@ static UINT ACTION_AppSearchReg(MSIPACKAGE
*package, LPWSTR *appValue, MSISIGNAT
     switch (type & 0x0f)
     {
     case msidbLocatorTypeDirectory:
+	/* directories should be returned with a trailing backslash
+           so check for it and append if necessary */

So this 'fix' is only for ACTION_AppSearchReg.  What about all the
other AppSearch functionalities that also search for a directory?
Those will remain unfixed.  Also, why are you making it the
responsibility of AppSearchReg to add the backslash?  You're assuming
that ACTION_SearchDirectory never changes the value sent in and only
copies it straight back if it finds the path.  That's a bad
assumption.  Plus, if you fix this at the lowest level, you get a fix
for all the AppSearch functionalities for free.

-- 
James Hawkins