Closed Bug 186725 Opened 22 years ago Closed 21 years ago

nntp service NewURI doesn't check to see if SetSpec failed..

Categories

(MailNews Core :: Networking: NNTP, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: timeless, Assigned: hamfastgamgee)

References

()

Details

Attachments

(1 file, 1 obsolete file)

i'm not sure how serious this is, but most implementations check.
Attached patch This fixes the issue (obsolete) — Splinter Review
This checks for success of the SetSpec call.  It also de-tabs this particular
function (I figured I might as well do it while I was there).
Taking....
Assignee: sspitzer → andersma
Attachment #110160 - Flags: superreview?(sspitzer)
Attachment #110160 - Flags: review?(darin)
Comment on attachment 110160 [details] [diff] [review]
This fixes the issue

>Index: src/nsNntpService.cpp

>+    nsresult rv = NS_OK;
> 
>     nsCOMPtr <nsINntpUrl> nntpUrl = do_CreateInstance(NS_NNTPURL_CONTRACTID,&rv);

no reason to initialize rv since do_CreateInstance will always set rv.

>     NS_ENSURE_SUCCESS(rv,rv);
>     if (!nntpUrl) return NS_ERROR_FAILURE;

it should be sufficient to check rv.  no need to null check nntpUrl (i know
this is existing code, but it would be good to clean it up while you're at it).

> 
>-	nntpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) _retval);
>+    nntpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) _retval);

nit, prefer:

      rv = CallQueryInterface(nntpUrl, _retval);
      if (NS_FAILED(rv)) return rv;

>+    rv = (*_retval)->SetSpec(aSpec);
>+    NS_ENSURE_SUCCESS(rv,rv);
>+    return rv;

this code potentially introduces a leak.  the caller can rightfully do this:

  nsIURI *uri;
  rv = nntpService->NewURI(..., &uri);
  if (NS_FAILED(rv)) return rv;

this code would cause the uri object to leak if SetSpec failed.  so, you
should either use a COMPtr for the QueryInterface result or be sure to
call NS_RELEASE(*_retval) if SetSpec fails.
Attachment #110160 - Flags: review?(darin) → review-
Attached patch per darinSplinter Review
Attachment #110160 - Attachment is obsolete: true
*** Bug 186730 has been marked as a duplicate of this bug. ***
Attachment #110935 - Flags: superreview?(darin)
Comment on attachment 110935 [details] [diff] [review]
per darin

>Index: mozilla/mailnews/news/src/nsNntpService.cpp

>+    nsCOMPtr<nsINntpUrl> nntpUrl = do_CreateInstance(NS_NNTPURL_CONTRACTID, &rv);
>     NS_ENSURE_SUCCESS(rv,rv);
>     if (!nntpUrl) return NS_ERROR_FAILURE;

nit: eliminate redundant check.

with that sr=darin
Attachment #110935 - Flags: superreview?(darin) → superreview+
checked in
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Verified via code inspection using LXR.

1.235	timeless%mozdev.org	Jun 15 12:36	 	Bug 186725 nntp service NewURI doesn't
check to see if SetSpec failed..
r=caillon sr=darin
Status: RESOLVED → VERIFIED
Attachment #110160 - Flags: superreview?(sspitzer)
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: