Office 2007 MSI Crash - Null dereference @ MsiViewExecute

[Mike Kaplinskiy]

On Sun, May 3, 2009 at 10:36 PM, James Hawkins <truiken at gmail.com> wrote:
> On Sun, May 3, 2009 at 1:52 PM, Mike Kaplinskiy
> <mike.kaplinskiy at gmail.com> wrote:
>> On Thu, Apr 30, 2009 at 1:08 PM, James Hawkins <truiken at gmail.com> wrote:
>>> On Thu, Apr 30, 2009 at 4:03 AM, Austin English <austinenglish at gmail.com> wrote:
>>>> On Tue, Apr 28, 2009 at 9:27 PM, Mike Kaplinskiy
>>>> <mike.kaplinskiy at gmail.com> wrote:
>>>>> I was looking at the trace of the crash from bug 17600, and it looks like
>>>>> a custom action is calling MsiViewExecute with a null hRec.
>>>>>
>>>>> I (sadly) don't know much about the wine MSI architecture, but the
>>>>> msiobj_lock on line 484 should fail since rec will never be fetched
>>>>> (null). I think the intention was to make it query->hdr (as it is
>>>>> released later).
>>>>
>>>> A testcase for it would show if you're right or wrong ;-).
>>>>
>>>
>>> Not really.  If you grep through the msi tests, you'll see that we
>>> call MsiViewExecute with NULL hRec all over the place.  That doesn't
>>> mean there isn't a bug, just saying.
>>>
>>> --
>>> James Hawkins
>>>
>>
>> So I got a little time to look into this, and was completely blown
>> away. Apparently doing
>>
>> TRACE("dereferencing 0?=%p\n", (void *) &(((MSIRECORD *) NULL)->hdr));
>>
>> works, since nothing ever gets dereferenced. I will post a patch for
>> this small nit.
>>
>> On a slightly different note, I have traced the problem in office 2007
>> to the patch by James, and in particular to INSERT_execute. The row
>> index gets set to 0 if the primary key is null (which is what office
>> install is doing), but if the insert is temporary TABLE_insert assumes
>> that row >= tv->table->row_count, and subtracts from the row index,
>> putting us in the negatives (or high positives due to uint).
>>
>> I have a patch for this, but I don't know how to make a testcase (all
>> I know is office installs), so would anyone mind pointing me to
>> somewhere that explains msi temporary inserts?
>>
>> And should I post the patch up on wine-patches to get it critiqued?
>> (sorry, first time)
>>
>
> I'll take a look at your patch.  No offense intended, but it's
> probably not correct.  The problem is a beast and deeply rooted in the
> way we store temporary rows in the DB.  I have a huge test patch for
> it, and I had a fix, but my linux HD got fragged (bummer).
>
> --
> James Hawkins
>

(Sorry, forgot to forward to the list)

Thanks James. I'm not gonna post it on the patches list quiet yet
then. I checked that the msi tests pass with the patch as well.

Mike.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-msi-fix-temporary-row-handling.patch
Type: text/x-diff
Size: 12682 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20090503/a38c71ee/attachment-0001.patch>