Align with OpenSSL on constness of static ASN1_OBJECTs.
ASN1_OBJECTs are awkward. Sometimes they are static, when returned from
OBJ_nid2obj, and sometimes they are dynamic, when parsed from
crypto/asn1.
Most structures in crypto/asn1 need to support unknown OIDs and thus
must own their ASN1_OBJECTs. But they also may be initialized with
static ones in various APIs, such as X509_ALGOR_set0. To make that work,
ASN1_OBJECT_free detects static ASN1_OBJECTs and is a no-op.
Functions like X509_ALGOR_set0 take ownership, so OpenSSL has them take
a non-const ASN1_OBJECT*. To match, OBJ_nid2obj then returns a non-const
ASN1_OBJECT*, to signal that it is freeable.
However, this means OBJ_nid2obj's mutability doesn't match its return
type. In the fork, we switched OBJ_nid2obj to return const. But, in
doing so, we had to make X509_ALGOR_set0 and X509_PUBKEY_set0_param take
const ASN1_OBJECT, even though they would actually take ownership of
dynamic ASN1_OBJECTs. There are also a few internal casts with a TODO to
be const-correct.
Neither situation is ideal. (Perhaps a more sound model would be to copy
static ASN1_OBJECTs before putting them in most structs. But that would
not match current usage.) But I think aligning with OpenSSL is the
lesser evil here, since it avoids misleading set0 functions. Managing
ownership of ASN1_OBJECTs is much more common than mutating them. To
that end, I've added a note that ASN1_OBJECTs you didn't create must be
assumed immutable[*].
Update-Note: The change to OBJ_nid2obj should be compatible. The changes
to X509_PUBKEY_set0_param and X509_ALGOR_set0 may require fixing some
pointer types.
[*] This is *almost* honored by all of our functions. The exception is
c2i_ASN1_OBJECT, which instead checks the DYNAMIC flag as part of the
object reuse business. This would come up if we ever embedded
ASN1_OBJECTs directly in structs.
Change-Id: I1e6c700645c12b43323dd3887adb74e795c285b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46164
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/obj/obj.c b/crypto/obj/obj.c
index 3bf1abf..3053132 100644
--- a/crypto/obj/obj.c
+++ b/crypto/obj/obj.c
@@ -338,12 +338,12 @@
return 1;
}
-const ASN1_OBJECT *OBJ_nid2obj(int nid) {
+ASN1_OBJECT *OBJ_nid2obj(int nid) {
if (nid >= 0 && nid < NUM_NID) {
if (nid != NID_undef && kObjects[nid].nid == NID_undef) {
goto err;
}
- return &kObjects[nid];
+ return (ASN1_OBJECT *)&kObjects[nid];
}
CRYPTO_STATIC_MUTEX_lock_read(&global_added_lock);
@@ -411,7 +411,7 @@
}
if (nid != NID_undef) {
- return (ASN1_OBJECT*) OBJ_nid2obj(nid);
+ return OBJ_nid2obj(nid);
}
}
diff --git a/crypto/pkcs7/pkcs7_x509.c b/crypto/pkcs7/pkcs7_x509.c
index 107bdea..5afd28b 100644
--- a/crypto/pkcs7/pkcs7_x509.c
+++ b/crypto/pkcs7/pkcs7_x509.c
@@ -235,7 +235,7 @@
return NULL;
}
OPENSSL_memset(ret, 0, sizeof(PKCS7));
- ret->type = (ASN1_OBJECT *)OBJ_nid2obj(NID_pkcs7_signed);
+ ret->type = OBJ_nid2obj(NID_pkcs7_signed);
ret->d.sign = OPENSSL_malloc(sizeof(PKCS7_SIGNED));
if (ret->d.sign == NULL) {
goto err;
diff --git a/crypto/x509/x509_req.c b/crypto/x509/x509_req.c
index 9ab6e9d..d7da620 100644
--- a/crypto/x509/x509_req.c
+++ b/crypto/x509/x509_req.c
@@ -245,7 +245,7 @@
goto err;
at = NULL;
attr->single = 0;
- attr->object = (ASN1_OBJECT *)OBJ_nid2obj(nid);
+ attr->object = OBJ_nid2obj(nid);
if (!req->req_info->attributes) {
if (!(req->req_info->attributes = sk_X509_ATTRIBUTE_new_null()))
goto err;
diff --git a/crypto/x509/x_algor.c b/crypto/x509/x_algor.c
index 13c9a8c..7b3d790 100644
--- a/crypto/x509/x_algor.c
+++ b/crypto/x509/x_algor.c
@@ -77,8 +77,7 @@
IMPLEMENT_ASN1_SET_OF(X509_ALGOR)
-int X509_ALGOR_set0(X509_ALGOR *alg, const ASN1_OBJECT *aobj, int ptype,
- void *pval)
+int X509_ALGOR_set0(X509_ALGOR *alg, ASN1_OBJECT *aobj, int ptype, void *pval)
{
if (!alg)
return 0;
@@ -89,9 +88,8 @@
return 0;
}
if (alg) {
- if (alg->algorithm)
- ASN1_OBJECT_free(alg->algorithm);
- alg->algorithm = (ASN1_OBJECT *)aobj;
+ ASN1_OBJECT_free(alg->algorithm);
+ alg->algorithm = aobj;
}
if (ptype == 0)
return 1;
diff --git a/crypto/x509/x_attrib.c b/crypto/x509/x_attrib.c
index 9d9397c..194f9e1 100644
--- a/crypto/x509/x_attrib.c
+++ b/crypto/x509/x_attrib.c
@@ -85,7 +85,7 @@
X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int atrtype, void *value)
{
- const ASN1_OBJECT *obj = OBJ_nid2obj(nid);
+ ASN1_OBJECT *obj = OBJ_nid2obj(nid);
if (obj == NULL) {
return NULL;
}
@@ -96,9 +96,7 @@
goto err;
}
- /* TODO(fork): const correctness. |ASN1_OBJECT| is messy because static
- * objects are const but freeable with a no-op |ASN1_OBJECT_free|. */
- ret->object = (ASN1_OBJECT *)obj;
+ ret->object = obj;
ret->single = 0;
ret->value.set = sk_ASN1_TYPE_new_null();
if (ret->value.set == NULL ||
diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c
index 37dee49..c3fb1e3 100644
--- a/crypto/x509/x_pubkey.c
+++ b/crypto/x509/x_pubkey.c
@@ -180,9 +180,8 @@
return NULL;
}
-int X509_PUBKEY_set0_param(X509_PUBKEY *pub, const ASN1_OBJECT *aobj,
- int ptype, void *pval,
- unsigned char *penc, int penclen)
+int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *aobj, int ptype,
+ void *pval, unsigned char *penc, int penclen)
{
if (!X509_ALGOR_set0(pub->algor, aobj, ptype, pval))
return 0;
diff --git a/crypto/x509v3/v3_cpols.c b/crypto/x509v3/v3_cpols.c
index 216e7ae..a2ed01c 100644
--- a/crypto/x509v3/v3_cpols.c
+++ b/crypto/x509v3/v3_cpols.c
@@ -239,8 +239,7 @@
goto merr;
if (!sk_POLICYQUALINFO_push(pol->qualifiers, qual))
goto merr;
- /* TODO(fork): const correctness */
- qual->pqualid = (ASN1_OBJECT *)OBJ_nid2obj(NID_id_qt_cps);
+ qual->pqualid = OBJ_nid2obj(NID_id_qt_cps);
if (qual->pqualid == NULL) {
OPENSSL_PUT_ERROR(X509V3, ERR_R_INTERNAL_ERROR);
goto err;
@@ -307,8 +306,7 @@
POLICYQUALINFO *qual;
if (!(qual = POLICYQUALINFO_new()))
goto merr;
- /* TODO(fork): const correctness */
- qual->pqualid = (ASN1_OBJECT *)OBJ_nid2obj(NID_id_qt_unotice);
+ qual->pqualid = OBJ_nid2obj(NID_id_qt_unotice);
if (qual->pqualid == NULL) {
OPENSSL_PUT_ERROR(X509V3, ERR_R_INTERNAL_ERROR);
goto err;
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 9269553..3c75ee8 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -356,6 +356,16 @@
#define ASN1_OBJECT_FLAG_DYNAMIC 0x01 // internal use
#define ASN1_OBJECT_FLAG_DYNAMIC_STRINGS 0x04 // internal use
#define ASN1_OBJECT_FLAG_DYNAMIC_DATA 0x08 // internal use
+
+// An asn1_object_st (aka |ASN1_OBJECT|) represents an ASN.1 OBJECT IDENTIFIER.
+//
+// Note: Although the struct is exposed, mutating an |ASN1_OBJECT| is only
+// permitted when initializing it. The library maintains a table of static
+// |ASN1_OBJECT|s, which may be referenced by non-const |ASN1_OBJECT| pointers.
+// Code which receives an |ASN1_OBJECT| pointer externally must assume it is
+// immutable, even if the pointer is not const.
+//
+// TODO(davidben): Document this more completely in its own section.
struct asn1_object_st {
const char *sn, *ln;
int nid;
diff --git a/include/openssl/obj.h b/include/openssl/obj.h
index 764188f..49b7adc 100644
--- a/include/openssl/obj.h
+++ b/include/openssl/obj.h
@@ -124,9 +124,22 @@
// Getting information about nids.
-// OBJ_nid2obj returns the ASN1_OBJECT corresponding to |nid|, or NULL if |nid|
-// is unknown.
-OPENSSL_EXPORT const ASN1_OBJECT *OBJ_nid2obj(int nid);
+// OBJ_nid2obj returns the |ASN1_OBJECT| corresponding to |nid|, or NULL if
+// |nid| is unknown.
+//
+// This function returns a static, immutable |ASN1_OBJECT|. Although the output
+// is not const, callers may not mutate it. It is also not necessary to release
+// the object with |ASN1_OBJECT_free|.
+//
+// However, functions like |X509_ALGOR_set0| expect to take ownership of a
+// possibly dynamically-allocated |ASN1_OBJECT|. |ASN1_OBJECT_free| is a no-op
+// for static |ASN1_OBJECT|s, so |OBJ_nid2obj| is compatible with such
+// functions.
+//
+// Callers are encouraged to store the result of this function in a const
+// pointer. However, if using functions like |X509_ALGOR_set0|, callers may use
+// a non-const pointer and manage ownership.
+OPENSSL_EXPORT ASN1_OBJECT *OBJ_nid2obj(int nid);
// OBJ_nid2sn returns the short name for |nid|, or NULL if |nid| is unknown.
OPENSSL_EXPORT const char *OBJ_nid2sn(int nid);
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 86896af..5263d31 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -905,7 +905,7 @@
OPENSSL_EXPORT X509_REVOKED *X509_REVOKED_dup(X509_REVOKED *rev);
OPENSSL_EXPORT X509_REQ *X509_REQ_dup(X509_REQ *req);
OPENSSL_EXPORT X509_ALGOR *X509_ALGOR_dup(X509_ALGOR *xn);
-OPENSSL_EXPORT int X509_ALGOR_set0(X509_ALGOR *alg, const ASN1_OBJECT *aobj,
+OPENSSL_EXPORT int X509_ALGOR_set0(X509_ALGOR *alg, ASN1_OBJECT *aobj,
int ptype, void *pval);
OPENSSL_EXPORT void X509_ALGOR_get0(const ASN1_OBJECT **paobj, int *pptype,
const void **ppval,
@@ -1538,10 +1538,9 @@
const unsigned char **pk, int *ppklen,
X509_ALGOR **pa, PKCS8_PRIV_KEY_INFO *p8);
-OPENSSL_EXPORT int X509_PUBKEY_set0_param(X509_PUBKEY *pub,
- const ASN1_OBJECT *aobj, int ptype,
- void *pval, unsigned char *penc,
- int penclen);
+OPENSSL_EXPORT int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *aobj,
+ int ptype, void *pval,
+ unsigned char *penc, int penclen);
OPENSSL_EXPORT int X509_PUBKEY_get0_param(ASN1_OBJECT **ppkalg,
const unsigned char **pk, int *ppklen,
X509_ALGOR **pa, X509_PUBKEY *pub);
