Bugs discovered by using the anti-c lint engine.

RESOLVED FIXED in 3.6

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: John Carter, Assigned: Bishakha Banerjee)

Tracking

unspecified
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

16 years ago
This is a list of bugs found by antic (http://artho.com/jlint/)

The vast majority of false positives have been removed leaving only
definite bugs and highly suspicious cases.

======================================================================
Case 5 will be overwritten. 
mozilla/security/nss/cmd/crmftest/testcrmf.c:1270:5: Possible miss of BREAK
before CASE/DEFAULT

    switch (dbVersion) {
    case 7:
       dbver = "7";
       break;
    case 6:
       dbver = "6";
       break;
    case 5:
        dbver = "5";
    case 4:
    default:
        dbver = "";
	break;
======================================================================
In case crmfSubsequentMessage, rv will get overridden with SECFailure

mozilla/security/nss/lib/crmf/crmfpop.c:597:5: Possible miss of BREAK before
CASE/DEFAULT

    case crmfSubsequentMessage:
        rv = crmf_add_privkey_subseqmessage(inCertReqMsg, subseqMess, 
					    crmfKeyAgreement);
    case crmfDHMAC:
        /* This case should be added in the future. */
    default:
        rv = SECFailure;
    }
======================================================================
In case SEC_OID_X942_DIFFIE_HELMAN_KEY: keyType gets overridden by "nullKey"

mozilla/security/nss/lib/cryptohi/seckey.c:883:7: Possible miss of BREAK before
CASE/DEFAULT


      case SEC_OID_X942_DIFFIE_HELMAN_KEY:
	keyType = dhKey;
      default:
======================================================================
Somebody doesn't like Diffie Helman....
mozilla/security/nss/lib/pk11wrap/pk11cert.c:1614:5: Possible miss of BREAK
before CASE/DEFAULT

    case dhKey:
        newItem = SECITEM_DupItem(&pubk->u.dh.publicValue);
    case fortezzaKey:
    default:
	newItem = NULL; /* Fortezza Fix later... */
======================================================================
This switch is too hairy for me to tell if this really is a bug, but
it looks suspicious...
mozilla/security/nss/lib/pk11wrap/pk11pbe.c:536:5: Possible miss of BREAK before
CASE/DEFAULT
    case pbeBitGenCipherIV:
	if (bitsNeeded > 64) {
	    break;
	}
	if (hashAlgorithm != SEC_OID_SHA1) {
	    break;
	}
	mechanism = CKM_PBE_SHA1_DES3_EDE_CBC;
    case pbeBitGenCipherKey:

======================================================================
This code is never reached.....

mozilla/security/nss/lib/pkix/src/AttributeTypeAndValue/PCreateFromUTF8.c:113:3:
Unreachable statement

  if( (nssArenaMark *)NULL != mark ) {
    if( PR_SUCCESS != nssArena_Unmark(arena, mark) ) {
      goto loser;
    }
  }
======================================================================
And again...
mozilla/security/nss/lib/pkix/src/Name/PCreateFromUTF8.c:115:3: Unreachable
statement
And again....
mozilla/security/nss/lib/pkix/src/RelativeDistinguishedName/PCreateFromUTF8.c:115:3:
Unreachable statement

  if( (nssArenaMark *)NULL != mark ) {
    if( PR_SUCCESS != nssArena_Unmark(arena, mark) ) {
      goto loser;
    }
  }
======================================================================
Classic missing "break"s....
mozilla/security/nss/lib/smime/smimeutil.c:249:9: Possible miss of BREAK before
CASE/DEFAULT
    switch (algtag) {
    case SEC_OID_RC2_CBC:
	keylen_bits = PK11_GetKeyStrength(key, algid);
	switch (keylen_bits) {
	case 40:
	    c = SMIME_RC2_CBC_40;
	case 64:
	    c = SMIME_RC2_CBC_64;
	case 128:
	    c = SMIME_RC2_CBC_128;
	default:
	    rv = SECFailure;
	    break;
	}
	break;
    case SEC_OID_DES_CBC:
	c = SMIME_DES_CBC_56;
    case SEC_OID_DES_EDE3_CBC:
	c = SMIME_DES_EDE3_168;
    case SEC_OID_FORTEZZA_SKIPJACK:
	c = SMIME_FORTEZZA;
    default:
	rv = SECFailure;
    }
======================================================================
Possible missing break? Looks suspicious....
mozilla/security/nss/lib/softoken/pkcs11.c:984:5: Possible miss of BREAK before
CASE/DEFAULT

    case CKK_DSA:
	if ( !pk11_hasAttribute(object, CKA_SUBPRIME)) {
	    return CKR_TEMPLATE_INCOMPLETE;
	}
    case CKK_DH:
======================================================================
And again.... I'm not sure about this one...
mozilla/security/nss/lib/softoken/pkcs11.c:1117:5: Possible miss of BREAK before
CASE/DEFAULT

    case CKK_DSA:
	if ( !pk11_hasAttribute(object, CKA_SUBPRIME)) {
	    return CKR_TEMPLATE_INCOMPLETE;
	}
	if ( !pk11_hasAttribute(object, CKA_NETSCAPE_DB)) {
	    return CKR_TEMPLATE_INCOMPLETE;
	}
    case CKK_DH:
======================================================================
This one looks bad...
mozilla/security/nss/lib/softoken/pkcs11c.c:2317:5: Possible miss of BREAK
before CASE/DEFAULT
    case CKM_TLS_PRF_GENERAL:
	crv = pk11_TLSPRFInit(context, key, key_type);

    default:
	crv = CKR_MECHANISM_INVALID;
	break;

======================================================================
Possible bug, perhaps not....
mozilla/security/nss/lib/ssl/sslsock.c:743:7: Possible miss of BREAK before
CASE/DEFAULT

      case SSL_ENABLE_FDX:
      	ssl_defaults.fdx = on;

======================================================================
Also occurs in nsprpub
If USE_REENTRANT_LIBC is defined, fprint of error will occur whether
error or not. Solution: Put braces round it.


mozilla/security/coreconf/nsinstall/pathsub.c:76:9: May be wrong assumption
about IF body

    if (error)

#ifdef USE_REENTRANT_LIBC
    R_STRERROR_R(errno);
	fprintf(stderr, ": %s", r_strerror_r);
#else
	fprintf(stderr, ": %s", strerror(errno));
#endif
======================================================================

Comment 1

16 years ago
Assigned the bug to Bishakha.  These warnings need to be reviewed
carefully.  They are not necessarily bugs.
Assignee: wtc → bishakhabanerjee
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → 3.4.1

Comment 2

16 years ago
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee

Comment 3

16 years ago
Set target milestone to NSS 3.5.
Target Milestone: 3.4.1 → 3.5

Comment 4

16 years ago
Created attachment 92126 [details] [diff] [review]
Applies some of the suggestions

Just mindlessly turning some of the suggestions into patches. I left anything
that was "unsure" out of the patch.  Not that everything in the patch is
necessarily correct, but it gives something to hold discussions over =)
Review comments on the foregoing patch:

