Closed Bug 1278954 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Networking: NNTP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 50.0

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1137542)

Attachments

(1 file, 1 obsolete file)

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.
Proposed patch attached.
Assignee: nobody → ishikawa
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.
(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
Only initialization of port and warning. No error return.
Attachment #8761648 - Attachment is obsolete: true
Attachment #8762089 - Flags: review?(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
Attachment #8762089 - Flags: review?(rkent) → review+
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?
(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!
Keywords: checkin-needed
(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.
https://hg.mozilla.org/comm-central/rev/2895b88c37567c328f6a1136344cae468d38a24e
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
You need to log in before you can comment on or make changes to this bug.