1182667 - Compile NSS with -Werror

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

Attachment: werr.patch

I've stamped out all the errors that gcc and clang can produce.

Most of this was easy.  Some were not.  I believe that I have not changed any functionality.  Tests pass, though I'm running another check now.

Comment 1

[Robert Relyea]

Comment on attachment 8632316 [details] [diff] [review]
werr.patch

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

I've finished reviewing everything under command. I don't have any real problems with anything I've found, but I did find a few things that should probably be fixed.

::: cmd/certcgi/certcgi.c
@@ +359,1 @@
>  static CERTCertificateRequest *

Does certcgi even work still?

::: cmd/crlutil/crlutil.c
@@ +132,2 @@
>  	        cert = CERT_FindCertByName(certHandle,
>  	                                   &crlNode->crl->crl.derName);

Probably should change this to crlNote->crl->crl.derName.data != NULL rather than just droping the test. Clearly the origininal &clrNode->crl->crl.derName is wrong.

::: cmd/crmftest/testcrmf.c
@@ +131,4 @@
>         PORT_SetError(SEC_ERROR_INVALID_ARGS);
>         return SECFailure;
> +    }
> +    (void)PK11_GenerateRandom((unsigned char *)dest, sizeof(long));

We shout act on the error code here rather than remove it. If the PK11_GenerateRanom fails, we should return SECFailure. PK11_GenerateRandom will set the error code if it failed.

::: cmd/fipstest/fipstest.c
@@ +2692,1 @@
>              

I can't help but think we really need to pass the predictionResistance flag to drbg.

::: cmd/lib/secutil.c
@@ +359,5 @@
>      }
>  
>      if (PK11_NeedUserInit(slot)) {
>  	newpw = secu_InitSlotPassword(slot, PR_FALSE, &pwdata);
> +	(void)PK11_InitPin(slot, (char*)NULL, newpw);

We should act on the failure here rather than ignore it. SECU_ChangePW2 does return an error code, we shouldn't fail silently.

::: cmd/modutil/error.h
@@ +132,5 @@
>  
>  	LAST_MSG  /* must be last */
>  } Message;
>  
> +extern char *msgStrings[];

We don't have any sizeof(msgStrings) anywhere? Must not since it all compliled.
Maybe a comment here that msgStrings is defined in modutil.c..

::: cmd/modutil/installparse.c
@@ +202,5 @@
>  #if YYDEBUG
>      register char *yys;
>      extern char *getenv();
>  
> +    if ((yys = getenv("YYDEBUG")))

At this point maybe adding the explicit != NULL here would make things clearer..

@@ +219,5 @@
>      yyvsp = yyvs;
>      *yyssp = yystate = 0;
>  
>  yyloop:
> +    if ((yyn = yydefred[yystate])) goto yyreduce;

Same here.

::: cmd/ssltap/ssltap.c
@@ +581,3 @@
>      }
>      q += p;
>      PR_fprintf(PR_STDOUT,"}\n");

I'm not sure what compiler warning triggered this change, but there is a change in semantic. In the old code the full session-id label is dropped if chv2->sidLength == 0. In the new code we print and emply session-id. NOTE: there's a bug in the old code where the q+=p and the PR_fprintf(STDOUT,"}\n"); Was not correctly nested inside the if.

Comment 2

[Martin Thomson [:mt:]]

(In reply to Robert Relyea from comment #1)
> I've finished reviewing everything under command. I don't have any real
> problems with anything I've found, but I did find a few things that should
> probably be fixed.

Thanks for being so prompt about it :)

> ::: cmd/certcgi/certcgi.c
> @@ +359,1 @@
> >  static CERTCertificateRequest *
> 
> Does certcgi even work still?

Well, I found some real crazy code in there.  Maybe we should discuss deleting it.

> ::: cmd/crmftest/testcrmf.c
> @@ +131,4 @@
> >         PORT_SetError(SEC_ERROR_INVALID_ARGS);
> >         return SECFailure;
> > +    }
> > +    (void)PK11_GenerateRandom((unsigned char *)dest, sizeof(long));
> 
> We shout act on the error code here rather than remove it. If the
> PK11_GenerateRanom fails, we should return SECFailure. PK11_GenerateRandom
> will set the error code if it failed.

I tried hard (though I clearly failed) to not change behaviour, but I generally agree.  There were several of these.

> ::: cmd/ssltap/ssltap.c
> @@ +581,3 @@
> >      }
> >      q += p;
> >      PR_fprintf(PR_STDOUT,"}\n");
> 
> I'm not sure what compiler warning triggered this change, but there is a
> change in semantic. 

Hmm, I recall there being an unsigned comparison there, but this is a non-zero test.  I'll back this change out and see what complains.

Comment 3

[Martin Thomson [:mt:]]

Attachment: tmp

This one doesn't work yet, but it's progress toward a win32 patch.

Comment 4

[Martin Thomson [:mt:]]

Attachment: werror.patch

This addresses Bob's notes in cmd/ and adds a whole lot more changes to get the build working on Windows as well under -WX.  I still need another round-trip to windows to iron out any last flaws though.

Comment 5

[Robert Relyea]

martin I'll be a way for a week and a half. I'll take a look when I get back.

Comment 6

[Martin Thomson [:mt:]]

Attachment: werr-1.patch

This addresses Bob's comments and a few other snafus.  I've also added switched windows builds to -WX.

Comment 7

[Robert Relyea]

Comment on attachment 8634919 [details] [diff] [review]
werr-1.patch

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

