Re: crypt32: Avoid reading unitialized variables (Coverity)
2011/12/13 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
CID 4647-4648 --- dlls/crypt32/cert.c | 2 +- dlls/crypt32/encode.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/crypt32/cert.c b/dlls/crypt32/cert.c index 63107e1..ee96abd 100644 --- a/dlls/crypt32/cert.c +++ b/dlls/crypt32/cert.c @@ -2554,7 +2554,7 @@ BOOL WINAPI CertAddEnhancedKeyUsageIdentifier(PCCERT_CONTEXT pCertContext, LPCSTR pszUsageIdentifier) { BOOL ret; - DWORD size; + DWORD size = 0;
TRACE("(%p, %s)\n", pCertContext, debugstr_a(pszUsageIdentifier));
diff --git a/dlls/crypt32/encode.c b/dlls/crypt32/encode.c index 05a3558..143ef0a 100644 --- a/dlls/crypt32/encode.c +++ b/dlls/crypt32/encode.c @@ -768,7 +768,7 @@ static BOOL WINAPI CRYPT_AsnEncodeExtensions(DWORD dwCertEncodingType, ret = TRUE; for (i = 0, dataLen = 0; ret && i < exts->cExtension; i++) { - DWORD size; + DWORD size = 0;
ret = CRYPT_AsnEncodeExtension(&exts->rgExtension[i], NULL, &size); if (ret) -- 1.7.8
Just a quick check, since Debian had a big kerfuffle when someone went and initialized variables in a crypto module, but is there any chance that those variables were left uninitialized on purpose?
On 14 December 2011 14:44, Michael Curran <curran.michaelp(a)gmail.com> wrote:
Just a quick check, since Debian had a big kerfuffle when someone went and initialized variables in a crypto module, but is there any chance that those variables were left uninitialized on purpose?
Except for some TRACEs, I don't think these are actually read.
On 14 December 2011 14:54, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 14 December 2011 14:44, Michael Curran <curran.michaelp(a)gmail.com> wrote:
Just a quick check, since Debian had a big kerfuffle when someone went and initialized variables in a crypto module, but is there any chance that those variables were left uninitialized on purpose?
Except for some TRACEs, I don't think these are actually read. And actually, that probably makes the patch wrong too. If those functions end up being called by an application you'd still be tracing uninitialized data, but Coverity just wouldn't notice.
On Wed, Dec 14, 2011 at 14:58, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 14 December 2011 14:54, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 14 December 2011 14:44, Michael Curran <curran.michaelp(a)gmail.com> wrote:
Just a quick check, since Debian had a big kerfuffle when someone went and initialized variables in a crypto module, but is there any chance that those variables were left uninitialized on purpose?
Except for some TRACEs, I don't think these are actually read. And actually, that probably makes the patch wrong too. If those functions end up being called by an application you'd still be tracing uninitialized data, but Coverity just wouldn't notice.
They're only used in TRACEs since the passed pUsage (previous param) is NULL indeed. Coverity is actually OK reporting the "defect" since at least one path (when tracing is enabled) prints the uninitialized garbage. Should the TRACE (e.g. on crypt32:2435) be adapted so it conditionally prints the value instead? Or be removed altogether? Frédéric
Should the TRACE (e.g. on crypt32:2435) be adapted so it conditionally prints the value instead? Or be removed altogether?
I'd prefer a conditional print if it's not too ugly. There haven't been too many decoding bugs in a while, but the trace can sometimes help identify them. Thanks, --Juan
2011/12/14 Juan Lang <juan.lang(a)gmail.com>:
Should the TRACE (e.g. on crypt32:2435) be adapted so it conditionally prints the value instead? Or be removed altogether?
I'd prefer a conditional print if it's not too ugly. There haven't been too many decoding bugs in a while, but the trace can sometimes help identify them.
Thanks, --Juan
On second thought, coverity would still complain even with the TRACEs adapted, so I'll mark these defects as Ignored in Coverity Frédéric
On second thought, coverity would still complain even with the TRACEs adapted, so I'll mark these defects as Ignored in Coverity
Does the attached patch address one of the Coverity complaints? --Juan
2011/12/16 Juan Lang <juan.lang(a)gmail.com>:
On second thought, coverity would still complain even with the TRACEs adapted, so I'll mark these defects as Ignored in Coverity
Does the attached patch address one of the Coverity complaints? --Juan
I can't launch a new Coverity run by myself (only Amine Khaldi has the required privileges IIRC), so am not sure whether Coverity would still complain about reading uninitialized memory. Amine, can you/is it possible to launch a limited Coverity run with Juan's patch applied? Frédéric
2011/12/16 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
2011/12/16 Juan Lang <juan.lang(a)gmail.com>:
On second thought, coverity would still complain even with the TRACEs adapted, so I'll mark these defects as Ignored in Coverity
Does the attached patch address one of the Coverity complaints? --Juan
I can't launch a new Coverity run by myself (only Amine Khaldi has the required privileges IIRC), so am not sure whether Coverity would still complain about reading uninitialized memory.
Amine, can you/is it possible to launch a limited Coverity run with Juan's patch applied?
After talking with Amine on IRC, it seems your patch should be OK. I'll update and resend the patch
participants (4)
-
Frédéric Delanoy -
Henri Verbeet -
Juan Lang -
Michael Curran