Re: [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  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.