Each of these changes appears correct, as far as they go.  However, one of 
them, the change to smimeutil.c, only fixes 3 of the 6 shown switch cases 
that are missing breaks.

Comments on the items not addressed by this patch:

The missing break in pk11pbe, just prior to case pbeBitGenCipherKey
appears to be a bug.

The missing break in sslsock.c is a bug.  The case is probably unused.

Both of the two places shown in pkcs11.c where case CKK_DSA falls 
through into case CKK_DH are correct and intentional.  Some comment like 
/* fall through */ should be added to make this apparent

Re: the unreachable code shown in two copies of PCreateFromUTF8.c,
This code looks unfinished (incomplete).  Is it even being built?
I'd say we shouldn't attempt to delint this code until it's finished.

Comment 6

16 years ago
Created attachment 92753 [details] [diff] [review]
Addresses comment 5 concerns
Attachment #92126 - Attachment is obsolete: true
r=nelsonb for patch id 92753.  

Thanks for developing this patch, and for filtering out the nuisance lint 
warnings!  IMO, this patch shows good judgement in choosing which lint 
complaints to fix.
(Assignee)

Comment 8

16 years ago
Nelson,

checked in riceman+bmo@mail.rit.edu's patch as requested to tip.

/cvsroot/mozilla/security/coreconf/nsinstall/pathsub.c,v  <--  pathsub.c
new revision: 1.4; previous revision: 1.3
done

/cvsroot/mozilla/security/nss/cmd/crmftest/testcrmf.c,v  <--  testcrmf.c
new revision: 1.3; previous revision: 1.2
done

/cvsroot/mozilla/security/nss/lib/crmf/crmfpop.c,v  <--  crmfpop.c
new revision: 1.2; previous revision: 1.1
done

/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.19; previous revision: 1.18
done

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.91; previous revision: 1.90
done

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.6; previous revision: 1.5
done

/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.9; previous revision: 1.8
done

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.55; previous revision: 1.54
done

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.34; previous revision: 1.33
done

/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.29; previous revision: 1.28
done

Comment 9

15 years ago
Fixed in NSS 3.6.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Target Milestone: 3.5 → 3.6
You need to log in before you can comment on or make changes to this bug.