1167410 - Value stored to 'eTLD' during its initialization is never read in nsEffectiveTLDService::GetBaseDomainInternal

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

+++ This bug was initially created as a clone of Bug #1144625 +++

Dead initialization. See fx-scan report at: https://people.mozilla.org/~sledru/reports/fx-scan-build/report-nsEffectiveTLDService.cpp-GetBaseDomainInternal-27-1.html#EndPath

Value stored to 'eTLD' during its initialization is never read in nsEffectiveTLDService::GetBaseDomainInternal. Line 254.

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Cloning the bug automatically added a depends on the original bug. Removing it as that's not needed.

Comment 2

[Marco]

If it's OK I'd like to take care of this.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Marco, sorry but Ritu has been working on it already.

Comment 4

[Marco]

No problem.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Attachment: patch.diff

Submitting a patch for review.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Ritu, you should now look for a reviewer for your patch :)
Usually, I look at the hg history to find the owner of the code.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8610715 [details] [diff] [review]
patch.diff

Requesting review on the patch I submitted. Thanks.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8610715 [details] [diff] [review]
patch.diff

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

::: netwerk/dns/nsEffectiveTLDService.cpp
@@ +250,5 @@
>    const char *prevDomain = nullptr;
>    const char *currDomain = aHostname.get();
>    const char *nextDot = strchr(currDomain, '.');
>    const char *end = currDomain + aHostname.Length();
> +  const char *eTLD; //Initialization was never used so commenting it out = currDomain;

This comment is a bit redundant. I'd just remove it entirely. (Just upload a new version of the patch when you have it and obsolete this version.)

It looks like I have done a number of reviews in this file, but they were actually just for wide-spread data structure changes. For this change, you should get a networking peer to review it ( https://wiki.mozilla.org/Modules/Core#Necko ). You could try Nick Hurley.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Andrew! I will do that.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8610715 [details] [diff] [review]
patch.diff

Will upload a new patch soon.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Attachment: patchv2.diff

patch v2

Comment 12

[Andrew McCreight [:mccr8]]

That patch looks like a diff from the old patch, but you should upload a patch that is both of these combined, so that it is the diff from trunk.

Comment 13

[u408661]

Comment on attachment 8614909 [details] [diff] [review]
patchv2.diff

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

Like :mccr8 said, please upload a new patch that has the full change, from current in-tree version to your latest version.

Comment 14

[u408661]

Also, looking at this, I'm not sure this is anything other than noise in the clang-analyzer output. The way the code is now, the default eTLD (currDomain) is relatively obvious, but changing the default to nullptr hides the default value way down in that not-quite-infinite loop. I'll have to think on it a bit more.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Attachment: patchv3.diff

Patch v3 submitted.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Here's a link to a Try build that is underway to test this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34b9afbf6e1a

Comment 17

[u408661]

Comment on attachment 8616838 [details] [diff] [review]
patchv3.diff

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

::: netwerk/dns/nsEffectiveTLDService.cpp
@@ +250,5 @@
>    const char *prevDomain = nullptr;
>    const char *currDomain = aHostname.get();
>    const char *nextDot = strchr(currDomain, '.');
>    const char *end = currDomain + aHostname.Length();
> +  const char *eTLD = nullptr;

Add a comment explaining that the default value is currDomain, as set in that while loop below, in order to make the code as readable as it is with this line that clang-analyzer complained about. Then we'll be good to go.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Attachment: patchv3.diff

With the additional comment suggested by Nicholas.

Comment 19

[u408661]

Comment on attachment 8626772 [details] [diff] [review]
patchv3.diff

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

Looks good to me, thanks!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0b2f8117cb

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff0b2f8117cb