Blob Blame History Raw
From c4bd40a483c7962beb4a8b1c7f84d647de66bf0c Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Tue, 29 Aug 2017 13:08:11 +0200
Subject: [PATCH 1/5] util: fix the return value type of ReadDERFromFile()

SECStatus is an inappropriate type for returning the count of objects.

Upstream-commit: 01c13264e59050ea34aefc3d2a38e9ead247276c
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 src/ckpem.h   |  6 ++----
 src/pinst.c   | 15 +++++----------
 src/pobject.c | 10 ++++------
 src/util.c    |  7 +++----
 4 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/src/ckpem.h b/src/ckpem.h
index e6ecc5f..be9977a 100644
--- a/src/ckpem.h
+++ b/src/ckpem.h
@@ -259,10 +259,8 @@ pem_CreateMDObject
 #define NSS_PEM_ARRAY_SIZE(x) ((sizeof (x))/(sizeof ((x)[0])))
 
 /* Read DER encoded data from a PEM file or a binary (der-encoded) file. */
-/* NOTE: Discrepancy with the the way callers use of the return value as a count
- * Fix this when we sync. up with the cleanup work being done at nss-pem project.
- */
-SECStatus ReadDERFromFile(SECItem ***derlist, char *filename, PRBool ascii, int *cipher, char **ivstring, PRBool certsonly);
+int ReadDERFromFile(SECItem ***derlist, char *filename, PRBool ascii,
+                    int *cipher, char **ivstring, PRBool certsonly);
 
 /* Fetch an attribute of the specified type. */
 const NSSItem * pem_FetchAttribute ( pemInternalObject *io, CK_ATTRIBUTE_TYPE type, CK_RV *pError);
