Refine looking up of attributes in arrays

There was a class of bugs for looking up invalid or empty
attributes in the internal PKCS#11 attribute arrays.

 * Refine what p11_attrs_find_valid() treats as valid
 * Rename p11_attrs_is_empty() to p11_attrs_terminator() for clarity

4 files changed, 91 insertions, 80 deletions

diff --git a/common/attrs.c b/common/attrs.c
index 0b8032c..a438264 100644
--- a/common/attrs.c
+++ b/common/attrs.c
@@ -52,7 +52,7 @@
 #define ELEMS(x) (sizeof (x) / sizeof (x[0]))
 
 bool
-p11_attrs_is_empty (const CK_ATTRIBUTE *attrs)
+p11_attrs_terminator (const CK_ATTRIBUTE *attrs)
 {
 	return (attrs == NULL || attrs->type == CKA_INVALID);
 }
@@ -65,7 +65,7 @@ p11_attrs_count (const CK_ATTRIBUTE *attrs)
 	if (attrs == NULL)
 		return 0UL;
 
-	for (count = 0; !p11_attrs_is_empty (attrs); count++, attrs++);
+	for (count = 0; !p11_attrs_terminator (attrs); count++, attrs++);
 
 	return count;
 }
@@ -79,7 +79,7 @@ p11_attrs_free (void *attrs)
 	if (!attrs)
 		return;
 
-	for (i = 0; !p11_attrs_is_empty (ats + i); i++)
+	for (i = 0; !p11_attrs_terminator (ats + i); i++)
 		free (ats[i].pValue);
 	free (ats);
 }
@@ -147,7 +147,7 @@ attrs_build (CK_ATTRIBUTE *attrs,
 
 	/* Mark this as the end */
 	(attrs + at)->type = CKA_INVALID;
-	assert (p11_attrs_is_empty (attrs + at));
+	assert (p11_attrs_terminator (attrs + at));
 	return attrs;
 }
 
@@ -248,7 +248,7 @@ p11_attrs_find (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type)
 			return attrs + i;
 	}
@@ -278,7 +278,7 @@ p11_attrs_find_bool (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
 		    attrs[i].ulValueLen == sizeof (CK_BBOOL) &&
 		    attrs[i].pValue != NULL) {
@@ -317,7 +317,7 @@ p11_attrs_find_ulong (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
 		    attrs[i].ulValueLen == sizeof (CK_ULONG) &&
 		    attrs[i].pValue != NULL) {
@@ -329,24 +329,25 @@ p11_attrs_find_ulong (CK_ATTRIBUTE *attrs,
 	return false;
 }
 
-bool
-p11_attrs_findn_ulong (CK_ATTRIBUTE *attrs,
-                       CK_ULONG count,
-                       CK_ATTRIBUTE_TYPE type,
-                       CK_ULONG *value)
+void *
+p11_attrs_find_value (CK_ATTRIBUTE *attrs,
+                      CK_ATTRIBUTE_TYPE type,
+                      size_t *length)
 {
 	CK_ULONG i;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
-		    attrs[i].ulValueLen == sizeof (CK_ULONG) &&
+		    attrs[i].ulValueLen != 0 &&
+		    attrs[i].ulValueLen != (CK_ULONG)-1 &&
 		    attrs[i].pValue != NULL) {
-			*value = *((CK_ULONG *)attrs[i].pValue);
-			return true;
+			if (length)
+				*length = attrs[i].ulValueLen;
+			return attrs[i].pValue;
 		}
 	}
 
-	return false;
+	return NULL;
 }
 
 CK_ATTRIBUTE *
@@ -355,24 +356,10 @@ p11_attrs_find_valid (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
-		if (attrs[i].type == type &&
-		    attrs[i].ulValueLen != (CK_ULONG)-1)
-			return attrs + i;
-	}
-
-	return NULL;
-}
-
-CK_ATTRIBUTE *
-p11_attrs_findn_valid (CK_ATTRIBUTE *attrs,
-                       CK_ULONG count,
-                       CK_ATTRIBUTE_TYPE type)
-{
-	CK_ULONG i;
-
-	for (i = 0; i < count; i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
+		    attrs[i].pValue != NULL &&
+		    attrs[i].ulValueLen != 0 &&
 		    attrs[i].ulValueLen != (CK_ULONG)-1)
 			return attrs + i;
 	}
