Re: msi(1/3): Don't crash if rec is NULL

[James Hawkins]

On 9/14/07, Juan Lang <juan.lang(a)gmail.com> wrote:
> This series of patches is in response to Jeremy's troubles with
> iTunes.  They may or may not fix the installer for him, but they
> prevent potential crashes.
> --Juan

MSI_FormatRecordW is an internal function and we should be crashing if
rec is NULL.

-- 
James Hawkins

msi(1/3): Don't crash if rec is NULL

[Juan Lang]

> MSI_FormatRecordW is an internal function and we should be crashing if
> rec is NULL.

Are you sure?  This is causing the iTunes installer to crash for a
couple people.  Besides, I don't think the code is written with this
assumption in mind.  In deformat_string_internal:

static DWORD deformat_string_internal(MSIPACKAGE *package, LPCWSTR ptr,
                                     WCHAR** data, DWORD len, MSIRECORD* record,
                                     INT* failcount)
...
    if (ptr==NULL)
    {
        TRACE("Deformatting NULL string\n");
        *data = NULL;
        return 0;
    }

So deformat_string_internal handles NULL correctly.  This patch
changes one of the call sites to avoid dereferencing a NULL pointer
before passing it to a function that deals with a NULL input.

--Juan

msi(1/3): Don't crash if rec is NULL

[James Hawkins]

On 9/14/07, Juan Lang <juan.lang(a)gmail.com> wrote:
> > MSI_FormatRecordW is an internal function and we should be crashing if
> > rec is NULL.
>
> Are you sure?  This is causing the iTunes installer to crash for a
> couple people.  Besides, I don't think the code is written with this
> assumption in mind.  In deformat_string_internal:
>
> static DWORD deformat_string_internal(MSIPACKAGE *package, LPCWSTR ptr,
>                                      WCHAR** data, DWORD len, MSIRECORD* record,
>                                      INT* failcount)
> ...
>     if (ptr==NULL)
>     {
>         TRACE("Deformatting NULL string\n");
>         *data = NULL;
>         return 0;
>     }
>
> So deformat_string_internal handles NULL correctly.  This patch
> changes one of the call sites to avoid dereferencing a NULL pointer
> before passing it to a function that deals with a NULL input.

The public APIs check for bad records and return ERROR_INVALID_HANDLE,
so they will never send in a NULL rec.  Something internally is
sending in a NULL rec, and that needs to be fixed.

-- 
James Hawkins

msi(1/3): Don't crash if rec is NULL

[Jeremy White]

Your patches also cure the symptom, Juan; thanks.

I agree with the concerns James expressed, though:  I didn't feel that I
understood the whole story.  (Heck, I really have *no* clue what I'm
talking about <grin>).

However, the call to MSI_FormatMessageW is not an internal call, I think;
it looks to me as though it's called as a direct result of an inbound
call to MsiFormatRecordA():
  0014:Call msi.MsiFormatRecordA(00000002,00000005,1000e768,7b7ea55c) ret=10003064
  (in http://bugs.winehq.org/attachment.cgi?id=8060   about 10-15 lines from bottom)

So the question in my mind is:
  Can a record still be a legitimate record if MSI_RecordGetFieldCount returns 0?

If the answer is yes, then I think the patches are correct.

Cheers,

Jeremy
> The public APIs check for bad records and return ERROR_INVALID_HANDLE,
> so they will never send in a NULL rec.  Something internally is
> sending in a NULL rec, and that needs to be fixed.

msi(1/3): Don't crash if rec is NULL

[James Hawkins]

On 9/14/07, Jeremy White <jwhite(a)winehq.org> wrote:
> Your patches also cure the symptom, Juan; thanks.
>
> I agree with the concerns James expressed, though:  I didn't feel that I
> understood the whole story.  (Heck, I really have *no* clue what I'm
> talking about <grin>).
>
> However, the call to MSI_FormatMessageW is not an internal call, I think;
> it looks to me as though it's called as a direct result of an inbound
> call to MsiFormatRecordA():
>   0014:Call msi.MsiFormatRecordA(00000002,00000005,1000e768,7b7ea55c) ret=10003064
>   (in http://bugs.winehq.org/attachment.cgi?id=8060   about 10-15 lines from bottom)
>
> So the question in my mind is:
>   Can a record still be a legitimate record if MSI_RecordGetFieldCount returns 0?
>
> If the answer is yes, then I think the patches are correct.

I don't know off the top of my head, but tests should definitely be
added before a fix goes in.

-- 
James Hawkins

msi(1/3): Don't crash if rec is NULL

[Juan Lang]

> The public APIs check for bad records and return ERROR_INVALID_HANDLE,
> so they will never send in a NULL rec.  Something internally is
> sending in a NULL rec, and that needs to be fixed.

On the contrary, the record is not NULL, it has 0 parameters.

I've resent in the fix with a test showing it's correct.
--Juan