diff --git a/src/pinst.c b/src/pinst.c
index 25485b1..a5901f0 100644
--- a/src/pinst.c
+++ b/src/pinst.c
@@ -475,14 +475,12 @@ AddCertificate(char *certfile, char *keyfile, PRBool cacert,
     pemInternalObject *o = NULL;
     CK_RV error = 0;
     int objid, i = 0;
-    int nobjs = 0;
     SECItem **objs = NULL;
     char *ivstring = NULL;
     int cipher;
 
-    /* TODO: Fix discrepancy between our usage of the return value as
-     * as an int (a count) and the declaration as a SECStatus. */
-    nobjs = (int) ReadDERFromFile(&objs, certfile, PR_TRUE, &cipher, &ivstring, PR_TRUE /* certs only */);
+    int nobjs = ReadDERFromFile(&objs, certfile, /* ascii */ PR_TRUE, &cipher,
+                                &ivstring, /* certs only */ PR_TRUE);
     if (nobjs <= 0) {
         NSS_ZFreeIf(objs);
         return CKR_GENERAL_ERROR;
@@ -517,12 +515,9 @@ AddCertificate(char *certfile, char *keyfile, PRBool cacert,
 
         if (o != NULL && keyfile != NULL) { /* add the private key */
             SECItem **keyobjs = NULL;
-            int kobjs = 0;
-            /* TODO: Fix discrepancy between our usage of the return value as
-             * as an int and the declaration as a SECStatus. */
-            kobjs =
-                (int) ReadDERFromFile(&keyobjs, keyfile, PR_TRUE, &cipher,
-                                &ivstring, PR_FALSE);
+            int kobjs = ReadDERFromFile(&keyobjs, keyfile, /* ascii */ PR_TRUE,
+                                        &cipher, &ivstring,
+                                        /*certs only */ PR_FALSE);
             if (kobjs < 1) {
                 found_error = PR_TRUE;
             } else {
diff --git a/src/pobject.c b/src/pobject.c
index 918b327..d80092a 100644
--- a/src/pobject.c
+++ b/src/pobject.c
@@ -1165,11 +1165,8 @@ pem_CreateObject
     }
 
     if (objClass == CKO_CERTIFICATE) {
-        /* TODO: Fix discrepancy between our usage of the return value as
-         * as an int and the declaration as a SECStatus. Typecasting as a
-         * temporary workaround.
-         */
-        nobjs = (int) ReadDERFromFile(&derlist, filename, PR_TRUE, &cipher, &ivstring, PR_TRUE /* certs only */);
+        nobjs = ReadDERFromFile(&derlist, filename, /* ascii */ PR_TRUE,
+                                &cipher, &ivstring, /* certs only */ PR_TRUE);
         if (nobjs < 1)
             goto loser;
 
@@ -1213,7 +1210,8 @@ pem_CreateObject
         SECItem certDER;
         PRBool added;
 
-        nobjs = ReadDERFromFile(&derlist, filename, PR_TRUE, &cipher, &ivstring, PR_FALSE /* keys only */);
+        nobjs = ReadDERFromFile(&derlist, filename, /* ascii */ PR_TRUE,
+                                &cipher, &ivstring, /* keys only */ PR_FALSE);
         if (nobjs < 1)
             goto loser;
 
diff --git a/src/util.c b/src/util.c
index ee9e0a4..9bad613 100644
--- a/src/util.c
+++ b/src/util.c
@@ -144,10 +144,9 @@ static SECStatus ConvertAsciiToZAllocItem(SECItem *der, const char *ascii)
     return rv;
 }
 
-/* FIX: Returns a SECStatus yet callers take result as a count */
-SECStatus
-ReadDERFromFile(SECItem *** derlist, char *filename, PRBool ascii,
-		int *cipher, char **ivstring, PRBool certsonly)
+/* returns count of objects read, or -1 on error */
+int ReadDERFromFile(SECItem *** derlist, char *filename, PRBool ascii,
+		    int *cipher, char **ivstring, PRBool certsonly)
 {
     SECStatus rv;
     PRFileDesc *inFile;
-- 
2.17.2


From 729f16122f0bae216583323389aa6ef1418e4c37 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Thu, 20 Dec 2018 18:42:33 +0100
Subject: [PATCH 2/5] ckpem: make pem_objs a list instead of array

Since libcurl started to use PK11_CreateManagedGenericObject(),
the calls to PK11_DestroyGenericObject() started to take affect
at the level of nss-pem.  Internal objects were removed on each
call of curl_easy_cleanup() and allocated again on each call of
curl_easy_perform().  This would be fine if the corresponding items
of the global pem_objects array were reused, which was, however,
not the case.  Consequently, the array grew unboundedly while most
of its items were NULL pointers.  This caused severe performance
regression in libcurl-based applications.

This commit replaces the global array by a global linked list,
which allows to delete arbitrary items instead of keeping NULL
pointers.  For compatibility reasons, the original array indexes
are stored in the list nodes.

Bug: https://bugzilla.redhat.com/1659108

Fixes #2

Upstream-commit: 8fc25a6b062a34d09473d4f80058fffda37f41a2
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 src/ckpem.h    |  11 ++-
 src/list.h     | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/pfind.c    |  19 ++---
 src/pinst.c    |  56 +++++++-------
 src/pobject.c  |  27 +++----
 src/psession.c |  11 +--
 6 files changed, 258 insertions(+), 66 deletions(-)
 create mode 100644 src/list.h

diff --git a/src/ckpem.h b/src/ckpem.h
index be9977a..caad74b 100644
--- a/src/ckpem.h
+++ b/src/ckpem.h
@@ -39,6 +39,8 @@
 #ifndef CKPEM_H
 #define CKPEM_H
 
+#include "list.h"
+
 #define USE_UTIL_DIRECTLY
 #include <utilrename.h>
 
@@ -197,9 +199,14 @@ struct pemInternalObjectStr {
   char            *nickname;
   NSSCKMDObject   mdObject;
   CK_SLOT_ID      slotID;
-  CK_ULONG        gobjIndex;
   int             refCount;
 
+  /* all internal objects are linked in a global list */
+  struct list_head gl_list;
+
+  /* we represent sparse array as list but keep its elements indexed */
+  long            arrayIdx;
+
   /* used by pem_mdFindObjects_Next */
   CK_BBOOL        extRef;
 
@@ -208,7 +215,7 @@ struct pemInternalObjectStr {
   pemObjectListItem *list;
 };
 
-NSS_EXTERN_DATA pemInternalObject **pem_objs;
+NSS_EXTERN_DATA struct list_head pem_objs;
 NSS_EXTERN_DATA int pem_nobjs;
 NSS_EXTERN_DATA int token_needsLogin[];
 NSS_EXTERN_DATA NSSCKMDSlot *lastEventSlot;
diff --git a/src/list.h b/src/list.h
new file mode 100644
index 0000000..7b4aec4
--- /dev/null
+++ b/src/list.h
@@ -0,0 +1,200 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_LIST_H
+#define _LINUX_LIST_H
+
+/* simplified for our use case */
+#define READ_ONCE(x) (x)
+#define WRITE_ONCE(dst, src) (dst = (src))
+#define typeof(x) __typeof__(x)
+#define POISON_POINTER_DELTA 0
+
+/* following code is taken from <linux/list.h> of v4.20-rc7-202-g1d51b4b1d3f2 */
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr:    the pointer to the member.
+ * @type:   the type of the container struct this is embedded in.
+ * @member: the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({              \
+    void *__mptr = (void *)(ptr);                   \
+    ((type *)(__mptr - offsetof(type, member))); })
+
+/*
+ * These are non-NULL pointers that will result in page faults
+ * under normal circumstances, used to verify that nobody uses
+ * non-initialized list entries.
+ */
+#define LIST_POISON1  ((void *) 0x100 + POISON_POINTER_DELTA)
+#define LIST_POISON2  ((void *) 0x200 + POISON_POINTER_DELTA)
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+/*
+ * Simple doubly linked list implementation.
+ *
+ * Some of the internal functions ("__xxx") are useful when
+ * manipulating whole lists rather than single entries, as
+ * sometimes we already know the next/prev entries and we can
+ * generate better code by using them directly rather than
+ * using the generic single-entry routines.
+ */
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+	struct list_head name = LIST_HEAD_INIT(name)
+
+static inline void INIT_LIST_HEAD(struct list_head *list)
+{
+	WRITE_ONCE(list->next, list);
+	list->prev = list;
+}
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_add(struct list_head *new,
+			      struct list_head *prev,
+			      struct list_head *next)
+{
+	next->prev = new;
+	new->next = next;
+	new->prev = prev;
+	WRITE_ONCE(prev->next, new);
+}
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+static inline void list_add(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head, head->next);
+}
+
+
+/**
+ * list_add_tail - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it before
+ *
+ * Insert a new entry before the specified head.
+ * This is useful for implementing queues.
+ */
+static inline void list_add_tail(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head->prev, head);
+}
+
+/*
+ * Delete a list entry by making the prev/next entries
+ * point to each other.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_del(struct list_head * prev, struct list_head * next)
+{
+	next->prev = prev;
+	WRITE_ONCE(prev->next, next);
+}
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty() on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+static inline void __list_del_entry(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+}
+
+static inline void list_del(struct list_head *entry)
+{
+	__list_del_entry(entry);
+	entry->next = LIST_POISON1;
+	entry->prev = LIST_POISON2;
+}
+
+/**
+ * list_entry - get the struct for this entry
+ * @ptr:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_entry(ptr, type, member) \
+	container_of(ptr, type, member)
+
+/**
+ * list_first_entry - get the first element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_first_entry(ptr, type, member) \
+	list_entry((ptr)->next, type, member)
+
+/**
+ * list_last_entry - get the last element from a list
+ * @ptr:    the list head to take the element from.
+ * @type:   the type of the struct this is embedded in.
+ * @member: the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_last_entry(ptr, type, member) \
+    list_entry((ptr)->prev, type, member)
+
+/**
+ * list_next_entry - get the next element in list
+ * @pos:	the type * to cursor
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_next_entry(pos, member) \
+	list_entry((pos)->member.next, typeof(*(pos)), member)
+
+/**
+ * list_prev_entry - get the prev element in list
+ * @pos:    the type * to cursor
+ * @member: the name of the list_head within the struct.
+ */
+#define list_prev_entry(pos, member) \
+    list_entry((pos)->member.prev, typeof(*(pos)), member)
+
+/**
+ * list_for_each_entry	-	iterate over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry(pos, head, member)				\
+	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	     &pos->member != (head);					\
+	     pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_reverse - iterate backwards over list of given type.
+ * @pos:    the type * to use as a loop cursor.
+ * @head:   the head for your list.
+ * @member: the name of the list_head within the struct.
+ */
+#define list_for_each_entry_reverse(pos, head, member)          \
+    for (pos = list_last_entry(head, typeof(*pos), member);     \
+         &pos->member != (head);                    \
+         pos = list_prev_entry(pos, member))
+
+#endif
diff --git a/src/pfind.c b/src/pfind.c
index 2aff8ff..3f0fdcb 100644
--- a/src/pfind.c
+++ b/src/pfind.c
@@ -253,12 +253,13 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate,
     size_t result_array_capacity = 0;
     pemObjectType type = pemRaw;
     CK_OBJECT_CLASS objClass = pem_GetObjectClass(pTemplate, ulAttributeCount);
+    pemInternalObject *obj = NULL;
 
     *pError = CKR_OK;
 
     plog("collect_objects slot #%ld, ", slotID);
     plog("%d attributes, ", ulAttributeCount);
-    plog("%d objects to look through.\n", pem_nobjs);
+    plog("%d objects created in total.\n", pem_nobjs);
     plog("Looking for: ");
     /*
      * now determine type of the object
@@ -299,22 +300,18 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate,
     }
 
     /* find objects */
-    for (i = 0; i < pem_nobjs; i++) {
+    list_for_each_entry(obj, &pem_objs, gl_list) {
         int match = 1; /* matches type if type not specified */
-        if (NULL == pem_objs[i])
-            continue;
-
-        plog("  %d type = %d\n", i, pem_objs[i]->type);
+        plog("  %d type = %d\n", i, obj->type);
         if (type != pemAll) {
             /* type specified - must match given type */
-            match = (type == pem_objs[i]->type);
+            match = (type == obj->type);
         }
         if (match) {
-            match = (slotID == pem_objs[i]->slotID) &&
-                (CK_TRUE == pem_match(pTemplate, ulAttributeCount, pem_objs[i]));
+            match = (slotID == obj->slotID) &&
+                (CK_TRUE == pem_match(pTemplate, ulAttributeCount, obj));
         }
         if (match) {
-            pemInternalObject *o = pem_objs[i];
             pemInternalObject **new_result_array = (pemInternalObject **)
                 ensure_array_capacity(*result_array,
                                       &result_array_capacity,
@@ -328,7 +325,7 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate,
             if (*result_array != new_result_array) {
                 *result_array = new_result_array;
             }
-            (*result_array)[ result_array_entries ] = o;
+            (*result_array)[ result_array_entries ] = obj;
             result_array_entries++;
         }
     }
diff --git a/src/pinst.c b/src/pinst.c
index a5901f0..810db5a 100644
--- a/src/pinst.c
+++ b/src/pinst.c
@@ -50,7 +50,7 @@
 
 static PRBool pemInitialized = PR_FALSE;
 
-pemInternalObject **pem_objs;
+LIST_HEAD(pem_objs);
 int pem_nobjs = 0;
 int token_needsLogin[NUM_SLOTS];
 NSSCKMDSlot *lastEventSlot;
@@ -362,13 +362,9 @@ derEncodingsMatch(CK_OBJECT_CLASS objClass, pemInternalObject * obj,
 static CK_RV
 LinkSharedKeyObject(int oldKeyIdx, int newKeyIdx)
 {
-    int i;
-    for (i = 0; i < pem_nobjs; i++) {
+    pemInternalObject *obj;
+    list_for_each_entry(obj, &pem_objs, gl_list) {
         CK_RV rv;
-        pemInternalObject *obj = pem_objs[i];
-        if (NULL == obj)
-            continue;
-
         if (atoi(obj->id.data) != oldKeyIdx)
             continue;
 
@@ -381,13 +377,26 @@ LinkSharedKeyObject(int oldKeyIdx, int newKeyIdx)
     return CKR_OK;
 }
 
+/* return pointer to the internal object with given arrayIdx */
+static pemInternalObject *
+FindObjectByArrayIdx(const long arrayIdx)
+{
+    pemInternalObject *obj;
+
+    list_for_each_entry(obj, &pem_objs, gl_list)
+        if (arrayIdx == obj->arrayIdx)
+            return obj;
+
+    return NULL;
+}
+
 pemInternalObject *
 AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
                   pemObjectType type, SECItem * certDER,
                   SECItem * keyDER, const char *filename,
                   int objid, CK_SLOT_ID slotID, PRBool *pAdded)
 {
-    int i;
+    pemInternalObject *curObj;
 
     const char *nickname = strrchr(filename, '/');
     if (nickname
@@ -401,11 +410,7 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
         *pAdded = PR_FALSE;
 
     /* first look for the object in pem_objs, it might be already there */
-    for (i = 0; i < pem_nobjs; i++) {
-        pemInternalObject *const curObj = pem_objs[i];
-        if (NULL == curObj)
-            continue;
-
+    list_for_each_entry(curObj, &pem_objs, gl_list) {
         /* Comparing DER encodings is dependable and frees the PEM module
          * from having to require clients to provide unique nicknames.
          */
@@ -419,11 +424,11 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
              * the key object is shared by multiple client certificates, such
              * an assumption does not hold.  We have to update the references.
              */
-            LinkSharedKeyObject(pem_nobjs, i);
+            LinkSharedKeyObject(pem_nobjs, curObj->arrayIdx);
 
             if (CKO_CERTIFICATE == objClass) {
                 const int ref = atoi(curObj->id.data);
-                if (0 < ref && ref < pem_nobjs && !pem_objs[ref]) {
+                if (0 < ref && ref < pem_nobjs && !FindObjectByArrayIdx(ref)) {
                     /* The certificate we are going to reuse refers to an
                      * object that has already been removed.  Make it refer
                      * to the object that will be added next (private key).
@@ -433,7 +438,8 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
                 }
             }
 
-            plog("AddObjectIfNeeded: re-using internal object #%i\n", i);
+            plog("AddObjectIfNeeded: re-using internal object #%li\n",
+                 curObj->arrayIdx);
             curObj->refCount ++;
             return curObj;
         }
@@ -448,18 +454,9 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
     /* initialize pointers to functions */
     pem_CreateMDObject(NULL, io, NULL);
 
-    io->gobjIndex = pem_nobjs;
-
-    /* add object to global array */
-    if (pem_objs)
-        pem_objs = NSS_ZRealloc(pem_objs,
-                (pem_nobjs + 1) * sizeof(pemInternalObject *));
-    else
-        pem_objs = NSS_ZNEWARRAY(NULL, pemInternalObject *, 1);
-    if (!pem_objs)
-        return NULL;
-    pem_objs[pem_nobjs] = io;
-    pem_nobjs++;
+    /* add object to global list */
+    io->arrayIdx = pem_nobjs++;
+    list_add_tail(&io->gl_list, &pem_objs);
 
     if (pAdded)
         *pAdded = PR_TRUE;
@@ -748,8 +745,7 @@ pem_Finalize
     if (!pemInitialized)
         return;
 
-    NSS_ZFreeIf(pem_objs);
-    pem_objs = NULL;
+    INIT_LIST_HEAD(&pem_objs);
     pem_nobjs = 0;
 
     PR_AtomicSet(&pemInitialized, PR_FALSE);
diff --git a/src/pobject.c b/src/pobject.c
index d80092a..f118f5d 100644
--- a/src/pobject.c
+++ b/src/pobject.c
@@ -672,10 +672,8 @@ pem_DestroyInternalObject
         return;
     }
 
-    if (NULL != pem_objs)
-        /* remove reference to self from the global array */
-        pem_objs[io->gobjIndex] = NULL;
-
+    /* remove self from the global list */
+    list_del(&io->gl_list);
     NSS_ZFreeIf(io);
     return;
 }
@@ -1206,7 +1204,7 @@ pem_CreateObject
                 goto loser;
         }
     } else if (objClass == CKO_PRIVATE_KEY) {
-        int i;
+        pemInternalObject *curObj;
         SECItem certDER;
         PRBool added;
 
@@ -1220,30 +1218,27 @@ pem_CreateObject
 
         /* Brute force: find the id of the certificate, if any, in this slot */
         objid = -1;
-        for (i = pem_nobjs - 1; 0 <= i; i--) {
-            if (NULL == pem_objs[i])
-                continue;
-
-            if (slotID != pem_objs[i]->slotID)
+        list_for_each_entry_reverse(curObj, &pem_objs, gl_list) {
+            if (slotID != curObj->slotID)
                 continue;
 
-            if (pem_objs[i]->type != pemCert)
+            if (curObj->type != pemCert)
                 continue;
 
-            if (atoi(pem_objs[i]->id.data) != pem_nobjs)
+            if (atoi(curObj->id.data) != pem_nobjs)
                 /* not a certificate that refers to the key being added */
                 continue;
 
             objid = pem_nobjs;
-            certDER.data = NSS_ZAlloc(NULL, pem_objs[i]->derCert->len);
+            certDER.data = NSS_ZAlloc(NULL, curObj->derCert->len);
 
             if (certDER.data == NULL)
                 goto loser;
 
-            certDER.len = pem_objs[i]->derCert->len;
+            certDER.len = curObj->derCert->len;
             memcpy(certDER.data,
-                    pem_objs[i]->derCert->data,
-                    pem_objs[i]->derCert->len);
+                    curObj->derCert->data,
+                    curObj->derCert->len);
             break;
         }
 
diff --git a/src/psession.c b/src/psession.c
index 4e94a7f..13a5e5d 100644
--- a/src/psession.c
+++ b/src/psession.c
@@ -241,7 +241,7 @@ pem_mdSession_Login
     NSSLOWKEYPrivateKey *lpk = NULL;
     PLArenaPool *arena;
     SECItem plain;
-    int i;
+    pemInternalObject *curObj;
 
     fwSlot = NSSCKFWToken_GetFWSlot(fwToken);
     slotID = NSSCKFWSlot_GetSlotID(fwSlot);
@@ -256,12 +256,9 @@ pem_mdSession_Login
     token_needsLogin[slotID - 1] = PR_FALSE;
 
     /* Find the right key object */
-    for (i = 0; i < pem_nobjs; i++) {
-        if (NULL == pem_objs[i])
-            continue;
-
-        if ((slotID == pem_objs[i]->slotID) && (pem_objs[i]->type == pemBareKey)) {
-            io = pem_objs[i];
+    list_for_each_entry(curObj, &pem_objs, gl_list) {
+        if ((slotID == curObj->slotID) && (curObj->type == pemBareKey)) {
+            io = curObj;
             break;
         }
     }
-- 
2.17.2


From 6d968e0f54bfd48029086ae41067e6024bde76a2 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Fri, 21 Dec 2018 13:43:47 +0100
Subject: [PATCH 3/5] ckpem: define pem_nobjs and objid as long to avoid
 wraparound

Fixes #2

Upstream-commit: 5a14b625f529b35f997f60f4ff302f0bfc6ddc16
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 src/ckpem.h   |  4 ++--
 src/pfind.c   |  2 +-
 src/pinst.c   | 29 +++++++++++++++--------------
 src/pobject.c |  4 ++--
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/ckpem.h b/src/ckpem.h
index caad74b..5d05087 100644
--- a/src/ckpem.h
+++ b/src/ckpem.h
@@ -216,7 +216,7 @@ struct pemInternalObjectStr {
 };
 
 NSS_EXTERN_DATA struct list_head pem_objs;
-NSS_EXTERN_DATA int pem_nobjs;
+NSS_EXTERN_DATA long pem_nobjs;
 NSS_EXTERN_DATA int token_needsLogin[];
 NSS_EXTERN_DATA NSSCKMDSlot *lastEventSlot;
 
@@ -304,7 +304,7 @@ PRBool pem_ParseString(const char *inputstring, const char delimiter,
 
 pemInternalObject *
 AddObjectIfNeeded(CK_OBJECT_CLASS objClass, pemObjectType type,
-                  SECItem *certDER, SECItem *keyDER, const char *filename, int objid,
+                  SECItem *certDER, SECItem *keyDER, const char *filename, long objid,
                   CK_SLOT_ID slotID, PRBool *pAdded);
 
 void pem_DestroyInternalObject (pemInternalObject *io);
diff --git a/src/pfind.c b/src/pfind.c
index 3f0fdcb..958c0d5 100644
--- a/src/pfind.c
+++ b/src/pfind.c
@@ -259,7 +259,7 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate,
 
     plog("collect_objects slot #%ld, ", slotID);
     plog("%d attributes, ", ulAttributeCount);
-    plog("%d objects created in total.\n", pem_nobjs);
+    plog("%ld objects created in total.\n", pem_nobjs);
     plog("Looking for: ");
     /*
      * now determine type of the object
diff --git a/src/pinst.c b/src/pinst.c
index 810db5a..87bf7c2 100644
--- a/src/pinst.c
+++ b/src/pinst.c
@@ -51,7 +51,7 @@
 static PRBool pemInitialized = PR_FALSE;
 
 LIST_HEAD(pem_objs);
-int pem_nobjs = 0;
+long pem_nobjs = 0L;
 int token_needsLogin[NUM_SLOTS];
 NSSCKMDSlot *lastEventSlot;
 
@@ -182,12 +182,12 @@ GetCertFields(unsigned char *cert, int cert_length,
 }
 
 static CK_RV
-assignObjectID(pemInternalObject *o, int objid)
+assignObjectID(pemInternalObject *o, const long objid)
 {
-    char id[16];
+    char id[24];
     int len;
 
-    sprintf(id, "%d", objid);
+    sprintf(id, "%ld", objid);
     len = strlen(id) + 1;       /* zero terminate */
     o->id.size = len;
     o->id.data = NSS_ZAlloc(NULL, len);
@@ -202,7 +202,7 @@ static pemInternalObject *
 CreateObject(CK_OBJECT_CLASS objClass,
              pemObjectType type, SECItem * certDER,
              SECItem * keyDER, const char *filename,
-             int objid, CK_SLOT_ID slotID)
+             long objid, CK_SLOT_ID slotID)
 {
     pemInternalObject *o;
     SECItem subject;
@@ -226,17 +226,17 @@ CreateObject(CK_OBJECT_CLASS objClass,
 
     switch (objClass) {
     case CKO_CERTIFICATE:
-        plog("Creating cert nick %s id %d in slot %ld\n", nickname, objid, slotID);
+        plog("Creating cert nick %s id %ld in slot %ld\n", nickname, objid, slotID);
         memset(&o->u.cert, 0, sizeof(o->u.cert));
         break;
     case CKO_PRIVATE_KEY:
-        plog("Creating key id %d in slot %ld\n", objid, slotID);
+        plog("Creating key id %ld in slot %ld\n", objid, slotID);
         memset(&o->u.key, 0, sizeof(o->u.key));
         /* more unique nicknames - https://bugzilla.redhat.com/689031#c66 */
         nickname = filename;
         break;
     case CKO_NETSCAPE_TRUST:
-        plog("Creating trust nick %s id %d in slot %ld\n", nickname, objid, slotID);
+        plog("Creating trust nick %s id %ld in slot %ld\n", nickname, objid, slotID);
         memset(&o->u.trust, 0, sizeof(o->u.trust));
         break;
     }
@@ -360,12 +360,12 @@ derEncodingsMatch(CK_OBJECT_CLASS objClass, pemInternalObject * obj,
 }
 
 static CK_RV
-LinkSharedKeyObject(int oldKeyIdx, int newKeyIdx)
+LinkSharedKeyObject(const long oldKeyIdx, const long newKeyIdx)
 {
     pemInternalObject *obj;
     list_for_each_entry(obj, &pem_objs, gl_list) {
         CK_RV rv;
-        if (atoi(obj->id.data) != oldKeyIdx)
+        if (atol(obj->id.data) != oldKeyIdx)
             continue;
 
         NSS_ZFreeIf(obj->id.data);
@@ -394,7 +394,7 @@ pemInternalObject *
 AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
                   pemObjectType type, SECItem * certDER,
                   SECItem * keyDER, const char *filename,
-                  int objid, CK_SLOT_ID slotID, PRBool *pAdded)
+                  long objid, CK_SLOT_ID slotID, PRBool *pAdded)
 {
     pemInternalObject *curObj;
 
@@ -427,7 +427,7 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
             LinkSharedKeyObject(pem_nobjs, curObj->arrayIdx);
 
             if (CKO_CERTIFICATE == objClass) {
-                const int ref = atoi(curObj->id.data);
+                const long ref = atol(curObj->id.data);
                 if (0 < ref && ref < pem_nobjs && !FindObjectByArrayIdx(ref)) {
                     /* The certificate we are going to reuse refers to an
                      * object that has already been removed.  Make it refer
@@ -471,7 +471,8 @@ AddCertificate(char *certfile, char *keyfile, PRBool cacert,
 {
     pemInternalObject *o = NULL;
     CK_RV error = 0;
-    int objid, i = 0;
+    long objid;
+    int i = 0;
     SECItem **objs = NULL;
     char *ivstring = NULL;
     int cipher;
@@ -746,7 +747,7 @@ pem_Finalize
         return;
 
     INIT_LIST_HEAD(&pem_objs);
-    pem_nobjs = 0;
+    pem_nobjs = 0L;
 
     PR_AtomicSet(&pemInitialized, PR_FALSE);
 }
diff --git a/src/pobject.c b/src/pobject.c
index f118f5d..9a8321c 100644
--- a/src/pobject.c
+++ b/src/pobject.c
@@ -1089,7 +1089,7 @@ pem_CreateObject
     SECItem **derlist = NULL;
     int nobjs = 0;
     int i;
-    int objid;
+    long objid;
 #if 0
     pemToken *token;
 #endif
@@ -1225,7 +1225,7 @@ pem_CreateObject
             if (curObj->type != pemCert)
                 continue;
 
-            if (atoi(curObj->id.data) != pem_nobjs)
+            if (atol(curObj->id.data) != pem_nobjs)
                 /* not a certificate that refers to the key being added */
                 continue;
 
-- 
2.17.2


From 513ac624ec4c640f53f2bcdee11dc332358b77d7 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Fri, 21 Dec 2018 14:26:01 +0100
Subject: [PATCH 4/5] ckpem: keep objid as number to avoid use of atol()

Fixes #2

Upstream-commit: 188c63f7b824e256d20b70ee080ea88d96d8e9c6
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 src/ckpem.h   | 1 +
 src/pinst.c   | 5 +++--
 src/pobject.c | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/ckpem.h b/src/ckpem.h
index 5d05087..e2f18cf 100644
--- a/src/ckpem.h
+++ b/src/ckpem.h
@@ -194,6 +194,7 @@ struct pemInternalObjectStr {
   CK_OBJECT_CLASS objClass;
   NSSItem         hashKey;
   NSSItem         id;
+  long            objid;
   unsigned char   hashKeyData[128];
   SECItem         *derCert;
   char            *nickname;
diff --git a/src/pinst.c b/src/pinst.c
index 87bf7c2..26f3ac2 100644
--- a/src/pinst.c
+++ b/src/pinst.c
@@ -195,6 +195,7 @@ assignObjectID(pemInternalObject *o, const long objid)
         return CKR_HOST_MEMORY;
 
     memcpy(o->id.data, id, len);
+    o->objid = objid;
     return CKR_OK;
 }
 
@@ -365,7 +366,7 @@ LinkSharedKeyObject(const long oldKeyIdx, const long newKeyIdx)
     pemInternalObject *obj;
     list_for_each_entry(obj, &pem_objs, gl_list) {
         CK_RV rv;
-        if (atol(obj->id.data) != oldKeyIdx)
+        if (obj->objid != oldKeyIdx)
             continue;
 
         NSS_ZFreeIf(obj->id.data);
@@ -427,7 +428,7 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass,
             LinkSharedKeyObject(pem_nobjs, curObj->arrayIdx);
 
             if (CKO_CERTIFICATE == objClass) {
-                const long ref = atol(curObj->id.data);
+                const long ref = curObj->objid;
                 if (0 < ref && ref < pem_nobjs && !FindObjectByArrayIdx(ref)) {
                     /* The certificate we are going to reuse refers to an
                      * object that has already been removed.  Make it refer
diff --git a/src/pobject.c b/src/pobject.c
index 9a8321c..2ffffed 100644
--- a/src/pobject.c
+++ b/src/pobject.c
@@ -1225,7 +1225,7 @@ pem_CreateObject
             if (curObj->type != pemCert)
                 continue;
 
-            if (atol(curObj->id.data) != pem_nobjs)
+            if (curObj->objid != pem_nobjs)
                 /* not a certificate that refers to the key being added */
                 continue;
 
-- 
2.17.2


From db140f2d9259ac73d59c50a658d444e418b81c3c Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 16 Jan 2019 16:19:50 +0100
Subject: [PATCH 5/5] pfind: fix use of uninitialized var in plog() call

Detected by static analysis:

Error: UNINIT (CWE-457):
nss-pem-1.0.3/src/pfind.c:251: var_decl: Declaring variable "i" without initializer.
nss-pem-1.0.3/src/pfind.c:305: uninit_use_in_call: Using uninitialized value "i" when calling "plog".
  303|       list_for_each_entry(obj, &pem_objs, gl_list) {
  304|           int match = 1; /* matches type if type not specified */
  305|->         plog("  %d type = %d\n", i, obj->type);
  306|           if (type != pemAll) {
  307|               /* type specified - must match given type */

Error: CLANG_WARNING:
nss-pem-1.0.3/src/pfind.c:305:9: warning: Function call argument is an uninitialized value
nss-pem-1.0.3/src/pfind.c:364:9: note: Assuming 'fwSlot' is not equal to null
nss-pem-1.0.3/src/pfind.c:364:5: note: Taking false branch
nss-pem-1.0.3/src/pfind.c:370:9: note: Assuming 'arena' is not equal to null
nss-pem-1.0.3/src/pfind.c:370:5: note: Taking false branch
nss-pem-1.0.3/src/pfind.c:375:9: note: Assuming 'rv' is not equal to null
nss-pem-1.0.3/src/pfind.c:375:5: note: Taking false branch
nss-pem-1.0.3/src/pfind.c:381:9: note: Assuming 'fo' is not equal to null
nss-pem-1.0.3/src/pfind.c:381:5: note: Taking false branch
nss-pem-1.0.3/src/pfind.c:395:9: note: Calling 'collect_objects'
nss-pem-1.0.3/src/pfind.c:251:5: note: 'i' declared without an initial value
nss-pem-1.0.3/src/pfind.c:267:5: note: Control jumps to 'case 0:'  at line 293
nss-pem-1.0.3/src/pfind.c:296:9: note:  Execution continues on line 303
nss-pem-1.0.3/src/pfind.c:305:9: note: Function call argument is an uninitialized value

Error: COMPILER_WARNING:
nss-pem-1.0.3/src/pfind.c: scope_hint: In function 'pem_FindObjectsInit'
nss-pem-1.0.3/src/pfind.c:305:13: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized]
nss-pem-1.0.3/src/pfind.c:251:14: note: 'i' was declared here

Upstream-commit: 5c05ed268675c7544b1a51198e1e9e566be17608
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 src/pfind.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/pfind.c b/src/pfind.c
index 958c0d5..83d5b89 100644
--- a/src/pfind.c
+++ b/src/pfind.c
@@ -248,7 +248,6 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate,
                 pemInternalObject *** result_array,
                 CK_RV * pError, CK_SLOT_ID slotID)
 {
-    PRUint32 i;
     size_t result_array_entries = 0;
     size_t result_array_capacity = 0;
     pemObjectType type = pemRaw;
@@ -302,7 +301,7 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate,
     /* find objects */
     list_for_each_entry(obj, &pem_objs, gl_list) {
         int match = 1; /* matches type if type not specified */
-        plog("  %d type = %d\n", i, obj->type);
+        plog("  %ld type = %d\n", obj->arrayIdx, obj->type);
         if (type != pemAll) {
             /* type specified - must match given type */
             match = (type == obj->type);
-- 
2.17.2