@@ -380,7 +367,6 @@ p11_attrs_findn_valid (CK_ATTRIBUTE *attrs,
 	return NULL;
 }
 
-
 bool
 p11_attrs_remove (CK_ATTRIBUTE *attrs,
                   CK_ATTRIBUTE_TYPE type)
@@ -410,7 +396,7 @@ p11_attrs_purge (CK_ATTRIBUTE *attrs)
 {
 	int in, out;
 
-	for (in = 0, out = 0; !p11_attrs_is_empty (attrs + in); in++) {
+	for (in = 0, out = 0; !p11_attrs_terminator (attrs + in); in++) {
 		if (attrs[in].ulValueLen == (CK_ULONG)-1) {
 			free (attrs[in].pValue);
 			attrs[in].pValue = NULL;
@@ -423,7 +409,7 @@ p11_attrs_purge (CK_ATTRIBUTE *attrs)
 	}
 
 	attrs[out].type = CKA_INVALID;
-	assert (p11_attrs_is_empty (attrs + out));
+	assert (p11_attrs_terminator (attrs + out));
 
 }
 
@@ -433,7 +419,7 @@ p11_attrs_match (const CK_ATTRIBUTE *attrs,
 {
 	CK_ATTRIBUTE *attr;
 
-	for (; !p11_attrs_is_empty (match); match++) {
+	for (; !p11_attrs_terminator (match); match++) {
 		attr = p11_attrs_find ((CK_ATTRIBUTE *)attrs, match->type);
 		if (!attr)
 			return false;
diff --git a/common/attrs.h b/common/attrs.h
index 619403d..f6eb950 100644
--- a/common/attrs.h
+++ b/common/attrs.h
@@ -61,7 +61,7 @@ CK_ATTRIBUTE *      p11_attrs_merge         (CK_ATTRIBUTE *attrs,
 
 void                p11_attrs_purge         (CK_ATTRIBUTE *attrs);
 
-bool                p11_attrs_is_empty      (const CK_ATTRIBUTE *attrs);
+bool                p11_attrs_terminator    (const CK_ATTRIBUTE *attrs);
 
 CK_ULONG            p11_attrs_count         (const CK_ATTRIBUTE *attrs);
 
@@ -87,18 +87,13 @@ bool                p11_attrs_find_ulong    (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type,
                                              CK_ULONG *value);
 
-bool                p11_attrs_findn_ulong   (CK_ATTRIBUTE *attrs,
-                                             CK_ULONG count,
+void *              p11_attrs_find_value    (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type,
-                                             CK_ULONG *value);
+                                             size_t *length);
 
 CK_ATTRIBUTE *      p11_attrs_find_valid    (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type);
 
-CK_ATTRIBUTE *      p11_attrs_findn_valid   (CK_ATTRIBUTE *attrs,
-                                             CK_ULONG count,
-                                             CK_ATTRIBUTE_TYPE type);
-
 bool                p11_attrs_remove        (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type);
 
diff --git a/common/mock.c b/common/mock.c
index 411c6eb..3ba3085 100644
--- a/common/mock.c
+++ b/common/mock.c
@@ -124,24 +124,6 @@ free_session (void *data)
 	free (sess);
 }
 
-static bool
-find_boolean_attribute (CK_ATTRIBUTE *attrs,
-                        CK_ATTRIBUTE_TYPE type,
-                        CK_BBOOL *value)
-{
-	CK_ATTRIBUTE *attr;
-
-	attr = p11_attrs_find (attrs, type);
-	if (attr != NULL &&
-	    attr->pValue != NULL &&
-	    attr->ulValueLen == sizeof (CK_BBOOL)) {
-		*value = *((CK_BBOOL *)attr->pValue);
-		return true;
-	}
-
-	return false;
-}
-
 static CK_RV
 lookup_object (Session *sess,
                CK_OBJECT_HANDLE object,
@@ -164,7 +146,7 @@ lookup_object (Session *sess,
 
 	if (!*attrs)
 		return CKR_OBJECT_HANDLE_INVALID;
-	else if (!logged_in && find_boolean_attribute (*attrs, CKA_PRIVATE, &priv) && priv)
+	else if (!logged_in && p11_attrs_find_bool (*attrs, CKA_PRIVATE, &priv) && priv)
 		return CKR_USER_NOT_LOGGED_IN;
 
 	return CKR_OK;
@@ -1183,7 +1165,7 @@ mock_C_CreateObject (CK_SESSION_HANDLE session,
 
 	attrs = p11_attrs_buildn (NULL, template, count);
 
-	if (find_boolean_attribute (attrs, CKA_PRIVATE, &priv) && priv) {
+	if (p11_attrs_find_bool (attrs, CKA_PRIVATE, &priv) && priv) {
 		if (!logged_in) {
 			p11_attrs_free (attrs);
 			return CKR_USER_NOT_LOGGED_IN;
@@ -1191,7 +1173,7 @@ mock_C_CreateObject (CK_SESSION_HANDLE session,
 	}
 
 	*object = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*object), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*object), attrs);
@@ -1232,7 +1214,7 @@ mock_C_CopyObject (CK_SESSION_HANDLE session,
 	if (rv != CKR_OK)
 		return rv;
 
-	if (find_boolean_attribute (attrs, CKA_PRIVATE, &priv) && priv) {
+	if (p11_attrs_find_bool (attrs, CKA_PRIVATE, &priv) && priv) {
 		if (!logged_in)
 			return CKR_USER_NOT_LOGGED_IN;
 	}
@@ -1240,7 +1222,7 @@ mock_C_CopyObject (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (p11_attrs_dup (attrs), template, count);
 
 	*new_object = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*new_object), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*new_object), attrs);
@@ -1310,7 +1292,7 @@ mock_C_GetObjectSize (CK_SESSION_HANDLE session,
 		return rv;
 
 	*size = 0;
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].ulValueLen != (CK_ULONG)-1)
 			*size += attrs[i].ulValueLen;
 	}
@@ -2118,7 +2100,7 @@ prefix_mechanism_init (CK_SESSION_HANDLE session,
 	if (rv != CKR_OK)
 		return rv;
 
-	value = p11_attrs_find (attrs, CKA_VALUE);
+	value = p11_attrs_find_valid (attrs, CKA_VALUE);
 	if (value == NULL)
 		return CKR_KEY_TYPE_INCONSISTENT;
 
@@ -2697,7 +2679,7 @@ mock_C_GenerateKey (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*key), attrs);
@@ -2757,7 +2739,7 @@ mock_C_GenerateKeyPair (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*public_key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*public_key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*public_key), attrs);
@@ -2766,7 +2748,7 @@ mock_C_GenerateKeyPair (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*private_key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*private_key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*private_key), attrs);
@@ -2830,7 +2812,7 @@ mock_C_WrapKey (CK_SESSION_HANDLE session,
 		return CKR_MECHANISM_PARAM_INVALID;
 	}
 
-	attr = p11_attrs_find (attrs, CKA_VALUE);
+	attr = p11_attrs_find_valid (attrs, CKA_VALUE);
 	if (attr == NULL)
 		return CKR_WRAPPED_KEY_INVALID;
 
@@ -2914,7 +2896,7 @@ mock_C_UnwrapKey (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*key), attrs);
@@ -2981,7 +2963,7 @@ mock_C_DeriveKey (CK_SESSION_HANDLE session,
 	copy = p11_attrs_buildn (copy, &value, 1);
 
 	*key = ++unique_identifier;
-	if (find_boolean_attribute (copy, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (copy, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*key), copy);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*key), copy);
diff --git a/common/tests/test-attrs.c b/common/tests/test-attrs.c
index 49350be..f1e6d91 100644
--- a/common/tests/test-attrs.c
+++ b/common/tests/test-attrs.c
@@ -43,6 +43,21 @@
 #include "debug.h"
 
 static void
+test_terminator (CuTest *tc)
+{
+	CK_ATTRIBUTE attrs[] = {
+		{ CKA_LABEL, "label", 5 },
+		{ CKA_LABEL, NULL, 0 },
+		{ CKA_INVALID },
+	};
+
+	CuAssertIntEquals (tc, true, p11_attrs_terminator (attrs + 2));
+	CuAssertIntEquals (tc, true, p11_attrs_terminator (NULL));
+	CuAssertIntEquals (tc, false, p11_attrs_terminator (attrs));
+	CuAssertIntEquals (tc, false, p11_attrs_terminator (attrs + 1));
+}
+
+static void
 test_count (CuTest *tc)
 {
 	CK_BBOOL vtrue = CK_TRUE;
@@ -647,22 +662,53 @@ test_find_ulong (CuTest *tc)
 }
 
 static void
+test_find_value (CuTest *tc)
+{
+	void *value;
+	size_t length;
+
+	CK_ATTRIBUTE attrs[] = {
+		{ CKA_LABEL, "", (CK_ULONG)-1 },
+		{ CKA_LABEL, NULL, 5 },
+		{ CKA_LABEL, "", 0 },
+		{ CKA_LABEL, "test", 4 },
+		{ CKA_VALUE, NULL, 0 },
+		{ CKA_INVALID },
+	};
+
+	value = p11_attrs_find_value (attrs, CKA_LABEL, &length);
+	CuAssertPtrEquals (tc, attrs[3].pValue, value);
+	CuAssertIntEquals (tc, 4, length);
+
+	value = p11_attrs_find_value (attrs, CKA_LABEL, NULL);
+	CuAssertPtrEquals (tc, attrs[3].pValue, value);
+
+	value = p11_attrs_find_value (attrs, CKA_VALUE, &length);
+	CuAssertPtrEquals (tc, NULL, value);
+
+	value = p11_attrs_find_value (attrs, CKA_TOKEN, &length);
+	CuAssertPtrEquals (tc, NULL, value);
+}
+
+static void
 test_find_valid (CuTest *tc)
 {
 	CK_ATTRIBUTE *attr;
 
 	CK_ATTRIBUTE attrs[] = {
 		{ CKA_LABEL, "", (CK_ULONG)-1 },
+		{ CKA_LABEL, NULL, 5 },
+		{ CKA_LABEL, "", 0 },
 		{ CKA_LABEL, "test", 4 },
-		{ CKA_VALUE, NULL, 0 },
+		{ CKA_VALUE, "value", 5 },
 		{ CKA_INVALID },
 	};
 
 	attr = p11_attrs_find_valid (attrs, CKA_LABEL);
-	CuAssertPtrEquals (tc, attrs + 1, attr);
+	CuAssertPtrEquals (tc, attrs + 3, attr);
 
 	attr = p11_attrs_find_valid (attrs, CKA_VALUE);
-	CuAssertPtrEquals (tc, attrs + 2, attr);
+	CuAssertPtrEquals (tc, attrs + 4, attr);
 
 	attr = p11_attrs_find_valid (attrs, CKA_TOKEN);
 	CuAssertPtrEquals (tc, NULL, attr);
@@ -682,6 +728,7 @@ main (void)
 	SUITE_ADD_TEST (suite, test_hash);
 	SUITE_ADD_TEST (suite, test_to_string);
 
+	SUITE_ADD_TEST (suite, test_terminator);
 	SUITE_ADD_TEST (suite, test_count);
 	SUITE_ADD_TEST (suite, test_build_one);
 	SUITE_ADD_TEST (suite, test_build_two);
@@ -702,6 +749,7 @@ main (void)
 	SUITE_ADD_TEST (suite, test_findn);
 	SUITE_ADD_TEST (suite, test_find_bool);
 	SUITE_ADD_TEST (suite, test_find_ulong);
+	SUITE_ADD_TEST (suite, test_find_value);
 	SUITE_ADD_TEST (suite, test_find_valid);
 	SUITE_ADD_TEST (suite, test_remove);