r+ for everything except:
   under cmd shlibsign... The removed verify parameters should go remain as a comment, I think the change to shlibsign.sh was an accident. If it was intential, just drop a comment in the bug as to why and I'll probably be OK with it.
   The following instances where the version reference was removed: ldvector.c (freebl), pkcs11.c (softoken), lginit.c (nssdbm), smimeutil.c (smime), nssinit.c (nss), sslcon.c (ssl), secoid.c (util) In those cases where the volatile c = version[0] code wasn't the only change, the rest of the changes in those files are fine.
   The following instance where I think we should be returning an error code back to the caller on error rather than ignoring it: certdb.c. All the other instances I think it's legit to throw away the error code (and in some cases commented as such).
   The following instances where a for loop was changed to a do loop (apparently because the variable changed to an unsigned, so could no longer terminate when negative): ecl_mult.c ecp_jac.c mpi.c sftkdb.c. I prefer they stay for loops since, 1) part of doing a for loop is signaling start, end and increment conditions clearly, and 2) checking i > a value is a little more robust in the case where i somehow get's decremented twice (my paranoia). We can preserve the for loops by either a) restoring the variables to signed (and casting or creating a temp unsigned) or b) keeping the variable unsigned, but casting before the compare). I'm willing to hear arguments for why the do{}while loops are better... In which case I'll want to look at the loops again and make sure the original loops can't have the null loop case (the case where we skip the loop because the exit condition exists before we start.
    certdb.c, we should probably fix the bug that the warnings are pointing out rather than remove the code that starts to try to handle the bug.
    I'd like to run the CAVS tests on the drbg once we've checked in to make sure we don't have some carry code issues in the drbg. That can happen after checkin.
    I've asked for some comments. Those would be nice, but not necessary.

bob

PS. Thanks for dealing with the tedious work needed to create this patch.

bob

::: cmd/bltest/blapitest.c
@@ +3707,5 @@
>  
> +print_usage:
> +        if (cipherInfo) {
> +            PORT_Free(cipherInfo);
> +        }

It's weird you got a warning here, PORT_Free works with a NULL pointer just like free does, but most of NSS has this paranoia check, so I prefer your new code as matching NSS style better anway.

::: cmd/fipstest/fipstest.c
@@ +2566,2 @@
>      PRBool predictionResistance = PR_FALSE;
> +#endif

Thanks for silencing the warnings, but keeping the notion that we need to add the preditionResistance code.

::: cmd/lib/secutil.c
@@ +405,1 @@
>  }

Looks like the tools caught a leak.:)

::: cmd/shlibsign/shlibsign.c
@@ -375,5 @@
> -    0xde, 0xc9, 0x9f, 0x04, 0x87, 0xcf, 0x4f, 0x86,
> -    0xc3, 0x29, 0x7d, 0xb7, 0x89, 0xbf, 0xe3, 0xde };
> -
> -static const unsigned int counter2=210;
> -

These should stay around as comments so we can verify the pqg parameters in this file.

::: cmd/shlibsign/sign.sh
@@ +28,5 @@
>          fi
>          PATH=${ARG1}/lib:${ARG1}/bin:${ARG4}:${PATH}
>      fi
>      export PATH
> +    echo $PATH

Is this a debugging that got sucked up in your patch, or a change you actually want to keep?

::: lib/certdb/certdb.c
@@ +2494,3 @@
>  		} else {
> +		    (void)CERT_AddTempCertToPerm(certs[i],
> +                                                 nickname?nickname:canickname, NULL);

Another case where we should send report the error back to the caller of CERT_ImportCerts

::: lib/certdb/crl.c
@@ +639,5 @@
>  
>      /* we can't use the cache here because we must look in the same
>         token */
> +    (void)SEC_FindCrlByKeyOnSlot(slot, &newCrl->crl.derName, type,
> +                                 &oldCrl, CRL_DECODE_SKIP_ENTRIES);

OK, in this case if we fail, we are expecting that oldCrl will be NULL..

@@ +2691,5 @@
>                  rv = SECFailure;
>              }
>              if (SECFailure == rv)
>              {
> +                (void)CERT_FindCRLEntryReasonExten(entry, &reason);

In this case we are already going to fail, so no need to pass the error code.

::: lib/certhigh/certhigh.c
@@ +47,5 @@
>      nickname1++;
>      if (PORT_Strcmp(nickname1,nickname2) != 0) {
>  	return PR_FALSE;
>      }
>      /* compare the other token with the internal slot here */

So the issuer here is we don't actually compare the token name with the internal token. The removed code was part of setting up the test, but not the actual test. I wonder if we shouldn't actually lookup token and see if it's an alias for the internal token rather than remove the code that finds the token name.

::: lib/freebl/drbg.c
@@ +360,4 @@
>      PRNG_ADD_BITS_AND_CARRY(V(rng), VSize(rng), rng->reseed_counter, 
> +					sizeof rng->reseed_counter, carry)
> +    carry = 1;
> +    PRNG_ADD_CARRY_ONLY(rng->reseed_counter,(sizeof rng->reseed_counter)-1, carry);

Before we check this change in, I want to run the CAVS tests to make sure we didn't break FIPS in the drbg.

::: lib/freebl/ecl/ecl_mult.c
@@ +237,5 @@
>  	mp_zero(ry);
>  
> +        i = d;
> +	do {
> +                --i;

I think the for look is easer to read than the do/while. I'd rather keep i as signed then, especially since d is signed. may be a temp unsigned variable we assign i to inside the loop.

::: lib/freebl/ecl/ecp_jac.c
@@ +494,5 @@
>  	MP_CHECKOK(mp_init(&rz));
>  	MP_CHECKOK(ec_GFp_pt_set_inf_jac(rx, ry, &rz));
>  
> +        i = d;
> +        do {

Same as the previous comment about changing for to do ...

::: lib/freebl/ldvector.c
@@ -298,5 @@
> -
> -    /* force a reference that won't get optimized away */
> -    volatile char c;
> -
> -    c = __nss_freebl_version[0];

This is meant to force a reverence to nss_freebl_version so the linker doesn't optimize it away. We've told the compilier that c is volatile, which means it has side affects, it should not be complaining here!

::: lib/freebl/mpi/mpi-priv.h
@@ +254,5 @@
>                              mp_digit divisor, mp_digit *quot, mp_digit *rem);
>  
>  /* c += a * b * (MP_RADIX ** offset);  */
>  #define s_mp_mul_d_add_offset(a, b, c, off) \
> +  s_mpv_mul_d_add_prop(MP_DIGITS(a), MP_USED(a), b, MP_DIGITS(c) + off)

OK, you handled this by changing the caller not to expect a return code. That's reasonable. We probably should include a comment here and where it's called saying it should be treated as a (void) function.

@@ -254,5 @@
>                              mp_digit divisor, mp_digit *quot, mp_digit *rem);
>  
>  /* c += a * b * (MP_RADIX ** offset);  */
>  #define s_mp_mul_d_add_offset(a, b, c, off) \
> -(s_mpv_mul_d_add_prop(MP_DIGITS(a), MP_USED(a), b, MP_DIGITS(c) + off), MP_OKAY)

Hmm, this is a semantic change, though I'm not sure why the original semantic. Basically, this cases s_mp_mul_d_add_offset to always return MP_OKAY. s_mpv_mul_d_add_prop() is an assembler file. It may not actually return anything, so the point of this macro is to make the code accept calling the assembler, but treating it as if it returns OK.

::: lib/freebl/mpi/mpi.c
@@ +2934,2 @@
>      DIGIT(mp, ix + p) = DIGIT(mp, ix);
> +  } while(ix > 0);

