868678 - Fix miscellaneous problems in the new SECItemArray code.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Rename |len| to |nitems|

The SECItemArray structure is defined as follows:

  typedef struct SECItemArrayStr SECItemArray;

  struct SECItemArrayStr {
      SECItem *items;
      unsigned int len;
  };

The name of the |len| field is confusing.

1. It could be misinterpreted to mean the length in bytes.

2. It could be confused with the |len| field of the SECItem structure.

I propose that we rename the |len| field of SECItemArray. This patch
renames |len| to |nitems|, but there are other possibilities:

  struct SECItemArrayStr {
      SECItem *items;
      unsigned int nitems;
  };

  struct SECItemArrayStr {
      SECItem *items;
      unsigned int count;
  };

  struct SECItemArrayStr {
      SECItem *items;
      unsigned int size;
  };

After writing this patch, I noticed a problem with |nitems|: it looks
like |items|.

Also, I wonder if renaming the |items| field to |elems| may result
in slightly more readable code. For example, compare

    /* Use the array's first item only (single stapling) */
    len = 1 + ss->certStatusArray->items[0].len + 3;

with

    /* Use the array's first item only (single stapling) */
    len = 1 + ss->certStatusArray->elems[0].len + 3;

Comment 1

[Wan-Teh Chang]

Comment on attachment 745439 [details] [diff] [review]
Rename |len| to |nitems|

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

Notes on some changes in my patch.

::: lib/ssl/ssl3ext.c
@@ +199,5 @@
>      PRUint32 *aes_key_length, const unsigned char **mac_key,
>      PRUint32 *mac_key_length)
>  {
>      if (PR_CallOnce(&generate_session_keys_once,
> +	    ssl3_GenerateSessionTicketKeys) != PR_SUCCESS)

This is an unrelated change: PR_CallOnce() returns PRStatus, so we
should use PR_SUCCESS here.

@@ -1740,5 @@
>  ssl3_ServerHandleStatusRequestXtn(sslSocket *ss, PRUint16 ex_type,
>  				  SECItem *data)
>  {
>      SECStatus rv = SECSuccess;
> -    PRUint32 len = 0;

This removes an unused variable.

::: lib/util/secitem.c
@@ -351,5 @@
>          }
> -        /*
> -         * If array is not NULL, the above has set array->data and
> -         * array->len to 0.
> -         */

This comment was copied from SECITEM_AllocItem, but the code here is
different: it doesn't set array->items and array->nitems to 0.

So I removed this comment, and add the code to set array->items and
array->nitems to 0.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745439 [details] [diff] [review]
Rename |len| to |nitems|

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

(In reply to Wan-Teh Chang from comment #0)
> The name of the |len| field is confusing.
> 
> 1. It could be misinterpreted to mean the length in bytes.

I would prefer to leave it as-is, since Kai and Bob both seemed to think the names were good, and there is some consistency with other things in NSS, and I also think the names are reasonable.

Compare:

struct CERTDERCertsStr {
    PLArenaPool *arena;
    int numcerts;
    SECItem *rawCerts;
};

...

struct CERTCertificateListStr {
    SECItem *certs;
    int len;					/* number of certs */
    PLArenaPool *arena;
};

...

struct CERTGeneralNameListStr {
    PLArenaPool *arena;
    CERTGeneralName *name;
    int refCount;
    int len;
    PZLock *lock;
};

I think "len" is a better name than nitems, for sure.

I also think "items" is a better name than "elems" (or at least not worse) considering the items are SECItems.

I can see why "len" could be confusing, with the example code you provided at the end of your summary. If you think a change is warranted, I would suggest "numItems" instead of any of the things you suggested, as being the most clear.

Please post an updated patch and r? me if you still think a change is warranted.

Comment 3

[Wan-Teh Chang]

Attachment: Make sure SECITEM_AllocArray sets array->items and array->len to 0 on failure

Brian: thank you for your opinions on the renaming patch. I'll
think more about it.

This patch fixes the SECITEM_AllocArray problem only. I assume we
want SECITEM_AllocArray to match what SECITEM_AllocItem does on
failure.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745446 [details] [diff] [review]
Make sure SECITEM_AllocArray sets array->items and array->len to 0 on failure

IMO, it is a bad idea to use PORT_Assert to check the precondition that array->items == NULL for this function, especially since it is public. It would be better to have check like:

if (array && array->items) {
   PORT_SetError(SEC_ERROR_INVALID_ARGS);
   return SECFailure;
}

This would make it clearer that we are never causing a memory leak by overwriting array->items. Note that SECITEM_AllocItem has the same problem.

Nit: IMO, at least in new functions like this, we should not be using "x != NULL" and "x == NULL" but instead "x" and "!x", according to the Mozilla style guide.

Please update the description of this bug to indicate the problem you are fixing.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #4)
> if (array && array->items) {
>    PORT_SetError(SEC_ERROR_INVALID_ARGS);
>    return SECFailure;
> }

er, "return NULL";

Comment 6

[Wan-Teh Chang]

Attachment: Make sure SECITEM_AllocArray sets array->items and array->len to 0 on failure, v2

Brian: I added the error checking you suggested to SECITEM_AllocArray.
I kept the assertion because I found a SECITEM_AllocArray call in
ssl3con.c that doesn't check the return value.

Comment 7

[Wan-Teh Chang]

Attachment: SECITEM_DupArray should allow |from| to be an empty SECItemArray

It should be OK for from->items to be NULL, as long as from->len is 0.
That represents an empty SECItemArray. SECITEM_DupArray should be able
to duplicate an empty array.

I also fixed some coding style nits.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745471 [details] [diff] [review]
Make sure SECITEM_AllocArray sets array->items and array->len to 0 on failure, v2

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

Note that SECItem_AllocItem has the same problem with the input parameter checking. It may be worth fixing in a new bug.g

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 745439 [details] [diff] [review]
Rename |len| to |nitems|

r=kaie because of the fixes you described in comment 1.

I don't mind renaming the len attribute.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 745471 [details] [diff] [review]
Make sure SECITEM_AllocArray sets array->items and array->len to 0 on failure, v2

r=kaie

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 745474 [details] [diff] [review]
SECITEM_DupArray should allow |from| to be an empty SECItemArray

> SECItemArray *
> SECITEM_DupArray(PLArenaPool *arena, const SECItemArray *from)
> {
>     SECItemArray *result;
>     unsigned int i;
> 
>-    if (!from || !from->items || !from->len)
>+    if (!from || (!from->items && from->len))


Without a comment, I find it difficult to understand that you explicitly allow an empty array.
I'd prefer a comment in front of this check, that explains the allowed scenarios.

/* Require a "from" array.
 * Reject an inconsistent "from" array with NULL data and nonzero length.
 * However, allow a "from" array of zero length.
 */

r=kaie

Comment 12

[Wan-Teh Chang]

Attachment: SECITEM_DupArray should allow |from| to be an empty SECItemArray, v2

Kai: thank you for the review. I added the comment you
suggested and checked this in.

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

Comment 13

[Wan-Teh Chang]

Comment on attachment 745439 [details] [diff] [review]
Rename |len| to |nitems|

I abandoned this patch. The fixes I described in comment 1
have been checked in separately.