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

Status: VERIFIED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[timeless]

i'm not sure how serious this is, but most implementations check.

Comment 1

[Mark Anderson]

Attachment: This fixes the issue

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

Comment 2

[Mark Anderson]

Taking....

Comment 3

[Darin Fisher]

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.

Comment 4

[timeless]

Attachment: per darin

Comment 5

[timeless]

*** Bug 186730 has been marked as a duplicate of this bug. ***

Comment 6

[Darin Fisher]

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

Comment 7

[timeless]

checked in

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

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