yet another for() turned into do(). I'm not sure why, since in this case ix is still unsigned.

::: lib/freebl/mpi/mpmontg.c
@@ +46,5 @@
>    MP_CHECKOK( s_mp_pad(T, i) );
>    for (i = 0; i < MP_USED(&mmm->N); ++i ) {
>      mp_digit m_i = MP_DIGIT(T, i) * mmm->n0prime;
>      /* T += N * m_i * (MP_RADIX ** i); */
> +    s_mp_mul_d_add_offset(&mmm->N, m_i, T, i);

Ah, you handled the always return true at the caller.

::: lib/nss/nssinit.c
@@ -1247,5 @@
>      int vmajor = 0, vminor = 0, vpatch = 0, vbuild = 0;
>      const char *ptr = importedVersion;
> -    volatile char c; /* force a reference that won't get optimized away */
> -
> -    c = __nss_base_version[0];

Same issue as freebl, and s/mime

::: lib/smime/smimeutil.c
@@ -769,5 @@
>       * patch releases.
>       */
> -    volatile char c; /* force a reference that won't get optimized away */
> -
> -    c = __nss_smime_version[0];

Same thing a freebl...

::: lib/softoken/legacydb/pcertdb.c
@@ +3718,5 @@
>  			if ( subjectEntry->emailAddrs[0] ) {
>  			    PORT_Memcpy(subjectEntry->emailAddrs[0], emailAddr,
>  				    key.size - 1);
>  			    subjectEntry->nemailAddrs = 1;
> +			    (void)WriteDBSubjectEntry(handle, subjectEntry);

I think these are OK to throw the error code away...

::: lib/softoken/pkcs11.c
@@ -3145,3 @@
>      CHECK_FORK();
>      
> -    c = __nss_softokn_version[0];

Just like freebl, we need this reference to force the linker to pull in the string.

::: lib/softoken/sftkdb.c
@@ +1055,5 @@
>  	return CKR_OK;
>      }
>  
> +    end = attr->ulValueLen - 1;
> +    do {

Again, a for loop turned into a do loop...

::: lib/ssl/sslcon.c
@@ -3684,5 @@
>       * patch releases.
>       */
> -    volatile char c; /* force a reference that won't get optimized away */
> -
> -    c = __nss_ssl_version[0];

Same comment as freebl, S/MIME, and nss init

::: lib/util/secoid.c
@@ -1914,5 @@
>      int i;
>      char * envVal;
> -    volatile char c; /* force a reference that won't get optimized away */
> -
> -    c = __nss_util_version[0];

Same comment as freebl, S/MIME, nss init, and ssl

Comment 8

[Robert Relyea]

OK, you handled this by changing the caller not to expect a return code. That's reasonable. We probably should include a comment here and where it's called saying it should be treated as a (void) function.

@@ -254,5 @@
>                              mp_digit divisor, mp_digit *quot, mp_digit *rem);
>  
>  /* c += a * b * (MP_RADIX ** offset);  */
>  #define s_mp_mul_d_add_offset(a, b, c, off) \
> -(s_mpv_mul_d_add_prop(MP_DIGITS(a), MP_USED(a), b, MP_DIGITS(c) + off), MP_OKAY)

Hmm, this is a semantic change, though I'm not sure why the original semantic. Basically, this cases s_mp_mul_d_add_offset to always return MP_OKAY. s_mpv_mul_d_add_prop() is an assembler file. It may not actually return anything, so the point of this macro is to make the code accept calling the assembler, but treating it as if it returns OK.

In case it's confusing, the latter comment was made before I saw that you changed the caller, so I'm OK with this change, preferably with a comment added.

Comment 9

[Martin Thomson [:mt:]]

I've made the requested changes, but I want to do one last run back on my linux box before giving you a new diff and interdiff to look at.  Thanks for reviewing.

(In reply to Robert Relyea from comment #7)
>    under cmd shlibsign... The removed verify parameters should go remain as
> a comment, I think the change to shlibsign.sh was an accident. If it was
> intential, just drop a comment in the bug as to why and I'll probably be OK
> with it.

Kai already did that and I took his changes on rebase.

>    The following instances where the version reference was removed:

I'd have to say that that is a bit of a kludge.  Fixing this required some #pragma magic.  I don't want to lose the warnings elsewhere.

>    The following instance where I think we should be returning an error code
> back to the caller on error rather than ignoring it: certdb.c. All the other
> instances I think it's legit to throw away the error code (and in some cases
> commented as such).

Right, I'll have the call fail; a little bit of extra plumbing required though.

>    The following instances where a for loop was changed to a do loop
> (apparently because the variable changed to an unsigned, so could no longer
> terminate when negative): ecl_mult.c ecp_jac.c mpi.c sftkdb.c. I prefer they
> stay for loops [...]

I've changed these back to for loops, but with the following change:

- for (i = max - 1; i >= 0; --i) {
+ for (i = max; i-- > 0; ) {

It loses the separation between condition and decrement, but it is correct and it keeps the looping logic localized, which is the biggest problem with the do{}while change.  Do you think a comment about using the "down to" operator (-->) would help make it clearer?

The one in sftkdb.c is clearer with the extra condition moved inside the loop, I think.

> ::: cmd/lib/secutil.c
> @@ +405,1 @@
> >  }
> 
> Looks like the tools caught a leak.:)

In this case, only by accident :)  There was an uninitialized variable as well.

