Re: msi: Avoid a potential NULL pointer dereference (coccicheck).
On 05/20/2010 01:15 AM, Michael Stefaniuc wrote:
The last "goto done" is for si == NULL. When MSI_GetSummaryInformationW returns NULL there is a crash. --- dlls/msi/msi.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 3170e6d..9c08d1b 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -551,7 +551,8 @@ static UINT MSI_ApplicablePatchW( MSIPACKAGE *package, LPCWSTR patch )
done: msiobj_release(&patch_db->hdr ); - msiobj_release(&si->hdr ); + if (si) + msiobj_release(&si->hdr ); return r; }
Hi Michael, This one is mentioned by Coverity (#970). Marcus marked this one as 'FALSE' with the remark: "hdr is at position 0, so this will be NULL and msiobj_release handles it." Thoughts? -- Cheers, Paul.
On 05/20/2010 07:55 AM, Paul Vriens wrote:
On 05/20/2010 01:15 AM, Michael Stefaniuc wrote:
The last "goto done" is for si == NULL. When MSI_GetSummaryInformationW returns NULL there is a crash. --- dlls/msi/msi.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 3170e6d..9c08d1b 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -551,7 +551,8 @@ static UINT MSI_ApplicablePatchW( MSIPACKAGE *package, LPCWSTR patch )
done: msiobj_release(&patch_db->hdr ); - msiobj_release(&si->hdr ); + if (si) + msiobj_release(&si->hdr ); return r; }
Hi Michael,
This one is mentioned by Coverity (#970). Marcus marked this one as 'FALSE' with the remark:
"hdr is at position 0, so this will be NULL and msiobj_release handles it."
Thoughts?
The main reason I'm asking is because we have several of these lingering around (and marked as 'FALSE' in Coverity). -- Cheers, Paul.
On Thu, 2010-05-20 at 08:04 +0200, Paul Vriens wrote:
The main reason I'm asking is because we have several of these lingering around (and marked as 'FALSE' in Coverity).
In this particular case we should just get rid of the goto.
On 05/20/2010 10:00 AM, Hans Leidekker wrote:
On Thu, 2010-05-20 at 08:04 +0200, Paul Vriens wrote:
The main reason I'm asking is because we have several of these lingering around (and marked as 'FALSE' in Coverity).
In this particular case we should just get rid of the goto.
But is it a (potential) issue? I guess not. If it's not an issue wee can leave the code as is, not? -- Cheers, Paul.
On Thu, 2010-05-20 at 10:03 +0200, Paul Vriens wrote:
The main reason I'm asking is because we have several of these lingering around (and marked as 'FALSE' in Coverity).
In this particular case we should just get rid of the goto.
But is it a (potential) issue? I guess not. If it's not an issue wee can leave the code as is, not?
It's a header construction where you typically exploit the fact that the header and extended structure start at the same address. So yes, we could leave the code as is.
Hans Leidekker wrote:
On Thu, 2010-05-20 at 10:03 +0200, Paul Vriens wrote:
The main reason I'm asking is because we have several of these lingering around (and marked as 'FALSE' in Coverity). In this particular case we should just get rid of the goto.
But is it a (potential) issue? I guess not. If it's not an issue wee can leave the code as is, not?
It's a header construction where you typically exploit the fact that the header and extended structure start at the same address. So yes, we could leave the code as is. "exploit" doesn't sound like a clean approach to me. The compiler does the right thing here but there is quite some effort involved for a human reader to validate that. The reports of good static analyze tools should more like a hint "there is a problem around that code" and the problem doesn't have to be the exact same thing the tool actually reported.
bye michael
On Thu, 2010-05-20 at 12:49 +0200, Michael Stefaniuc wrote:
It's a header construction where you typically exploit the fact that the header and extended structure start at the same address. So yes, we could leave the code as is. "exploit" doesn't sound like a clean approach to me. The compiler does the right thing here but there is quite some effort involved for a human reader to validate that. The reports of good static analyze tools should more like a hint "there is a problem around that code" and the problem doesn't have to be the exact same thing the tool actually reported.
Yes, that's why removing the goto is the better solution here, because it simply avoids that case.
Paul Vriens wrote:
On 05/20/2010 01:15 AM, Michael Stefaniuc wrote:
The last "goto done" is for si == NULL. When MSI_GetSummaryInformationW returns NULL there is a crash. --- dlls/msi/msi.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 3170e6d..9c08d1b 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -551,7 +551,8 @@ static UINT MSI_ApplicablePatchW( MSIPACKAGE *package, LPCWSTR patch )
done: msiobj_release(&patch_db->hdr ); - msiobj_release(&si->hdr ); + if (si) + msiobj_release(&si->hdr ); return r; }
Hi Michael,
This one is mentioned by Coverity (#970). Marcus marked this one as 'FALSE' with the remark:
"hdr is at position 0, so this will be NULL and msiobj_release handles it."
Thoughts?
Do the compilers treat the addressof operator on the struct member as si + FIELD_OFFSET(MSISUMMARYINFO, hdr) ? Then yes, no dereference happens and the result of the above calculation is NULL which is fine as input for msiobj_release(). bye michael
On Thu, May 20, 2010 at 12:36:21PM +0200, Michael Stefaniuc wrote:
Paul Vriens wrote:
On 05/20/2010 01:15 AM, Michael Stefaniuc wrote:
The last "goto done" is for si == NULL. When MSI_GetSummaryInformationW returns NULL there is a crash. --- dlls/msi/msi.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 3170e6d..9c08d1b 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -551,7 +551,8 @@ static UINT MSI_ApplicablePatchW( MSIPACKAGE *package, LPCWSTR patch )
done: msiobj_release(&patch_db->hdr ); - msiobj_release(&si->hdr ); + if (si) + msiobj_release(&si->hdr ); return r; }
Hi Michael,
This one is mentioned by Coverity (#970). Marcus marked this one as 'FALSE' with the remark:
"hdr is at position 0, so this will be NULL and msiobj_release handles it."
Thoughts?
Do the compilers treat the addressof operator on the struct member as si + FIELD_OFFSET(MSISUMMARYINFO, hdr) ? Then yes, no dereference happens and the result of the above calculation is NULL which is fine as input for msiobj_release().
Yes, they do. Ciao, Marcus
participants (4)
-
Hans Leidekker -
Marcus Meissner -
Michael Stefaniuc -
Paul Vriens