If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fix miscellaneous problems in the new SECItemArray code.

RESOLVED FIXED in 3.15

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 745439 [details] [diff] [review]
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;
Attachment #745439 - Flags: superreview?(kaie)
Attachment #745439 - Flags: review?(bsmith)
(Assignee)

Comment 1

5 years ago
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 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.
Attachment #745439 - Flags: review?(bsmith)
(Assignee)

Comment 3

5 years ago
Created attachment 745446 [details] [diff] [review]
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.
Attachment #745446 - Flags: superreview?(kaie)
Attachment #745446 - Flags: review?(bsmith)
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.
Attachment #745446 - Flags: review?(bsmith) → review+
(In reply to Brian Smith (:bsmith) from comment #4)
> if (array && array->items) {
>    PORT_SetError(SEC_ERROR_INVALID_ARGS);
>    return SECFailure;
> }

er, "return NULL";
(Assignee)

Comment 6

5 years ago
Created attachment 745471 [details] [diff] [review]
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.
Attachment #745446 - Attachment is obsolete: true
Attachment #745446 - Flags: superreview?(kaie)
Attachment #745471 - Flags: superreview?(kaie)
Attachment #745471 - Flags: review?(bsmith)
(Assignee)

Comment 7

5 years ago
Created attachment 745474 [details] [diff] [review]
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.
Attachment #745474 - Flags: review?(kaie)
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
Attachment #745471 - Flags: review?(bsmith) → review+
(Assignee)

Updated

5 years ago
Attachment #745471 - Flags: checked-in+

Comment 9

4 years ago
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.
Attachment #745439 - Flags: superreview?(kaie) → review+

Comment 10

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

r=kaie
Attachment #745471 - Flags: superreview?(kaie) → superreview+

Comment 11

4 years ago
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
Attachment #745474 - Flags: review?(kaie) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 751954 [details] [diff] [review]
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
Attachment #745474 - Attachment is obsolete: true
Attachment #751954 - Flags: review?(kaie)
Attachment #751954 - Flags: checked-in+
(Assignee)

Comment 13

4 years ago
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.
Attachment #745439 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Summary: Rename the |len| field of SECItemArray → Fix miscellaneous problems in the new SECItemArray code.

Updated

4 years ago
Attachment #751954 - Flags: review?(kaie) → review+
You need to log in before you can comment on or make changes to this bug.