Re: msi(1/3): Don't crash if rec is NULL
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_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
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
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.
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
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
participants (3)
-
James Hawkins -
Jeremy White -
Juan Lang