Re: crypt32: check for NULL target pointers (Coverity)

[Juan Lang]

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,
--Juan

crypt32: check for NULL target pointers (Coverity)

[Marcus Meissner]

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 
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);