1278954 - (coverity) uninialized variable:/mailnews/news/src/nsNntpService.cpp: |port| is not always initialized.

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this: |port| is not initialized when
server is not found properly.


680nsresult
681nsNntpService::SetUpNntpUrlForPosting(const char *aAccountKey, char **newsUrlSpec)
682{
683  nsresult rv = NS_OK;
684
685  nsCString host;
   1. var_decl: Declaring variable port without initializer.
686  int32_t port;
687
688  nsCOMPtr<nsIMsgIncomingServer> nntpServer;
689  rv = GetNntpServerByAccount(aAccountKey, getter_AddRefs(nntpServer));
   2. Condition !!!NS_FAILED_impl(rv), taking false branch
   3. Condition !!!NS_FAILED_impl(rv), taking false branch
   4. Condition (bool)!!!NS_FAILED_impl(rv), taking false branch
690  if (NS_SUCCEEDED(rv) && nntpServer)
691  {
692    nntpServer->GetHostName(host);
693    nntpServer->GetPort(&port);
694  }
695
   5. Condition host.IsEmpty(), taking true branch
   CID 1137542 (#1 of 1): Uninitialized scalar variable (UNINIT)6. uninit_use_in_call: Using uninitialized value port when calling PR_smprintf.
696  *newsUrlSpec = PR_smprintf("%s/%s:%d",kNewsRootURI, host.IsEmpty() ? "news" : host.get(), port);
697  if (!*newsUrlSpec) return NS_ERROR_FAILURE;
698  return NS_OK;
699}


I think we can do two things:
initialize port to (-1) just in case.

Also, after the if on 690, put 
  else 
  { 
      NS_WARNING("failure to obtain host and port."); 
      return NS_ERROR_FAILURE; 
  }

since we are failing to notice possibly rare but valid(?)  error situation.

The return value NS_ERROR_FAILURE is stolen from line 697.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Bug 1278954 - set (-1) to |port| and print warning if it is not properly initialized.

Proposed patch attached.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8761648 [details] [diff] [review]
Bug 1278954 - set (-1) to |port| and print warning if it is not properly initialized.

Review of attachment 8761648 [details] [diff] [review]:
-----------------------------------------------------------------

I think it is good to set a default value for port. I am reluctant to return with an error though, which is a behavior change, when there is no obvious problem we are trying to solve.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #2)
> Comment on attachment 8761648 [details] [diff] [review]
> Bug 1278954 - set (-1) to |port| and print warning if it is not properly
> initialized.
> 
> Review of attachment 8761648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it is good to set a default value for port. I am reluctant to return
> with an error though, which is a behavior change, when there is no obvious
> problem we are trying to solve.

OK, I will simply leave NS_WARNING, but continue to run without returning.
Will create another patch.

TIA

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Attachments Bug 1278954 - set (-1) to |port| and print warning if it is not properly initialized.

Only initialization of port and warning. No error return.

Comment 5

[Kent James (:rkent)]

Comment on attachment 8762089 [details] [diff] [review]
Attachments Bug 1278954 - set (-1) to |port| and print warning if it is not properly initialized.

Review of attachment 8762089 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 6

[ISHIKAWA, Chiaki]

Thank you.
I put in the checkin-needed keyword.
Correct me if I am wrong. Whowever checks in the patch can add the reviewer's notation r+=rkent, etc?

Comment 7

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #6)
> Thank you.
> I put in the checkin-needed keyword.
> Correct me if I am wrong. Whowever checks in the patch can add the
> reviewer's notation r+=rkent, etc?

Yes, the person checking the patch in will add the r=reviewer. You do not need to upload a new patch just with that change. If you need to make other small changes (as requested by reviewer) then you can use that occasion to also add the r=reviewer tag.

Thanks for all the reports and fixes!

Comment 8

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #7)
> 
> Yes, the person checking the patch in will add the r=reviewer. You do not
> need to upload a new patch just with that change. If you need to make other
> small changes (as requested by reviewer) then you can use that occasion to
> also add the r=reviewer tag.
> 
> Thanks for all the reports and fixes!

Thank you for the clarification and you are welcome. So far, issues found by coverity
are relatively easy to handle.

Comment 9

[:aceman]

https://hg.mozilla.org/comm-central/rev/2895b88c37567c328f6a1136344cae468d38a24e