DRefTool analysis for psm files

RESOLVED WORKSFORME

Status

Core Graveyard
Security: UI
RESOLVED WORKSFORME
15 years ago
2 years ago

People

(Reporter: timeless, Assigned: mcsmurf)

Tracking

Other Branch

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-cuz])

Attachments

(1 attachment, 1 obsolete attachment)

12.41 KB, patch
timeless
: review-
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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"
(Reporter)

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core
(Assignee)

Comment 1

13 years ago
Created attachment 177227 [details] [diff] [review]
Patch
(Assignee)

Updated

13 years ago
Attachment #177227 - Flags: review?(timeless)
(Assignee)

Comment 2

13 years ago
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).
(Reporter)

Comment 3

13 years ago
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
Attachment #177227 - Flags: review?(timeless) → review-
(Assignee)

Comment 4

13 years ago
Created attachment 177406 [details] [diff] [review]
Patch 2

Issues should be fixed now, i excluded whitespace changes from this diff
Attachment #177227 - Attachment is obsolete: true
Attachment #177406 - Flags: review?(timeless)

Updated

13 years ago
Whiteboard: [kerh-cuz]
(Reporter)

Comment 5

13 years ago
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
Attachment #177406 - Flags: review?(timeless) → review-
(Reporter)

Comment 6

13 years ago
mcsmurf: could you please finish this off? i just hit this mess crawling through nss.
Assignee: timeless → bugzilla
QA Contact: bmartin → ui
"new" failures are fatal now, and other code changed in this patch appears to have been removed.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.