> ::: lib/certhigh/certhigh.c
> @@ +47,5 @@
> >      nickname1++;
> >      if (PORT_Strcmp(nickname1,nickname2) != 0) {
> >  	return PR_FALSE;
> >      }
> >      /* compare the other token with the internal slot here */
> 
> So the issuer here is we don't actually compare the token name with the
> internal token. The removed code was part of setting up the test, but not
> the actual test. I wonder if we shouldn't actually lookup token and see if
> it's an alias for the internal token rather than remove the code that finds
> the token name.

I'm not familiar enough with this code to do this; I can add a comment or open a bug to track the need for a change if you like.
 
> Before we check this change in, I want to run the CAVS tests to make sure we
> didn't break FIPS in the drbg.

If I can help out, let me know.

Comment 10

[Robert Relyea]

Thanks Martin, Those all sound good,

I'll run the CAVS dest for DRBG once you check in.

A comment and open bug for the nickname issue is fine.

r+ for everything now.

Comment 11

[Martin Thomson [:mt:]]

Attachment: bug1182667.patch

Hi Bob, this is the changes you requested.  I'll attach an interdiff so that you can see the changes.

I fixed the __nss_xxx_version volatile references with a new header file that suppresses the warnings locally.

I changed the loops as discussed.

I added a number of default values for variables in test files; these are probably not a strictly issue, but the compiler complained.

Some compilers complained about unused variables in optimized modes, when they were only used in PORT_Assert; I added a new #define for PORT_AssertSuccess that runs the code and asserts for success in debug builds.

Comment 12

[Martin Thomson [:mt:]]

Attachment: inter.diff

Flagging this as a patch so that splinter does the right thing; it's just an interdiff.

Comment 13

[Robert Relyea]

Comment on attachment 8645808 [details] [diff] [review]
inter.diff

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

The (type) ((char *)ptr - (char *)NULL) cast really is crying out for a macro. something like

#define NSS_PTR_TO_SCALAR(x)  ((char *)(x)-(char *)NULL)

r+ with or without that change.

Comment 14

[Robert Relyea]

Comment on attachment 8645807 [details] [diff] [review]
bug1182667.patch

r+ based on my review of the interdiff and review of the previous patch.

Comment 15

[Martin Thomson [:mt:]]

