229197 - DRefTool analysis for psm files

Status: RESOLVED WORKSFORME

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

Bugs:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/pki/src/nsASN1Tree.cpp&rev=1.15&mark=121#116
Deref-error: "walk"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/pki/src/nsASN1Tree.cpp&rev=1.15&mark=126#121
Deref-error: "walk"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/pki/src/nsASN1Tree.cpp&rev=1.15&mark=139#134
Deref-error: "mTopNode"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsCMS.cpp&rev=1.18&mark=848#843
Deref-error: "obj"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsCRLManager.cpp&rev=1.7&mark=279#274
Deref-error: "crlData"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSASN1Object.cpp&rev=1.8&mark=193#188
Deref-error: "sequence"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSASN1Object.cpp&rev=1.8&mark=194#189
Deref-error: "sequence"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSASN1Object.cpp&rev=1.8&mark=205#200
Deref-error: "printableItem"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSASN1Object.cpp&rev=1.8&mark=229#224
Deref-error: "sequence"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp&rev=1.35&mark=263#258
Deref-error: "status"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp&rev=1.35&mark=267#262
Deref-error: "status"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp&rev=1.35&mark=268#263
Deref-error: "status"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp&rev=1.35&mark=269#264
Deref-error: "status"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=680#675
Deref-error: "cert"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=691#686
Deref-error: "cert"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=695#690
Deref-error: "cert"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=1080#1075
Deref-error: "printableItem"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=1086#1081
Deref-error: "printableItem"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=1091#1086
Deref-error: "printableItem"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=1093#1088
Deref-error: "printableItem"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.102&mark=1373#1368
Deref-error: "validitySequence"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=650#645
Deref-error: "event"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=651#646
Deref-error: "event"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=678#673
Deref-error: "urlString"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=691#686
Deref-error: "psmDownloader"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=692#687
Deref-error: "psmDownloader"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=696#691
Deref-error: "urlString"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=1458#1453
Deref-error: "pCert"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.112&mark=1467#1462
Deref-error: "pCert"

Not a bug (but it's not good code):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsCipherInfo.cpp&rev=1.4&mark=67#62
Deref-error: "aCipherInfo"

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 2

[Frank Wein [:mcsmurf]]

Comment on attachment 177227 [details] [diff] [review]
Patch

>Index: nsNSSCallbacks.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp,v
>retrieving revision 1.37
>diff -u -p -1 -8 -r1.37 nsNSSCallbacks.cpp
>--- nsNSSCallbacks.cpp	25 Apr 2004 15:02:36 -0000	1.37
>+++ nsNSSCallbacks.cpp	12 Mar 2005 15:30:00 -0000
>@@ -258,36 +258,38 @@ void PR_CALLBACK HandshakeCallback(PRFil
>   }
[...]
>     /* Set the SSL Status information */
>     nsCOMPtr<nsSSLStatus> status = new nsSSLStatus();
>+    if (!status)
>+      return NS_ERROR_OUT_OF_MEMORY;

This would be changed to "return;" since the function decl is void (and this
can't be changed here AFAIK).

Comment 3

[timeless]

Comment on attachment 177227 [details] [diff] [review]
Patch

>   nsAutoString shortDesc;
>   const PRUnichar* formatStrings[1] = { ToNewUnicode(NS_ConvertUTF8toUCS2(caName)) };
please check this for failure, or better, use:
NS_ConvertUTF8toUCS2 CAName(caName);
const PRUnichar* formatStrings[1] = { CAName.get() };
and save the evil stuff.

>   nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
>   if (NS_SUCCEEDED(rv)) {
>     nsMemory::Free(NS_CONST_CAST(PRUnichar*, formatStrings[0]));
currently this is leaked if getservicefails
>   }

>Index: nsNSSComponent.cpp
>@@ -635,36 +635,39 @@ static void setOCSPOptions(nsIPrefBranch
> nsresult
> nsNSSComponent::PostCRLImportEvent(nsCAutoString *urlString, PSMContentDownloader *psmDownloader)
> {
>   nsresult rv;
>   
>   //Create the event
>   CRLDownloadEvent *event = new CRLDownloadEvent;
>+  if (!event)
>+    return NS_ERROR_OUT_OF_MEMORY;
i believe event will be leaked if it isn't posted:

>   nsCOMPtr<nsIEventQueueService> service = 
>                         do_GetService(NS_EVENTQUEUESERVICE_CONTRACTID, &rv);
>   if (NS_FAILED(rv)) 
here:
>     return rv;

>   rv = service->GetThreadEventQueue(NS_UI_THREAD, &result);
>   if (NS_FAILED(rv)) 
here:
>     return rv;

hopefully not past here:
>   //Post the event
>@@ -677,36 +680,39 @@ nsNSSComponent::DownloadCRLDirectly(nsAu
>   //This api is meant to support direct interactive update of crl from the crl manager
>   //or other such ui.
null check this:
>   PSMContentDownloader *psmDownloader = new PSMContentDownloader(PSMContentDownloader::PKCS7_CRL);
>   
null check this:
>   nsCAutoString *urlString = new nsCAutoString();
but don't leak psmDownloader!
>   urlString->AssignWithConversion(url.get());
>     
>   return PostCRLImportEvent(urlString, psmDownloader);
> }

> nsresult nsNSSComponent::DownloadCrlSilently()
> {
>   //Add this attempt to the hashtable
null check this:
>   nsStringKey *hashKey = new nsStringKey(mCrlUpdateKey.get());
>   crlsScheduledForDownload->Put(hashKey,(void *)nsnull);
>     
>   //Set up the download handler
>   PSMContentDownloader *psmDownloader = new PSMContentDownloader(PSMContentDownloader::PKCS7_CRL);
>+  if (!psmDownloader)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>   psmDownloader->setSilentDownload(PR_TRUE);
>   psmDownloader->setCrlAutodownloadKey(mCrlUpdateKey);
>   
>   //Now get the url string
null check this:
>   nsCAutoString *urlString = new nsCAutoString();
>   urlString->AssignWithConversion(mDownloadURL);
but don't leak psmDownloader!
> 
>   return PostCRLImportEvent(urlString, psmDownloader);
> }

thanks

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch 2

Issues should be fixed now, i excluded whitespace changes from this diff

Comment 5

[timeless]

Comment on attachment 177406 [details] [diff] [review]
Patch 2

>Index: nsNSSCallbacks.cpp
>@@ -245,60 +245,70 @@ void PR_CALLBACK HandshakeCallback(PRFil
>+  NS_ConvertUTF8toUCS2 CAName(caName);
>+  const PRUnichar* formatStrings[1] = { CAName.get() };

you don't want to do this anymore:
>     nsMemory::Free(NS_CONST_CAST(PRUnichar*, formatStrings[0]));

please ask kaie for the next review

Comment 6

[timeless]

mcsmurf: could you please finish this off? i just hit this mess crawling through nss.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

"new" failures are fatal now, and other code changed in this patch appears to have been removed.