Closed Bug 825627 Opened 7 years ago Closed 7 years ago

Classifier.cpp:802:66: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Build warning:
{
toolkit/components/url-classifier/Classifier.cpp:802:66: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}

The code in question:

778 nsresult
779 Classifier::ReadNoiseEntries(const Prefix& aPrefix,
780                              const nsACString& aTableName,
781                              int32_t aCount,
782                              PrefixArray* aNoiseEntries)
783 {
784   LookupCache *cache = GetLookupCache(aTableName);
785   if (!cache) {
786     return NS_ERROR_FAILURE;
787   }
788 
789   nsTArray<uint32_t> prefixes;
790   nsresult rv = cache->GetPrefixes(&prefixes);
791   NS_ENSURE_SUCCESS(rv, rv);
792 
793   int32_t idx = prefixes.BinaryIndexOf(aPrefix.ToUint32());
794 
795   if (idx == nsTArray<uint32_t>::NoIndex) {
796     NS_WARNING("Could not find prefix in PrefixSet during noise lookup");
797     return NS_ERROR_FAILURE;
798   }
799 
800   idx -= idx % aCount;
801 
802   for (int32_t i = 0; (i < aCount) && ((idx+i) < prefixes.Length()); i++) {
803     Prefix newPref;
804     newPref.FromUint32(prefixes[idx+i]);
805     if (newPref != aPrefix) {
806       aNoiseEntries->AppendElement(newPref);
807     }
808   }
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/Classifier.cpp#778

Here, both |i| and |idx| are compared to unsigned values, and IIUC they also can only take non-negative values -- so we should declare them as unsigned.
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Here, both |i| and |idx| are compared to unsigned values, and IIUC they also
> can only take non-negative values -- so we should declare them as unsigned.

So, this is slightly-complicated by the fact that 'aCount' is signed, and it's compared to both 'i' (in the loop condition) and it's used in an operation with 'idx' (at line 800, for the modulus operation).

Luckily, 'aCount' traces back to something that can be known-to-be-nonnegative, so we can just change it (and the variable that feeds into it in the chain of callers) to be unsigned as well.

I'm posting a patch to do that, first, and then I'll post a second patch to convert 'i' and 'idx' to fix the warning.
The |aCount| variable traces back to |gethashNoise|, and we already bounds-check it in AddNoise() (we return early if it's less than 1), so it's already known-to-be-nonnegative where it's actually used.

This patch converts it to an unsigned value everywhere.  Also, at the point where we read it from a (user-controlled) pref, this patch makes us use the default value (4) if the pref has a (nonsensical) negative value.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #696761 - Flags: review?(gpascutto)
...and this fixes the actual variables 'i' and 'idx' mentioned in comment 0.  This patch fixes the build warning.
Attachment #696763 - Flags: review?(gpascutto)
Attachment #696761 - Flags: review?(gpascutto) → review+
Attachment #696763 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/58af88ca07be
https://hg.mozilla.org/mozilla-central/rev/6a17b673aeca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.