(In reply to Robert Relyea from comment #13)
> #define NSS_PTR_TO_SCALAR(x)  ((char *)(x)-(char *)NULL)

I thought about that, but it's such an abuse that I didn't want to encourage more use of it.  What I wanted to do was fix all these conversions by changing the call site for each of these, but that can wait I suspect.

Comment 16

[Wan-Teh Chang]

Comment on attachment 8645808 [details] [diff] [review]
inter.diff

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

::: lib/ckfw/hash.c
@@ +47,5 @@
>  (
>    const void *key
>  )
>  {
> +  return (PLHashNumber)((char *)key - (char *)NULL);

This compiler warning can also be avoided by first casting the pointer to
an integer type large enough to hold a pointer, such as intptr_t or
uintptr_t.

  return (PLHashNumber)(uintptr_t)key;

I agree that this kind of trick probably should be a macro, so that we
can document it in one place.

By the way, do you think we should consider all 64-bits of the pointer
(on a 64-bit system) when converting a pointer to a (32-bit) hash number?

::: lib/util/secport.h
@@ +87,5 @@
>  SEC_END_PROTOS
>  
>  #define PORT_Assert PR_ASSERT
> +/* Unlike PORT_Assert, which does nothing in an optimized build, PORT_AssertSuccess
> + * is run all the time. */

This kind of difference violates the principle of least surprise.
We should try to avoid it.

Comment 17

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/4355f55afeb2

I'll file follow-up bugs for the issues that Wan-Teh raises.

Comment 18

[Martin Thomson [:mt:]]

On second thought, I'll just leave this open.

Comment 19

[Martin Thomson [:mt:]]

(In reply to Wan-Teh Chang from comment #16)
> Comment on attachment 8645808 [details] [diff] [review]
> inter.diff
> 
> Review of attachment 8645808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/ckfw/hash.c
> @@ +47,5 @@
> >  (
> >    const void *key
> >  )
> >  {
> > +  return (PLHashNumber)((char *)key - (char *)NULL);
> 
> This compiler warning can also be avoided by first casting the pointer to
> an integer type large enough to hold a pointer, such as intptr_t or
> uintptr_t.
> 
>   return (PLHashNumber)(uintptr_t)key;

That does seem to pass muster on gcc and clang.  I seem to recall having different results with MSVC though; I'll have to check on that.  I'm also unsure about availability of uintptr_t on all platforms.

> By the way, do you think we should consider all 64-bits of the pointer
> (on a 64-bit system) when converting a pointer to a (32-bit) hash number?

Maybe; the odds of that being relevant are pretty slim though.  Assuming that the double-cast is OK and uintptr_t is available, then maybe:

/* Using the PLHashNumberFromPointer macro is not recommended. */
#if sizeof(void*) > (PL_HASH_BITS / 8)
#define PLHashNumberFromPointer(p) (PLHashNumber)(((uintptr_t)(p) >> PL_HASH_BITS) ^ (uintptr_t)(p))
#else
#define PLHashNumberFromPointer(p) (PLHashNumber)(uintptr_t)(p)
#endif

>> ... PORT_AssertSuccess ...
> 
> This kind of difference violates the principle of least surprise.
> We should try to avoid it.

Would a different name help?  I can name this PORT_CheckSuccess(), PORT_ExpectSuccess().  I'm having trouble thinking of anything better.

Comment 20

[Wan-Teh Chang]

(In reply to Martin Thomson [:mt:] from comment #19)
> I'm also unsure about availability of uintptr_t on all platforms.

I think it is fine to assume uintptr_t (a C99 type) is available on
all platforms because it piggybacks on the upgrade to C++11 compilers.

We can use the poorly-named NSPR type PRUword instead of uintptr_t.

Re: PORT_AssertSuccess: It is indeed the "Assert" in the name that's
problematic. I wonder if we should try defining PR_ASSERT so that
the code is still compiled in optimized/release builds, something
like this:

#ifdef NDEBUG
const int nspr_assert_enabled = 0;
#else
const int nspr_assert_enabled = 1;
#endif

#define PR_ASSERT(cond) \
    if (nspr_assert_enabled) { \
        if (!(cond)) {
            fprintf(stderr, "error message\n");
            abort();
        }
    }

Comment 21

[Martin Thomson [:mt:]]

I'll give PRUword a spin.

I really wanted to avoid making changes to NSPR, but you are right that that could be a way to avoid the unused argument problem.  Unfortunately, defining a const value results in duplicate symbols.

Also, NSPR doesn't compile with that change, and likely other projects as well.  There are a number of places that look like this:

#ifdef DEBUG
int x = 1;
#endif
...
PR_ASSERT(x);

See: https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptthread.c#1286-1313

Comment 22

[Martin Thomson [:mt:]]

Attachment: bug1158489-1.patch

I had to make a few changes to get this working on OS X.  Notably, I have disabled cross process semaphores.  They weren't working there anyway (see bug 1192500 for details).

Comment 23

[Martin Thomson [:mt:]]

Attachment: bug1158489-2.patch

This just replaces PORT_AssertSuccess with PORT_CheckSuccess throughout.  Happy to use a better name if someone has one.

Comment 24

[Martin Thomson [:mt:]]

Backed out:
 https://hg.mozilla.org/projects/nss/rev/6ba5356341a8

Errors fall into three classes:
old compilers (yuck)
-Werror affecting more systems than intended
some likely real bugs on Windows

Comment 25

[Martin Thomson [:mt:]]

Attachment: bug1182667.patch

Here's what I intend to submit today; if you have a few minutes to take a look Bob, that would be good.

Comment 26

[Martin Thomson [:mt:]]

Attachment: interdiff.patch

Interdiff for the above.

Comment 27

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/276a21813f7c

Leaving open for follow-ups.

Comment 28

[Wan-Teh Chang]

Re: PORT_CheckSuccess

Martin, I think the right thing to do is to add proper error
handling. The assumption that we can rely on developers running
debug builds is increasingly dubious because it may be too slow
to run a debug build of Firefox, for example.

There may be cases where we know a function call cannot fail
because we've already checked some preconditions. In those
cases I think PORT_CheckSuccess would be appropriate.

So, I think at least a warning comment should be added to
PORT_CheckSuccess to discourage its use in place of proper
error handling.

Comment 29

[Martin Thomson [:mt:]]

That's reasonable, does this sound OK?

+/* This runs a function that should return SECSuccess.
+ * The return value is asserted in a debug build, otherwise it is ignored.
+ * This is no substitute for proper error handling.  It is OK only if you
+ * have ensured that the function cannot fail by other means such as checking
+ * prerequisites.  In that case this can be used as a safeguard against
+ * unexpected changes in a function.
+ */

Comment 30

[Wan-Teh Chang]

r=wtc. Thanks, Martin! Should we also say it is intended for
NSS internal use?

Comment 31

[Wan-Teh Chang]

Attachment: Don't assign an enum value (int) to the |data| member (unsigned char *) of a SECItem.

Comment 32

[Wan-Teh Chang]

Comment on attachment 8651334 [details] [diff] [review]
Don't assign an enum value (int) to the |data| member (unsigned char *) of a SECItem.

Martin: this patch fixes the current compilation error on Windows:

cl -FoWIN954.0_DBG.OBJ/pkix_pl_ldapdefaultclient.obj -c -Zi -FdWIN954.0_DBG.OBJ/
 -Od -W3 -WX -nologo -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -MD -w
44267 -w44244 -w44018 -FS -arch:IA32 -DXP_PC -DSHLIB_SUFFIX=\"dll\" -DSHLIB_PREF
IX=\"\" -DSHLIB_VERSION=\"\" -DDEBUG -D_DEBUG -UNDEBUG -DDEBUG_Wan_Teh_Chang -DW
IN32 -D_X86_ -D_WINDOWS -DWIN95 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_D
ISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../../../dist/WIN954.0_DBG.OBJ/incl
ude -I../../../../../dist/public/nss -I../../../../../dist/private/nss  "/c/src/
nss-tip.1/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c"
pkix_pl_ldapdefaultclient.c
c:/src/nss-tip.1/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c(
365) : error C2220: warning treated as error - no 'object' file generated
c:/src/nss-tip.1/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c(
365) : warning C4047: '=' : 'unsigned char *' differs in levels of indirection f
rom 'int'

When you debugged this compiler warning this afternoon, the line numbers in
the source tree you used seemed to be off by one.

I considered replacing that line with:

-        msg.protocolOp.op.bindResponseMsg.resultCode.data = OTHER;
+        msg.protocolOp.op.bindResponseMsg.resultCode.data = NULL;
+        msg.protocolOp.op.bindResponseMsg.resultCode.data = 0;

But this turns out to be unnecessary because the
pkix_pl_LdapDefaultClient_DecodeBindResponse function zeros the
entire LDAPMessage structure.

Note: pkix_pl_LdapDefaultClient_DecodeBindResponse performs ASN.1
DER decoding. After decoding, the |resultCode| SECItem will point
to the resultCode, and we read the first byte pointed to by
|resultCode.data|. Ideally, we should first verify resultCode.len > 0
before doing that.

I also fixed two typecasts. In C, void * is implicitly converted to
any data pointer. In C++ a cast is required. So I followed the C++
requirement. I also removed an unnecessary cast to (char *).

Comment 33

[Wan-Teh Chang]

Attachment: Define PRNG_MAX_ADDITIONAL_BYTES as a macro to avoid the unused variable warning in 32-bit Mac OS X build

gcc -arch i386 -o Darwin14.4.0_DBG.OBJ/Darwin_SINGLE_SHLIB/drbg.o -c -g -fPIC -Di386 -Wall -fno-common -pipe -DDARWIN -DHAVE_STRERROR -DHAVE_BSD_FLOCK  -Werror -DXP_UNIX -DSHLIB_SUFFIX=\"dylib\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -UNDEBUG -DDEBUG_wtc -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_X86_OR_X64 -DNSS_X86 -DMP_USE_UINT_DIGIT -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE -DMP_ASSEMBLY_DIV_2DX1D -DMP_API_COMPATIBLE -I../../../dist/Darwin14.4.0_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl  drbg.c
drbg.c:27:22: error: unused variable 'PRNG_MAX_ADDITIONAL_BYTES'
      [-Werror,-Wunused-const-variable]
static const PRInt64 PRNG_MAX_ADDITIONAL_BYTES = LL_INIT(0x1, 0x0);
                     ^

PRNG_MAX_ADDITIONAL_BYTES is only used in some build configurations.

#if defined(NS_PTR_GT_32) || (defined(NSS_USE_64) && !defined(NS_PTR_LE_32))
    ...

    PR_STATIC_ASSERT(sizeof(size_t) > 4);

    if (bytes > (size_t)PRNG_MAX_ADDITIONAL_BYTES) {
        bytes = PRNG_MAX_ADDITIONAL_BYTES;
    }
#else

One way to fix this compiler warning is to put the variable definition
inside the same ifdef. But the name PRNG_MAX_ADDITIONAL_BYTES looks
like a macro, so I define it as a macro.

Comment 34

[Wan-Teh Chang]

Comment on attachment 8651344 [details] [diff] [review]
Define PRNG_MAX_ADDITIONAL_BYTES as a macro to avoid the unused variable warning in 32-bit Mac OS X build

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

::: lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c
@@ -361,5 @@
>  
>          decode.data = (void *)(client->rcvBuf);
>          decode.len = bufLen;
>  
> -        msg.protocolOp.op.bindResponseMsg.resultCode.data = OTHER;

Please ignore this change in the patch.

Comment 35

[Wan-Teh Chang]

Comment on attachment 8651334 [details] [diff] [review]
Don't assign an enum value (int) to the |data| member (unsigned char *) of a SECItem.

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

::: lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c
@@ -363,3 @@
>          decode.len = bufLen;
>  
> -        msg.protocolOp.op.bindResponseMsg.resultCode.data = OTHER;

Martin: it seems that this line causes a compiler warning on all
platforms. The successful builds all seem to have -Werror disabled.
Here is an example:

gmake[2]: Entering directory `/home/tinderbox/slavedir/rhel5-x64-OPT/hg/nss/lib/certdb'
../../coreconf/Linux.mk:169: Unable to find gcc >= 4.8 disabling -Werror

Comment 36

[Wan-Teh Chang]

Comment on attachment 8651334 [details] [diff] [review]
Don't assign an enum value (int) to the |data| member (unsigned char *) of a SECItem.

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

::: lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c
@@ -363,3 @@
>          decode.len = bufLen;
>  
> -        msg.protocolOp.op.bindResponseMsg.resultCode.data = OTHER;

I checked in the most important change in this patch first
to fix the compiler warning:
https://hg.mozilla.org/projects/nss/rev/ae6ffe7b3a90

The commit message explains why it is safe to remove this
statement.

Comment 37

[Wan-Teh Chang]

Comment on attachment 8651334 [details] [diff] [review]
Don't assign an enum value (int) to the |data| member (unsigned char *) of a SECItem.

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

::: lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c
@@ -363,3 @@
>          decode.len = bufLen;
>  
> -        msg.protocolOp.op.bindResponseMsg.resultCode.data = OTHER;

I copied the wrong function name in the commit message. Sigh.

Here is the corrected justification for the removal of this
statement:

This statement causes a compiler warning. It actually has no effect
because the pkix_pl_LdapDefaultClient_DecodeBindResponse call overwrites
the |msg| structure when it returns rv=SECSuccess, and we only inspect
|msg| when rv == SECSuccess.

Comment 38

[Wan-Teh Chang]

Comment on attachment 8651344 [details] [diff] [review]
Define PRNG_MAX_ADDITIONAL_BYTES as a macro to avoid the unused variable warning in 32-bit Mac OS X build

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

I checked in this patch first to fix build failures.
https://hg.mozilla.org/projects/nss/rev/6c5177e915db

The PR_INT64 macro was added in 2011:
https://hg.mozilla.org/projects/nspr/rev/8b7630e31292

This file (drbg.c) was added in 2009:
https://hg.mozilla.org/projects/nss/rev/ea9ccce5abba

So the PR_INT64 macro wasn't available then.

Comment 39

[Wan-Teh Chang]

Comment on attachment 8651334 [details] [diff] [review]
Don't assign an enum value (int) to the |data| member (unsigned char *) of a SECItem.

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

https://hg.mozilla.org/projects/nss/rev/ae6ffe7b3a90
https://hg.mozilla.org/projects/nss/rev/23910e8f017b

Comment 40

[Wan-Teh Chang]

Attachment: Fix a bug in the gcc >= 4.8 detection code

Comment 41

[Martin Thomson [:mt:]]

Comment on attachment 8652023 [details] [diff] [review]
Fix a bug in the gcc >= 4.8 detection code

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

::: coreconf/Darwin.mk
@@ +87,5 @@
>  NSS_HAS_GCC48 = true
>  endif
>  ifndef NSS_HAS_GCC48
>  NSS_HAS_GCC48 := $(shell \
> +  [ `$(CC) -dumpversion | cut -f 1 -d . -` -eq 4 -a \

Ahh, I see the problem, thanks for catching that.

Would -ge be a better choice here?

Comment 42

[Martin Thomson [:mt:]]

Oh, forget that last comment, -eq is perfect...

Comment 43

[Wan-Teh Chang]

Comment on attachment 8652023 [details] [diff] [review]
Fix a bug in the gcc >= 4.8 detection code

https://hg.mozilla.org/projects/nss/rev/a73ae1f6ac11

Comment 44

[Wan-Teh Chang]

Attachment: Fix "ignoring return value of ‘write’" compiler warnings in lib/dbm/h_page.c

I only see this when compiling with BUILD_OPT=1 under gcc 4.8:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/h_page.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DSTDC_HEADERS -DHAVE_STRERROR -DHAVE_SNPRINTF -DMEMMOVE -D__DBINTERFACE_PRIVATE -I../../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../../dist/public/dbm -I../../../../dist/private/dbm  h_page.c
h_page.c: In function ‘new_lseek’:
h_page.c:164:15: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
          write(fd, (char*)&buffer, (size_t)(1024 > len ? len : 1024));
               ^
h_page.c: In function ‘overflow_page’:
h_page.c:984:4: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
    (void)write(STDERR_FILENO, OVMSG, sizeof(OVMSG) - 1);
    ^
h_page.c:999:4: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
    (void)write(STDERR_FILENO, OVMSG, sizeof(OVMSG) - 1);
    ^
h_page.c:1025:5: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
     (void)write(STDERR_FILENO, OVMSG,
     ^
cc1: all warnings being treated as errors

I use different fixes.

For the first warning, I added real error handling.

For the other three warnings, I continue to ignore the return value,
but switch to fwrite(), whose return value can be ignored, because
we are writing an error message before returning a failure status.
It doesn't make sense to check for the failure of writing the error
message.

The traditional way of using a (void) cast to ignore function return
values no longer works in gcc 4.8. See

http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c

If this problem comes up again, we can try the more elegant solutions
in that stackoverflow page. (Some of the solutions are very ugly.)

Here are the man pages for fwrite() and write():
http://pubs.opengroup.org/onlinepubs/009695399/functions/fwrite.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

Comment 45

[Martin Thomson [:mt:]]

Comment on attachment 8652558 [details] [diff] [review]
Fix "ignoring return value of ‘write’" compiler warnings in lib/dbm/h_page.c

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

If the (void) casts aren't going to work, then perhaps we don't need them.

::: lib/dbm/src/h_page.c
@@ +157,5 @@
>   	   */
>   	  { 
>   	 	char buffer[1024];
>  	   	long len = seek_pos-end_pos;
> +	   	memset(buffer, 0, 1024);

Wow, that's a pretty bad bug.

Comment 46

[Wan-Teh Chang]

Comment on attachment 8652558 [details] [diff] [review]
Fix "ignoring return value of ‘write’" compiler warnings in lib/dbm/h_page.c

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

Patch checked in: https://hg.mozilla.org/projects/nss/rev/65f748a91a37

::: lib/dbm/src/h_page.c
@@ +157,5 @@
>   	   */
>   	  { 
>   	 	char buffer[1024];
>  	   	long len = seek_pos-end_pos;
> +	   	memset(buffer, 0, 1024);

I remember |&buffer| is the same as |buffer| if |buffer| is
declared as an array. But I prefer |buffer| or |&buffer[0]|.

@@ +981,5 @@
>  #define	OVMSG	"HASH: Out of overflow pages.  Increase page size\n"
>  	if (offset > SPLITMASK) {
>  		if (++splitnum >= NCACHED) {
>  #ifndef macintosh
> +			(void)fwrite(OVMSG, 1, sizeof(OVMSG) - 1, stderr);

The (void) casts are used extensively in this file, so I left
them alone.

Comment 47

[Wan-Teh Chang]

Attachment: Fix strict-aliasing violations in sslsnce.c

Building with gcc 4.8 and BUILD_OPT=1, I get these compiler warnings:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/sslsnce.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_ENABLE_ZLIB -I../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  sslsnce.c
sslsnce.c: In function ‘SSL_InheritMPServerSIDCacheInstance’:
sslsnce.c:1590:5: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
     *(ptrdiff_t *)(&cache->sidCacheLocks) += ptr;
     ^
sslsnce.c:1591:5: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
     *(ptrdiff_t *)(&cache->keyCacheLock ) += ptr;
     ^
...
sslsnce.c:1602:5: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
     *(ptrdiff_t *)(&cache->srvNameCacheData) += ptr;
     ^
cc1: all warnings being treated as errors

The code in question assumes ptrdiff_t is exactly the same size as a pointer,
which may be true but we can avoid that assumption.

In this patch I treat the current values in the pointer members as offsets
(they are) and add them to the base pointer my.cacheMem or cache->cacheMem.

Comment 48

[Wan-Teh Chang]

Attachment: Fix a "variable may be used uninitialized" warning in blapitest.c

Building with gcc 4.8 and BUILD_OPT=1, I get this compiler warning:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/blapitest.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_STATIC_LIBS -I../../nss/lib/softoken -I../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -I../../../dist/public/seccmd -I../../../dist/public/dbm -I../../../dist/public/softoken  blapitest.c
blapitest.c: In function ‘main’:
blapitest.c:3574:5: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  if (ret != 0) {
     ^
cc1: all warnings being treated as errors

|ret| is indeed not initialized if |rounds| is 0.

I fixed this by initializing |ret| to -1, which indicates failure (0 indicates
success). So if the test runs 0 rounds, we treat it as a failure. It also seems
reasonable to skip the test by setting rounds=0, but I don't know if we're doing
that.

Comment 49

[Wan-Teh Chang]

Attachment: Check the return value of fread() in certcgi.c

Building with gcc 4.8 and BUILD_OPT=1, I get this compiler warning:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/certcgi.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -DNSPR20 -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_STATIC_LIBS -I../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss  -I../../../dist/private/nss  -I../../../dist/public/seccmd -I../../../dist/public/dbm  certcgi.c
certcgi.c: In function ‘get_serial_number’:
certcgi.c:511:11: error: ignoring return value of ‘fread’, declared with attribute warn_unused_result [-Werror=unused-result]
      fread(&serial, sizeof(int), 1, serialFile);
           ^
cc1: all warnings being treated as errors

fread() returns the number of items read:

http://pubs.opengroup.org/onlinepubs/009695399/functions/fread.html

It seems that we don't need the ferror(serialFile) != 0 check, but I
didn't delete it.

Comment 50

[Martin Thomson [:mt:]]

Comment on attachment 8653002 [details] [diff] [review]
Fix strict-aliasing violations in sslsnce.c

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

This looks like a strict improvement.  I'm wondering though whether this structure could be unionized though.

Namely:

#define PTR_OR_OFFSET(ptrType) union { ptrType *ptr; size_t offset; }

struct cacheDescStr {
...
    PTR_OR_OFFSET(sidCacheLock) sidCacheLocks;
    PTR_OR_OFFSET(sidCacheLock) keyCacheLock;
    PTR_OR_OFFSET(sidCacheLock) certCacheLock;
... and so on...
    sidCacheLock    *          srvNameCacheLock;
    sidCacheSet     *          sidCacheSets;
    sidCacheEntry   *          sidCacheData;
    certCacheEntry  *          certCacheData;
    SSLWrappedSymWrappingKey * keyCacheData;
    PRUint8         *          ticketKeyNameSuffix;
    encKeyCacheEntry         * ticketEncKey;
    encKeyCacheEntry         * ticketMacKey;
    PTR_OR_OFFSET(PRUint32) ticketKeysValid;
    PTR_OR_OFFSET(srvNameCacheEntry) srvNameCacheData;
...
}

The main cost there is in changing all the code to use .ptr.  And then you need to change the places where it is mapped into memory in the way that you have.

I didn't do that because it seemed like more work than this stuff warranted.

Comment 51

[Wan-Teh Chang]

Attachment: Check the return value of fgets() in cmd/lib/secpwd.c

Building with gcc 4.8 and BUILD_OPT=1, I get this compiler warning:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/secpwd.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -DNSPR20 -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  secpwd.c
secpwd.c: In function ‘SEC_GetPassword’:
secpwd.c:77:14: error: ignoring return value of ‘fgets’, declared with attribute warn_unused_result [-Werror=unused-result]
  QUIET_FGETS ( phrase, sizeof(phrase), input);
              ^
cc1: all warnings being treated as errors

Here is the fgets() man page:
http://pubs.opengroup.org/onlinepubs/009695399/functions/fgets.html

Comment 52

[Wan-Teh Chang]

Attachment: Check the return value of fgets() in cmd/signtool/certgen.c

Building with gcc 4.8 and BUILD_OPT=1, I get these compiler warnings:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/certgen.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -I../../../dist/public/seccmd  certgen.c
certgen.c: In function ‘GetSubjectFromUser’:
certgen.c:125:10: error: ignoring return value of ‘fgets’, declared with attribute warn_unused_result [-Werror=unused-result]
     fgets(buf, STDIN_BUF_SIZE, stdin);
          ^
certgen.c:147:10: error: ignoring return value of ‘fgets’, declared with attribute warn_unused_result [-Werror=unused-result]
     fgets(buf, STDIN_BUF_SIZE, stdin);
          ^
...
certgen.c:241:10: error: ignoring return value of ‘fgets’, declared with attribute warn_unused_result [-Werror=unused-result]
     fgets(buf, STDIN_BUF_SIZE, stdin);
          ^
cc1: all warnings being treated as errors

I opted to handle errors. I wonder if we should just slap on the (void) casts.

Comment 53

[Wan-Teh Chang]

Comment on attachment 8653007 [details] [diff] [review]
Fix a "variable may be used uninitialized" warning in blapitest.c

https://hg.mozilla.org/projects/nss/rev/4a2025720da2

Comment 54

[Wan-Teh Chang]

Comment on attachment 8653011 [details] [diff] [review]
Check the return value of fread() in certcgi.c

https://hg.mozilla.org/projects/nss/rev/c1b4d7bb747f

Comment 55

[Wan-Teh Chang]

Comment on attachment 8653002 [details] [diff] [review]
Fix strict-aliasing violations in sslsnce.c

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

https://hg.mozilla.org/projects/nss/rev/9ca6d2b88206

Martin: I also considered using a union, but I believe this kind
of pointer arithmetic is legal in C. It is common in low-level
code.

I think it would be useful to create a function for the duplicate
code, and to macro-ize the pointer increment statements. The current
code is not easy to inspect.

Comment 56

[Martin Thomson [:mt:]]

Comment on attachment 8653033 [details] [diff] [review]
Check the return value of fgets() in cmd/signtool/certgen.c

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

Reporting the errors here seems sensible.  It probably won't increase the failure rate, but it might limit the damage if fgets actually breaks for some reason.

Comment 57

[Wan-Teh Chang]

Comment on attachment 8653029 [details] [diff] [review]
Check the return value of fgets() in cmd/lib/secpwd.c

https://hg.mozilla.org/projects/nss/rev/84325753da79

Comment 58

[Wan-Teh Chang]

Attachment: Check the return value of fgets() in cmd/signtool/certgen.c, v2

I found that the caller of GetSubjectFromUser() wasn't checking
the return value, so I added a check.

https://hg.mozilla.org/projects/nss/rev/c32fabbfb1d8

Comment 59

[Martin Thomson [:mt:]]

Attachment: decl.patch

Wan-Teh, I know that you want to use C99, but until then.

Comment 60

[Wan-Teh Chang]

Comment on attachment 8654207 [details] [diff] [review]
decl.patch

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

r=wtc. I added you as a reviewer for my larger patch in bug 1117022.

You can go ahead and check this in first to fix the compilation error.

Comment 61

[Martin Thomson [:mt:]]

Comment on attachment 8654207 [details] [diff] [review]
decl.patch

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

https://hg.mozilla.org/projects/nss/rev/96063e1e8226

Wan-Teh, you will need to rebase your patch to avoid collisions.

Comment 62

[Robert Relyea]

I think everything is checked in, If there is more feedback needed Martin, go ahead an reset the flags.

bob

Comment 64

[Martin Thomson [:mt:]]

This has been quiet for a while now.  Any new stuff can be addressed in new bugs.

Comment 65

[Wan-Teh Chang]

Martin: could you set the target milestone? (The NSS release in which
you completed this work.) Thanks!