1211725 - signed/unsigned comparisons in pk11akey.c

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Martin Thomson [:mt:]]

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 -               ^

Comment 1

[Martin Thomson [:mt:]]

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.

Comment 2

[Martin Thomson [:mt:]]

Attachment: 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.

Comment 3

[Kai Engert (:KaiE:)]

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

[Kai Engert (:KaiE:)]

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

[Kai Engert (:KaiE:)]

Attachment: 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.

Comment 6

[Martin Thomson [:mt:]]

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).

Comment 7

[Martin Thomson [:mt:]]

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.

Comment 8

[Martin Thomson [:mt:]]

Attachment: 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.

Comment 9

[Kai Engert (:KaiE:)]

(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

[Kai Engert (:KaiE:)]

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

[Kai Engert (:KaiE:)]

(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

[Kai Engert (:KaiE:)]

Attachment: 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.

Comment 13

[Kai Engert (:KaiE:)]

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

r=kaie if build errors is what you want.

Comment 14

[Martin Thomson [:mt:]]

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)

Comment 15

[Kai Engert (:KaiE:)]

Attachment: 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!

Comment 16

[Martin Thomson [:mt:]]

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.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: flexible-android-toolchain-3.patch

oops, I had missed one simplification.

Comment 18

[Kai Engert (:KaiE:)]

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.

Comment 19

[Martin Thomson [:mt:]]

Attachment: 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

Comment 20

[Martin Thomson [:mt:]]

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

[Kai Engert (:KaiE:)]

Attachment: build2

Comment 22

[Kai Engert (:KaiE:)]

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

[Kai Engert (:KaiE:)]

Comment on attachment 8671824 [details] [diff] [review]
build2

this one still fails on Android

Comment 24

[Kai Engert (:KaiE:)]

(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.

Comment 25

[Martin Thomson [:mt:]]

Let's add PK11_SetCertificateNickname to nss.def when this lands.

Comment 26

[Martin Thomson [:mt:]]

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.