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

[Massimo Del Fedele]

James Hawkins ha scritto:
> 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.

Why ? there are tons of comments just in appsearch.c
Just to make an example of 'obvious' comment there :

     /* check whether parent is set */
     parentName = msi_dup_record_field(row,2);
     if (parentName)

It would have been better a phrase like "please leave comment out 
because xyzreason". Usually I like to know what and why I should do stuffs.

> 
> +        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).

Right, my mistake... I forgot one parenthesis.

>  Second,
> you can't use L in Wine.

Again... why ? An explanation would bring no harm.
> 
> +            *appValue = (LPWSTR)value;
> +            value = msi_alloc(sz+1);
> 
> You forgot to check if you're out of memory.

Just 3-4 lines above :

     /* FIXME: sanity-check sz before allocating (is there an upper-limit
      * on the value of a property?)
      */
     value = msi_alloc( sz );
     (no check on returned value....)

And, just 3-4 lines below....

     case msidbLocatorTypeFileName:
         *appValue = strdupW((LPWSTR)value);
         break;
     (no check on returned value)

BTW, I agree, it should be checked.
> 
> +            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.
Thanx for the hint, I didn't know msi_realloc.
BTW, that's why I didn't write a testcase, my skills on msi are too limited.
> 
> 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
again, right. My mistake.
> 
> 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?
Because AppSearchReg does search in reg, where paths are un-slashed.
I could have fixed more low-level, of course, but then you'd have big 
probability to break unrelated stuffs.
Again, AppSearchReg MUST return path with backslash. About the other 
AppSearch, I don't know anything.
> Those will remain unfixed.  Also, why are you making it the
> responsibility of AppSearchReg to add the backslash?
Just look at Package.c and you'll know why...
     SHGetFolderPathW(NULL,CSIDL_STARTMENU,NULL,0,pth);
     strcatW(pth,cszbs);
     MSI_SetPropertyW(package, SMF, pth);
as an example.
   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.
Yep, right, I checked it on my case and it didn't rip off the backslash, 
but that could change... I should have done after it.
Plus, if you fix this at the lowest level, you get a fix
> for all the AppSearch functionalities for free.
> 
The "lower level", looking at code, would be change RegQueryValueExW() 
behaviour :
     rc = RegQueryValueExW(key, valueName, NULL, &regType, value, &sz);
which indeed IS correct like ii is, and for that one I DID a testcase. 
That was my first "hack" on this, it worked but WAS WRONG.

Going back one step...

 > 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.

My patch solves just one case that should indeed be solved. I don't say 
it's perfect solution, but that's the best I've found and I know it's 
correct. Maybe it doesn't fix the universe, but it does fix a bug, and 
just THAT bug, without fiddling with other stuffs.
Making it "more low level" as you told, i.e. changing RegQueryValueExW() 
would be wrong and would probably break tons of stuffs that expect it 
(correctly) to return a path WITHOUT a trailing backslash.

But maybe you wanted to say that is ACTION_SearchDirectory() that must 
be patched ? Maybe that's right. BUT then, as it's not an exported 
function, you should test ALL usages of it and see if that's a correct 
behaviour. Well, It's used just in AppSearchDr, besides of 
AppSearchReg(). Maybe, knowing what is for AppSearchDr() I could know if 
it's correct to have its result backslashed too... But I don't. Again, 
my skills on MSI are *very* limited. I've chosen to fix just something I 
know must be fixed instead of try to fix the world without knowing 
anything about it. Wrong ?

BTW, thanx for the review.

Max