Bugzilla bug 227296: fixed the bug that NSS_CMSAttribute_AddValue adds the

address of a stack variable to the attr->values array.  Added a new
function SECITEM_ArenaDupItem.  r=nelsonb.
Modified Files:
	nss/nss.def util/secitem.c util/secitem.h smime/cmsarray.c
	smime/cmsattr.c

5 files changed, 42 insertions, 12 deletions

diff --git a/security/nss/lib/nss/nss.def b/security/nss/lib/nss/nss.def
index 9b002400c..b88fd3cdb 100644
--- a/security/nss/lib/nss/nss.def
+++ b/security/nss/lib/nss/nss.def
@@ -774,6 +774,7 @@ PK11_MoveKey;
 PK11_PubDeriveExtended;
 PK11_PubUnwrapSymKeyWithFlagsPerm;
 PK11_UnwrapSymKeyWithFlagsPerm;
+SECITEM_ArenaDupItem;
 SECKEY_ECParams2KeySize;
 SECMOD_GetDBModuleList;
 SECMOD_GetDeadModuleList;
diff --git a/security/nss/lib/smime/cmsarray.c b/security/nss/lib/smime/cmsarray.c
index 68d7e1963..2673770d9 100644
--- a/security/nss/lib/smime/cmsarray.c
+++ b/security/nss/lib/smime/cmsarray.c
@@ -91,6 +91,10 @@ NSS_CMSArray_Add(PRArenaPool *poolp, void ***array, void *obj)
 			      (n + 1) * sizeof(void *),
 			      (n + 2) * sizeof(void *));
     }
+
+    if (dest == NULL)
+	return SECFailure;
+
     dest[n] = obj;
     dest[n+1] = NULL;
     *array = dest;
diff --git a/security/nss/lib/smime/cmsattr.c b/security/nss/lib/smime/cmsattr.c
index 743964982..250670745 100644
--- a/security/nss/lib/smime/cmsattr.c
+++ b/security/nss/lib/smime/cmsattr.c
@@ -86,13 +86,11 @@ NSS_CMSAttribute_Create(PRArenaPool *poolp, SECOidTag oidtag, SECItem *value, PR
 	goto loser;
 
     if (value != NULL) {
-	if ((copiedvalue = SECITEM_AllocItem(poolp, NULL, value->len)) == NULL)
+	if ((copiedvalue = SECITEM_ArenaDupItem(poolp, value)) == NULL)
 	    goto loser;
 
-	if (SECITEM_CopyItem(poolp, copiedvalue, value) != SECSuccess)
+	if (NSS_CMSArray_Add(poolp, (void ***)&(attr->values), (void *)copiedvalue) != SECSuccess)
 	    goto loser;
-
-	NSS_CMSArray_Add(poolp, (void ***)&(attr->values), (void *)copiedvalue);
     }
 
     attr->encoded = encoded;
@@ -113,18 +111,22 @@ loser:
 SECStatus
 NSS_CMSAttribute_AddValue(PLArenaPool *poolp, NSSCMSAttribute *attr, SECItem *value)
 {
-    SECItem copiedvalue;
+    SECItem *copiedvalue;
     void *mark;
 
     PORT_Assert (poolp != NULL);
 
     mark = PORT_ArenaMark(poolp);
 
-    /* XXX we need an object memory model #$%#$%! */
-    if (SECITEM_CopyItem(poolp, &copiedvalue, value) != SECSuccess)
+    if (value == NULL) {
+	PORT_SetError(SEC_ERROR_INVALID_ARGS);
+	goto loser;
+    }
+
+    if ((copiedvalue = SECITEM_ArenaDupItem(poolp, value)) == NULL)
 	goto loser;
 
-    if (NSS_CMSArray_Add(poolp, (void ***)&(attr->values), (void *)&copiedvalue) != SECSuccess)
+    if (NSS_CMSArray_Add(poolp, (void ***)&(attr->values), (void *)copiedvalue) != SECSuccess)
 	goto loser;
 
     PORT_ArenaUnmark(poolp, mark);
@@ -221,7 +223,7 @@ cms_attr_choose_attr_value_template(void *src_or_dest, PRBool encoding)
         attribute->encoded)) {
         /* we're encoding, and the attribute has no value or the attribute
          * value is already encoded. */
-       return SEC_ASN1_GET(SEC_AnyTemplate);
+        return SEC_ASN1_GET(SEC_AnyTemplate);
     }
 
     /* get attribute's typeTag */
diff --git a/security/nss/lib/util/secitem.c b/security/nss/lib/util/secitem.c
index eb4683ca4..daf550d62 100644
--- a/security/nss/lib/util/secitem.c
+++ b/security/nss/lib/util/secitem.c
@@ -180,18 +180,32 @@ SECITEM_ItemsAreEqual(const SECItem *a, const SECItem *b)
 SECItem *
 SECITEM_DupItem(const SECItem *from)
 {
+    return SECITEM_ArenaDupItem(NULL, from);
+}
+
+SECItem *
+SECITEM_ArenaDupItem(PRArenaPool *arena, const SECItem *from)
+{
     SECItem *to;
     
     if ( from == NULL ) {
 	return(NULL);
     }
     
-    to = (SECItem *)PORT_Alloc(sizeof(SECItem));
+    if ( arena != NULL ) {
+	to = (SECItem *)PORT_ArenaAlloc(arena, sizeof(SECItem));
+    } else {
+	to = (SECItem *)PORT_Alloc(sizeof(SECItem));
+    }
     if ( to == NULL ) {
 	return(NULL);
     }
 
-    to->data = (unsigned char *)PORT_Alloc(from->len);
+    if ( arena != NULL ) {
+	to->data = (unsigned char *)PORT_ArenaAlloc(arena, from->len);
+    } else {
+	to->data = (unsigned char *)PORT_Alloc(from->len);
+    }
     if ( to->data == NULL ) {
 	PORT_Free(to);
 	return(NULL);
@@ -199,7 +213,9 @@ SECITEM_DupItem(const SECItem *from)
 
     to->len = from->len;
     to->type = from->type;
-    PORT_Memcpy(to->data, from->data, to->len);
+    if ( to->len ) {
+	PORT_Memcpy(to->data, from->data, to->len);
+    }
     
     return(to);
 }
diff --git a/security/nss/lib/util/secitem.h b/security/nss/lib/util/secitem.h
index 76a5d16fb..d957ba0eb 100644
--- a/security/nss/lib/util/secitem.h
+++ b/security/nss/lib/util/secitem.h
@@ -93,6 +93,13 @@ extern SECStatus SECITEM_CopyItem(PRArenaPool *arena, SECItem *to,
 extern SECItem *SECITEM_DupItem(const SECItem *from);
 
 /*
+** Allocate an item and copy "from" into it.  The item itself and the 
+** data it points to are both allocated from the arena.  If arena is
+** NULL, this function is equivalent to SECITEM_DupItem.
+*/
+extern SECItem *SECITEM_ArenaDupItem(PRArenaPool *arena, const SECItem *from);
+
+/*
 ** Free "zap". If freeit is PR_TRUE then "zap" itself is freed.
 */
 extern void SECITEM_FreeItem(SECItem *zap, PRBool freeit);