signed/unsigned comparisons in pk11akey.c

NEW
Unassigned

Status

NSS
Libraries
P3
normal
2 years ago
2 months ago

People

(Reporter: mt, Unassigned)

Tracking

(Blocks: 1 bug)

trunk

Firefox Tracking Flags

(firefox44 affected)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
22:53:01     INFO -  pk11akey.c: In function 'PK11_ImportPublicKey':
 22:53:01    ERROR -  pk11akey.c:200:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -    PORT_Assert(templateCount <= (sizeof(theTemplate)/sizeof(CK_ATTRIBUTE)));
 22:53:01     INFO -                    ^
 22:53:01     INFO -  pk11akey.c: In function 'pk11_get_Decoded_ECPoint':
 22:53:01    ERROR -  pk11akey.c:406:26: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -    && (ecPoint->ulValueLen == keyLen)) {
 22:53:01     INFO -                            ^
 22:53:01    ERROR -  pk11akey.c:420:63: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -           if (keyLen && rv == SECSuccess && publicKeyValue->len == keyLen) {
 22:53:01     INFO -                                                                 ^
 22:53:01     INFO -  pk11akey.c: In function 'PK11_ExtractPublicKey':
 22:53:01    ERROR -  pk11akey.c:621:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -    PR_ASSERT(templateCount <= sizeof(template)/sizeof(CK_ATTRIBUTE));
 22:53:01     INFO -                    ^
 22:53:01    ERROR -  pk11akey.c:644:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -    PR_ASSERT(templateCount <= sizeof(template)/sizeof(CK_ATTRIBUTE));
 22:53:01     INFO -                    ^
 22:53:01    ERROR -  pk11akey.c:669:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -    PR_ASSERT(templateCount <= sizeof(template)/sizeof(CK_ATTRIBUTE));
 22:53:01     INFO -                    ^
 22:53:01    ERROR -  pk11akey.c:691:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -    PR_ASSERT(templateCount <= sizeof(template)/sizeof(CK_ATTRIBUTE));
 22:53:01     INFO -                    ^
 22:53:01     INFO -  pk11akey.c: In function 'PK11_ListPublicKeysInSlot':
 22:53:01    ERROR -  pk11akey.c:2326:13: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -       PORT_Assert(tsize <= sizeof(findTemp)/sizeof(CK_ATTRIBUTE));
 22:53:01     INFO -               ^
 22:53:01     INFO -  pk11akey.c: In function 'PK11_ListPrivKeysInSlot':
 22:53:01    ERROR -  pk11akey.c:2372:13: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
 22:53:01     INFO -       PORT_Assert(tsize <= sizeof(findTemp)/sizeof(CK_ATTRIBUTE));
 22:53:01     INFO -               ^
(Reporter)

Comment 1

2 years ago
This one worries me more than the one in bug 1211722, because this looks like it is the first file in the directory and there are lots of errors.  I think that we should consider a try run for this before re-landing.
(Reporter)

Comment 2

2 years ago
Created attachment 8670134 [details] [diff] [review]
bug1211725.patch

My biggest concern with this one is that it might not be enough.  I'll see if I can take this and the other patch and run it on try first.
Attachment #8670134 - Flags: review?(kaie)

Comment 3

2 years ago
I've been trying to help with the -Werror effort today, by trying to revive the Android building on the NSS buildbot.

I had to fix a cross compilation issue, and had to upgrade to a newer Android NDK, because the old one was limited to gcc-4.4.3

With the new NDK and Android gcc 4.9, I see additional failures. For example this one:

sha512.c: In function 'SHA256_Compress':
sha512.c:132:24: error: assignment makes integer from pointer without a cast [-Werror]
#define BYTESWAP4(x) x = SHA_HTONL(x)


I wonder why we haven't seen those on other builds yet?

I don't see that warning in 32-bit builds that use older compilers.

Comment 4

2 years ago
Suggestion:

- let's change the hardcoded NSS Android build settings, to make it build with the
  upgraded NDK

- on Android, instead of using -Werror, let's use the warnings configuration that
  I found on a firefox build logfile

This way, you can see all the relevant errors on the Android builder of the NSS buildbot.

Comment 5

2 years ago
Created attachment 8670400 [details] [diff] [review]
fix-android-ndk10

fix our android build settings to be compatible with our upgraded android builder (was hardcoded, still hardcoded)

With this, I was able to complete a local cross compilation of NSS, and I saw the warning that failed your firefox build as errors.
Attachment #8670400 - Flags: review?(martin.thomson)

Updated

2 years ago
Attachment #8670134 - Flags: review?(kaie) → review+
(Reporter)

Comment 6

2 years ago
Comment on attachment 8670400 [details] [diff] [review]
fix-android-ndk10

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

Does this have the effect of increasing our minimum supported NDK version?  I don't want to do that without discussion.

::: Makefile
@@ +55,5 @@
>  # Translate coreconf build options to NSPR configure options.
>  #
>  
>  ifeq ($(OS_TARGET),Android)
> +NSPR_CONFIGURE_OPTS += --with-android-ndk=$(ANDROID_NDK) --target=arm-linux-androideabi --with-android-version=$(OS_TARGET_RELEASE) --with-android-toolchain=$(ANDROID_NDK)/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86 --with-android-platform=$(ANDROID_NDK)/platforms/android-8/arch-arm

I think that you should be careful to use the variables that are set in Linux.mk:  ANDROID_TARGET, ANDROID_TOOLCHAIN and ANDROID_SYSROOT in particular.

::: coreconf/Linux.mk
@@ +162,5 @@
>  endif
>  ifeq (true,$(NSS_HAS_GCC48))
> +
> +ifeq ($(OS_TARGET),Android)
> +OS_CFLAGS += -Wall -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits -Werror=char-subscripts -Werror=comment -Werror=endif-labels -Werror=enum-compare -Werror=ignored-qualifiers -Werror=int-to-pointer-cast -Werror=multichar -Werror=nonnull -Werror=pointer-arith -Werror=pointer-sign -Werror=return-type -Werror=sequence-point -Werror=trigraphs -Werror=uninitialized -Werror=unknown-pragmas -Wno-unused -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds

I'd like to see some of these extra warnings enabled unconditionally, not just for android.  I reviewed https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options  and found that -Wall doesn't enable -Wsign-compare for instance.  Since people are developing on Linux or Mac for the most part, I think that it would be better if those options were set so that we make failures visible on those platforms.

This newly enables:
  -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits

All of the -Werror=XXX options are subsumed by a general -Werror, which I think we could enable if it weren't for the -Wsign-compare issues we are seeing.

We have -Wno-unused here, which I think can be removed.

And the following are set to disable errors, which I think can be safely removed from this list:
  -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bound

In conclusion, I think that this could be changed to:

OS_CFLAGS += -Wall -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits -Wno-error=sign-compare

I tested the combination of -Wsign-compare and -Wno-error=sign-compare and it produces the results we want (a warning, but no error).
Attachment #8670400 - Flags: review?(martin.thomson)
(Reporter)

Comment 7

2 years ago
I added -Wsign-compare to coreconf/Darwin.mk on my machine and observed that quite a few errors are generated.  I think that the right thing to do here is add -Wno-error=sign-compare (as I suggested), then open a bug to remove those warnings.  The number of warnings is manageable, maybe 50 all told.
(Reporter)

Updated

2 years ago
See Also: → bug 1212199
(Reporter)

Comment 8

2 years ago
Created attachment 8670616 [details] [diff] [review]
bug1211725-2.patch

Kai, I think that this patch is a better fit for the -Wxxx options.  I didn't include the changes to update the NDK here though.
Attachment #8670616 - Flags: review?(kaie)

Comment 9

2 years ago
(In reply to Martin Thomson [:mt:] from comment #6)
> Does this have the effect of increasing our minimum supported NDK version? 
> I don't want to do that without discussion.

I looked at the log of the failed Firefox build in more detail (listed in bug 1211568).

The Android 2.3 build used a gcc 4.9, and the Android 4.0 build used a gcc 4.8

Because in the NSS build system, we currently have the version hardcoded to 4.4.3, our ANDROID_TARGET appears to be just a default value that can be overridden - so it isn't a strict minimum dependency.

Maybe instead of hardcoding the compiler version in Linux.mk, we should probably require another environment variable for the compiler version (I see that NDK ships with multiple alternative versions).

Comment 10

2 years ago
Comment on attachment 8670616 [details] [diff] [review]
bug1211725-2.patch

With this patch, I still get this error, is that your intention?

sha512.c: In function 'SHA256_Compress':
sha512.c:132:24: error: assignment makes integer from pointer without a cast [-Werror]
 #define BYTESWAP4(x) x = SHA_HTONL(x)

Comment 11

2 years ago
(In reply to Kai Engert (:kaie) from comment #10)
> sha512.c: In function 'SHA256_Compress':
> sha512.c:132:24: error: assignment makes integer from pointer without a cast
> [-Werror]
>  #define BYTESWAP4(x) x = SHA_HTONL(x)


Would you like to fix all build errors on Android prior to landing NSS 3.21 into mozilla-inbound? Then your patch will still produce errors, and your patch is fine.

(If you'd rather like more time to fix the problems, and allows us to land NSS 3.21 earlier, then we could use flags that still tolerate warnings on Android. Up to you.)

Comment 12

2 years ago
Created attachment 8671264 [details] [diff] [review]
flexible-android-toolchain.patch

Using this patch, building on Android should work, in an environment that uses minimal overriding for build flags / compilers.

It shouldn't influence the Firefox builds. I checked the build logs, and the Firefox build uses different flags and overrides the compiler versions.

(In particular, the Firefox build doesn't set the ANDROID_TARGET variable, which the NSS build system requires.)

I'd like to land this patch to enable our NSS buildbot Android builder to work again.
Attachment #8670400 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8671264 - Flags: review?(martin.thomson)

Comment 13

2 years ago
Comment on attachment 8670616 [details] [diff] [review]
bug1211725-2.patch

r=kaie if build errors is what you want.
Attachment #8670616 - Flags: review?(kaie) → review+
(Reporter)

Comment 14

2 years ago
Comment on attachment 8671264 [details] [diff] [review]
flexible-android-toolchain.patch

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

::: Makefile
@@ +55,5 @@
>  # Translate coreconf build options to NSPR configure options.
>  #
>  
>  ifeq ($(OS_TARGET),Android)
> +NSPR_CONFIGURE_OPTS += --with-android-ndk=$(ANDROID_NDK) --target=arm-linux-androideabi --with-android-version=$(OS_TARGET_RELEASE) --with-android-toolchain=$(ANDROID_NDK)/toolchains/arm-linux-androideabi-$(ANDROID_TOOLCHAIN_VERSION)/prebuilt/linux-x86 --with-android-platform=$(ANDROID_NDK)/platforms/android-8/arch-arm

This should draw on the existing variables...

NSPR_CONFIGURE_OPTS += \
  --with-android-ndk=$(ANDROID_NDK) \
  --target=$(ANDROID_PREFIX) \
  --with-android-version=$(OS_TARGET_RELEASE) \
  --with-android-toolchain=$(ANDROID_TOOLCHAIN) \
  --with-android-platform=$(ANDROID_SYSROOT)
Attachment #8671264 - Flags: review?(martin.thomson)

Comment 15

2 years ago
Created attachment 8671553 [details] [diff] [review]
flexible-android-toolchain-2.patch

(In reply to Martin Thomson [:mt:] from comment #14)
> This should draw on the existing variables...

Thanks for finding this simplification!
Attachment #8671264 - Attachment is obsolete: true
Attachment #8671553 - Flags: review?(martin.thomson)
(Reporter)

Comment 16

2 years ago
Comment on attachment 8671553 [details] [diff] [review]
flexible-android-toolchain-2.patch

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

Fix --target and we're good.
Attachment #8671553 - Flags: review?(martin.thomson) → review+

Comment 17

2 years ago
Created attachment 8671563 [details] [diff] [review]
flexible-android-toolchain-3.patch

oops, I had missed one simplification.
Attachment #8671553 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8671563 - Flags: review+

Comment 18

2 years ago
I checked in all three patches.
https://hg.mozilla.org/projects/nss/rev/65cd77878e18
https://hg.mozilla.org/projects/nss/rev/72caf53cc7cf
https://hg.mozilla.org/projects/nss/rev/1f20a59a4275

We expected it would work, with only Android to fail.

However, we saw failures on multiple platforms.

I've backed out the change of compiler flags:
https://hg.mozilla.org/projects/nss/rev/2f6ef09e9495

This should now build again on all platforms, except an Android, where gcc-4.8 should now be enabled, and we should see the build warnings/errors that we need to fix.
(Reporter)

Comment 19

2 years ago
Created attachment 8671576 [details] [diff] [review]
empty-body.patch

I am trying this change out, with -Wno-error=empty-body, which was one of the newly added build flags.  That seemed to be the problem last time, but it's not clear if some of the others are safe yet, so we'll see.

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee209bd85274
(Reporter)

Comment 20

2 years ago
OK, there are several problems here:

1. clang doesn't seem to respect -Wno-error=sign-compare
2.
 13:59:11     INFO -  pkix_tools.c: In function 'pkix_i2hex':
 13:59:11     INFO -  pkix_tools.c:482:20: error: comparison is always true due to limited range of data type [-Werror=type-limits]
 13:59:11     INFO -           if ((digit >= 0)&&(digit <= 9))
 13:59:11     INFO -                      ^
3.
 13:59:51 INFO - certutil.c:625: error: undefined reference to '__PK11_SetCertificateNickname'

Comment 21

2 years ago
Created attachment 8671824 [details] [diff] [review]
build2
Attachment #8671824 - Flags: review?(martin.thomson)

Updated

2 years ago
Attachment #8670134 - Flags: checked-in+

Updated

2 years ago
Attachment #8671563 - Flags: checked-in+

Comment 22

2 years ago
Comment on attachment 8671824 [details] [diff] [review]
build2

Martin, unfortunately you try build didn't work.

I'd like to suggest, on Android, to temporarily use the Firefox build flags, allowing us to immediately start with testing NSS 3.21 beta on mozilla-central, using this patch.

I've started a try build with NSS trunk and this patch at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69682b301f46

Comment 23

2 years ago
Comment on attachment 8671824 [details] [diff] [review]
build2

this one still fails on Android
Attachment #8671824 - Flags: review?(martin.thomson)

Comment 24

2 years ago
(In reply to Martin Thomson [:mt:] from comment #20)
>  13:59:51 INFO - certutil.c:625: error: undefined reference to
> '__PK11_SetCertificateNickname'

I don't see this failure on our NSS builders.

We introduced this new special exported API in bug 1142209 for NSS 3.21.

If certutil is built as part of a Firefox build, does it use the specially modified versions of the NSS libraries? Then the Firefox build probably needs to add the above new symbol to the .def file that Firefox uses.
(Reporter)

Comment 25

2 years ago
Let's add PK11_SetCertificateNickname to nss.def when this lands.
(Reporter)

Comment 26

2 years ago
Comment on attachment 8671824 [details] [diff] [review]
build2

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

::: coreconf/Linux.mk
@@ +162,5 @@
>  endif
>  ifeq (true,$(NSS_HAS_GCC48))
> +
> +ifeq ($(OS_TARGET),Android)
> +OS_CFLAGS += -Wall -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits -Werror=char-subscripts -Werror=comment -Werror=endif-labels -Werror=enum-compare -Werror=ignored-qualifiers -Werror=int-to-pointer-cast -Werror=multichar -Werror=nonnull -Werror=pointer-arith -Werror=pointer-sign -Werror=return-type -Werror=sequence-point -Werror=trigraphs -Werror=uninitialized -Werror=unknown-pragmas -Wno-unused -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds

Can you remove -Wno-unused ?  We shouldn't be tripping that.  And I don't think that we need -Wno-error= options here.
Attachment #8671824 - Flags: review+

Updated

2 months ago
Blocks: 1399364
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.