Closed Bug 1330845 Opened 4 years ago Closed 4 years ago

cmd/modutil/pk11.c:608:24: warning: comparison of integers of different signs: 'PK11DisableReasons' and 'int' [-Wsign-compare]

Categories

(NSS :: Tools, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

When building mozilla-central with clang 3.8, I'm noticing this build warning from NSS code:
===
security/nss/cmd/modutil/pk11.c:608:24: warning: comparison of integers of different signs: 'PK11DisableReasons' and 'int' [-Wsign-compare]
             if (reason < numDisableReasonStr) {
                 ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
===

It's pointing to this line of code:
  https://hg.mozilla.org/projects/nss/annotate/tip/cmd/modutil/pk11.c#l608

Indeed, "reason" is of type PK11DisableReasons (which is an enum whose values start at 0), whereas "numDisableReasonStr" is of type "int".

Simple fix: we could just give numDisableReasonStr type "size_t".  That makes more sense anyway, since it's a count.

(THOUGH: I think compilers don't make any guarantee about whether enums are signed vs. unsigned, so maybe there exist compilers where this enum is actually a signed value.  For those compilers, my above-suggested size_t fix would *introduce* a signed/unsigned comparison and build warning. So maybe really we should be casting "reason" to int or size_t or whatever matches numDisableReasonStr, for the purposes of comparison, anyway.)
For reference, the enum type (PK11DisableReasons) is defined here:
https://hg.mozilla.org/projects/nss/annotate/1db0933a06a3/lib/pk11wrap/secmodt.h/#l307
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Simple fix: we could just give numDisableReasonStr type "size_t".  That
> makes more sense anyway, since it's a count.

On further reflection, I've convinced myself that this makes sense -- and building on that, we should:
 (1) cast the enum to size_t before thinking about using it as an array index
 (2) compare that casted-value against the array-length, which we should also change to size_t (since it is a size, and it's the result of sizeof() invocations)
 (3) If the casted enum is smaller than the array length, then we can proceed and use *the size_t-casted version of the enum* as an array index.

(We technically shouldn't use the enum value *itself* as an array-index, because its representation might be signed and its raw representation might be negative -- then, we definitely wouldn't be safe using it as an index!  As it happens, all the valid values for this enum are small nonnegative numbers, BUT better to be on the safe side and be sure we're indexing sanely in case of bad input.)
Attached patch fix v1 (obsolete) — Splinter Review
Technically the first one-liner is the only change needed to fix the build warning (adjusting the type of numDisableReasonStr). But the rest is for belt-and-suspenders safety, as noted in my previous comment and in my code-comment in this patch.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8842309 - Flags: review?(kaie)
Daniel, thank for your working on this, and thinking about improvements.

The "reason" variable isn't used anywhere else. It seems overkill to have two separate variables for that number.

The long comment shouldn't be necessary, just to explain how we avoid a compiler warning. I'd prefer simple code that's obvious to understand, without logic that distracts from the actual implementation.

If changing numDisableReasonStr to size_t fixes it, then the compiler has apparently concluded that this particular enum is always positive. Which surprises, me, because I had assumed that enum is always a signed type.

If we want to fully protect against bad input, we can simply add a check that reason >= 0, too.
Attached patch 1330845-v2.patch (obsolete) — Splinter Review
could this be sufficient?
Attachment #8842421 - Flags: review?(dholbert)
Comment on attachment 8842421 [details] [diff] [review]
1330845-v2.patch

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

::: cmd/modutil/pk11.c
@@ +604,5 @@
>          PR_fprintf(PR_STDOUT, PAD "Firmware Version: %d.%d\n",
>                     slotinfo.firmwareVersion.major, slotinfo.firmwareVersion.minor);
>          if (PK11_IsDisabled(slot)) {
>              reason = PK11_GetDisabledReason(slot);
> +            if (reason >= 0 && reason < numDisableReasonStr) {

Nope, this isn't quite sufficient -- it still produces a build warning, regardless of whether "reason" happens to be signed or unsigned under the hood (which is a decision the compiler makes for us).

Specifically:
 (1) if it's unsigned, the "reason >= 0" comparison triggers this build warning (in clang 3.8.1 at least)
> pk11.c:608:24: warning: comparison of unsigned enum expression >= 0 is always true [-Wtautological-compare]

 (2) if it's signed, the "reason < numDisableReasonStr" will trigger a -Wsign-compare build warning (since numDisableReasonStr is now unsigned, as of this patch).


We can make this all better if we simply cast 'reason' to a known type before we compare it against anything (as in the patch I posted).  And since we happen to know that the values are all expected to be small and nonnegative, this cast should be totally legitimate.
Attachment #8842421 - Flags: review?(dholbert) → review-
For reference, here's the enum definition (with the possible values):
/* PKCS #11 disable reasons */
typedef enum {
    PK11_DIS_NONE = 0,
    PK11_DIS_USER_SELECTED = 1,
    PK11_DIS_COULD_NOT_INIT_TOKEN = 2,
    PK11_DIS_TOKEN_VERIFY_FAILED = 3,
    PK11_DIS_TOKEN_NOT_PRESENT = 4
} PK11DisableReasons;

https://hg.mozilla.org/projects/nss/annotate/146aaa22cf55/lib/pk11wrap/secmodt.h#l306
Oops, only just now saw comment 4. (I sometimes go back through my bugmail in reverse. :))

(In reply to Kai Engert (:kaie) from comment #4)
> The "reason" variable isn't used anywhere else. It seems overkill to have
> two separate variables for that number.

We could just store the result of the function-call directly into a size_t variable, then, if you prefer!  Then we'd just have one variable for the number (of type size_t).

> If changing numDisableReasonStr to size_t fixes it, then the compiler has
> apparently concluded that this particular enum is always positive. Which
> surprises, me, because I had assumed that enum is always a signed type.

The compiler has decided to represent it as an unsigned value, yeah.

After some brief Googling last night, it seems that compilers have leeway to make this choice, and don't guarantee any particular signed vs. unsigned representation (unless you include an explicitly-negative value in the enum definition, in which case it clearly has to be signed. But we don't want to do that here since we're using it as an array index).

> If we want to fully protect against bad input, we can simply add a check
> that reason >= 0, too.

That would work, except it triggers a different build warning. (And it still doesn't guarantee that reason's signedness matches numDisableReasonStr.  On my computer they happen to both end up unsigned, after your patch, but a compiler could just as easily decide to make 'reason' signed and spam a warning.)
Attached patch fix v3Splinter Review
Here's a 3rd option, just changing "reason" to size_t so we don't end up with two variables for this thing that's only used once.  This is just a simplification of fix v1.

I renamed it to "reasonIdx" to make it clearer that it's an index (of type size_t) rather than an enum value, too.  Thoughts?
Attachment #8842309 - Attachment is obsolete: true
Attachment #8842421 - Attachment is obsolete: true
Attachment #8842309 - Flags: review?(kaie)
Attachment #8842517 - Flags: review?(kaie)
Comment on attachment 8842517 [details] [diff] [review]
fix v3

I like this solution. r=kaie
Attachment #8842517 - Flags: review?(kaie) → review+
https://hg.mozilla.org/projects/nss/rev/7aa88a68f4ac

Not sure if this will be in 3.30 or 3.31
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1212199
You need to log in before you can comment on or make changes to this bug.