On Thu, Jan 07, 2010 at 11:28:37AM -0800, Juan Lang wrote:
> Hi Marcus,
>
> Ideally this patch would be accompanied by a test. Also, this change:
>
> @@ -212,6 +212,10 @@ static BOOL CRYPT_DecodeEnsureSpace(DWORD dwFlags,
>
> if (dwFlags & CRYPT_DECODE_ALLOC_FLAG)
> {
> + if (!pvStructInfo) {
> + SetLastError(ERROR_INVALID_PARAMETER);
> + return FALSE;
> + }
>
> is a noop in all but one case: all the callers of
> CRYPT_DecodeEnsureSpace, save one, check pvStructInfo before calling
> it. It would be clearer, IMO, to change the single caller that
> doesn't check pvStructInfo (CryptDecodeObjectEx) rather than adding a
> check that is useless in most cases. A similar statement applies to
> the encode.c change: just change CryptEncodeObjectEx, not
> CRYPT_EncodeEnsureSpace.
>
> Finally, please indent consistently with the rest of the file.
>
> If you prefer, I can try to fix this. Triaging the Coverity bugs is
> probably enough work by itself, without being expected to fix them too
> ;-) Thanks,
In the meantime I had marked them as IGNORE already, as Windows also
shows inconsistent behaviour...
I would try this patch, although I am not sure I matched your indent style,
it is a bit strange:
>From d5d8f81fe7b0b7b9e91ae97611e4929821e9a564 Mon Sep 17 00:00:00 2001
From: Marcus Meissner <mar...@jet.franken.de>
Date: Tue, 22 Dec 2009 10:20:25 +0100
Subject: [PATCH] crypt32: check for NULL target pointers (Coverity)
Hi,
CID 596 and 595 ... we happily dereference pvEncoded
as NULL (if pcbEncoded is non-NULL), and pvStructInfo
(if pcbStructInfo is non-NULL).
Windows shows varying behaviour apparently, either
crash or error return... So we can chose error return too.
Adjusted after Juans comments.
Ciao, Marcus
---
dlls/crypt32/decode.c | 5 +++--
dlls/crypt32/encode.c | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/crypt32/decode.c b/dlls/crypt32/decode.c
index 1fd2383..1fda2a0 100644
--- a/dlls/crypt32/decode.c
+++ b/dlls/crypt32/decode.c
@@ -5885,8 +5885,9 @@ BOOL WINAPI CryptDecodeObjectEx(DWORD dwCertEncodingType,
LPCSTR lpszStructType,
{
ret = pCryptDecodeObject(dwCertEncodingType, lpszStructType,
pbEncoded, cbEncoded, dwFlags, NULL, pcbStructInfo);
- if (ret && (ret = CRYPT_DecodeEnsureSpace(dwFlags, pDecodePara,
- pvStructInfo, pcbStructInfo, *pcbStructInfo)))
+ if (ret && pvStructInfo &&
+ (ret = CRYPT_DecodeEnsureSpace(dwFlags, pDecodePara,
+ pvStructInfo, pcbStructInfo, *pcbStructInfo)))
ret = pCryptDecodeObject(dwCertEncodingType,
lpszStructType, pbEncoded, cbEncoded, dwFlags,
*(BYTE **)pvStructInfo, pcbStructInfo);
diff --git a/dlls/crypt32/encode.c b/dlls/crypt32/encode.c
index b7bbc83..585c696 100644
--- a/dlls/crypt32/encode.c
+++ b/dlls/crypt32/encode.c
@@ -4597,8 +4597,8 @@ BOOL WINAPI CryptEncodeObjectEx(DWORD dwCertEncodingType,
LPCSTR lpszStructType,
{
ret = pCryptEncodeObject(dwCertEncodingType, lpszStructType,
pvStructInfo, NULL, pcbEncoded);
- if (ret && (ret = CRYPT_EncodeEnsureSpace(dwFlags, pEncodePara,
- pvEncoded, pcbEncoded, *pcbEncoded)))
+ if (ret && pvStructInfo && (ret = CRYPT_EncodeEnsureSpace(
+ dwFlags, pEncodePara, pvEncoded, pcbEncoded, *pcbEncoded)))
ret = pCryptEncodeObject(dwCertEncodingType,
lpszStructType, pvStructInfo, *(BYTE **)pvEncoded,
pcbEncoded);
--
1.5.6
