Office 2007 MSI Crash - Null dereference @ MsiViewExecute
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). Sorry if I'm dead wrong - just my 2c
On Tue, Apr 28, 2009 at 9:27 PM, Mike Kaplinskiy <mike.kaplinskiy(a)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 ;-). -- -Austin
On Thu, Apr 30, 2009 at 4:03 AM, Austin English <austinenglish(a)gmail.com> wrote:
On Tue, Apr 28, 2009 at 9:27 PM, Mike Kaplinskiy <mike.kaplinskiy(a)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
James Hawkins wrote:
On Thu, Apr 30, 2009 at 4:03 AM, Austin English <austinenglish(a)gmail.com> wrote:
On Tue, Apr 28, 2009 at 9:27 PM, Mike Kaplinskiy <mike.kaplinskiy(a)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.
If there are tests that check this, I don't know how they could be passing (unless hView is invalid). The local variable rec isn't set if hRec is null, and it is dereferenced on line 484 & 486 regardless. I would write a test for this if I had time, but I don't know how the test harness works, and don't have time right now to learn. (nor do I know how the MSI framework works). It seems like it would be a big win to have this work, since it would return the Office 07 installer to platinum status.
On Thu, Apr 30, 2009 at 1:08 PM, James Hawkins <truiken(a)gmail.com> wrote:
On Thu, Apr 30, 2009 at 4:03 AM, Austin English <austinenglish(a)gmail.com> wrote:
On Tue, Apr 28, 2009 at 9:27 PM, Mike Kaplinskiy <mike.kaplinskiy(a)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) Mike.
On Sun, May 3, 2009 at 1:52 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
On Thu, Apr 30, 2009 at 1:08 PM, James Hawkins <truiken(a)gmail.com> wrote:
On Thu, Apr 30, 2009 at 4:03 AM, Austin English <austinenglish(a)gmail.com> wrote:
On Tue, Apr 28, 2009 at 9:27 PM, Mike Kaplinskiy <mike.kaplinskiy(a)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
participants (3)
-
Austin English -
James Hawkins -
Mike Kaplinskiy