Skip to content

Commit 9860446

Browse files
WillChilds-KleinWillChilds-Klein
authored
Change sk_*_find signature to 2-arg for OpenSSL comapat (#1429)
* Change sk_*_find signature to 2-arg for OpenSSL comapat OpenSSL [omits][1] the |out_index| parameter from |sk_*_find|. This (breaking) change conforms with that interface and exposes our old, 3-arg interface with |out_index| as |sk_*_find_awslc|. [1]: https://www.openssl.org/docs/man1.1.1/man3/sk_TYPE_find.html
1 parent ddc449d commit 9860446

File tree

19 files changed

+81
-98
lines changed

19 files changed

+81
-98
lines changed

‎crypto/stack/stack_test.cc‎

Lines changed: 35 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,11 @@ TEST(StackTest, Basic) {
113113
value = TEST_INT_new(6);
114114
ASSERT_TRUE(value);
115115
size_t index;
116-
EXPECT_FALSE(sk_TEST_INT_find(sk.get(), &index, value.get()));
117-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, raw));
118-
EXPECT_EQ(4u, index);
116+
EXPECT_FALSE(sk_TEST_INT_find_awslc(sk.get(), &index, value.get()));
117+
EXPECT_EQ(-1, sk_TEST_INT_find(sk.get(), value.get()));
118+
EXPECT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, raw));
119+
EXPECT_EQ(4UL, index);
120+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), raw));
119121

120122
// sk_TEST_INT_insert can also insert values at the end.
121123
value = TEST_INT_new(7);
@@ -244,12 +246,14 @@ TEST(StackTest, Sorted) {
244246
auto ten = TEST_INT_new(10);
245247
ASSERT_TRUE(ten);
246248
size_t index;
247-
EXPECT_FALSE(sk_TEST_INT_find(sk.get(), &index, ten.get()));
249+
EXPECT_FALSE(sk_TEST_INT_find_awslc(sk.get(), &index, ten.get()));
250+
EXPECT_EQ(-1, sk_TEST_INT_find(sk.get(), ten.get()));
248251

249252
auto three = TEST_INT_new(3);
250253
ASSERT_TRUE(three);
251-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, three.get()));
254+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, three.get()));
252255
EXPECT_EQ(3, *sk_TEST_INT_value(sk.get(), index));
256+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), three.get()));
253257

254258
sk_TEST_INT_sort(sk.get());
255259
EXPECT_TRUE(sk_TEST_INT_is_sorted(sk.get()));
@@ -264,11 +268,13 @@ TEST(StackTest, Sorted) {
264268

265269
// When sorted, find uses binary search.
266270
ASSERT_TRUE(ten);
267-
EXPECT_FALSE(sk_TEST_INT_find(sk.get(), &index, ten.get()));
271+
EXPECT_FALSE(sk_TEST_INT_find_awslc(sk.get(), &index, ten.get()));
272+
EXPECT_EQ(-1, sk_TEST_INT_find(sk.get(), ten.get()));
268273

269274
ASSERT_TRUE(three);
270-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, three.get()));
275+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, three.get()));
271276
EXPECT_EQ(3u, index);
277+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), three.get()));
272278

273279
// Copies preserve comparison and sorted information.
274280
bssl::UniquePtr<STACK_OF(TEST_INT)> copy(sk_TEST_INT_deep_copy(
@@ -279,14 +285,16 @@ TEST(StackTest, Sorted) {
279285
TEST_INT_free));
280286
ASSERT_TRUE(copy);
281287
EXPECT_TRUE(sk_TEST_INT_is_sorted(copy.get()));
282-
ASSERT_TRUE(sk_TEST_INT_find(copy.get(), &index, three.get()));
288+
ASSERT_TRUE(sk_TEST_INT_find_awslc(copy.get(), &index, three.get()));
283289
EXPECT_EQ(3u, index);
290+
EXPECT_EQ((int) index, sk_TEST_INT_find(copy.get(), three.get()));
284291

285292
ShallowStack copy2(sk_TEST_INT_dup(sk.get()));
286293
ASSERT_TRUE(copy2);
287294
EXPECT_TRUE(sk_TEST_INT_is_sorted(copy2.get()));
288-
ASSERT_TRUE(sk_TEST_INT_find(copy2.get(), &index, three.get()));
295+
ASSERT_TRUE(sk_TEST_INT_find_awslc(copy2.get(), &index, three.get()));
289296
EXPECT_EQ(3u, index);
297+
EXPECT_EQ((int) index, sk_TEST_INT_find(copy.get(), three.get()));
290298

291299
// Removing elements does not affect sortedness.
292300
TEST_INT_free(sk_TEST_INT_delete(sk.get(), 0));
@@ -296,25 +304,28 @@ TEST(StackTest, Sorted) {
296304
// Changing the comparison function invalidates sortedness.
297305
sk_TEST_INT_set_cmp_func(sk.get(), compare_reverse);
298306
EXPECT_FALSE(sk_TEST_INT_is_sorted(sk.get()));
299-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, three.get()));
307+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, three.get()));
300308
EXPECT_EQ(2u, index);
309+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), three.get()));
301310

302311
sk_TEST_INT_sort(sk.get());
303312
ExpectStackEquals(sk.get(), {6, 5, 4, 3, 2, 1});
304-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, three.get()));
313+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, three.get()));
305314
EXPECT_EQ(3u, index);
315+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), three.get()));
306316

307317
// Inserting a new element invalidates sortedness.
308318
auto tmp = TEST_INT_new(10);
309319
ASSERT_TRUE(tmp);
310320
ASSERT_TRUE(bssl::PushToStack(sk.get(), std::move(tmp)));
311321
EXPECT_FALSE(sk_TEST_INT_is_sorted(sk.get()));
312-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, ten.get()));
322+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, ten.get()));
313323
EXPECT_EQ(6u, index);
324+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), ten.get()));
314325
} while (std::next_permutation(vec.begin(), vec.end()));
315326
}
316327

317-
// sk_*_find should return the first matching element in all cases.
328+
// sk_*_find_awslc should return the first matching element in all cases.
318329
TEST(StackTest, FindFirst) {
319330
bssl::UniquePtr<STACK_OF(TEST_INT)> sk(sk_TEST_INT_new(compare));
320331
ASSERT_TRUE(sk);
@@ -330,27 +341,31 @@ TEST(StackTest, FindFirst) {
330341
const TEST_INT *two = sk_TEST_INT_value(sk.get(), 1);
331342
// Pointer-based equality.
332343
size_t index;
333-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, two));
344+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, two));
334345
EXPECT_EQ(1u, index);
346+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), two));
335347

336348
// Comparator-based equality, unsorted.
337349
sk_TEST_INT_set_cmp_func(sk.get(), compare);
338350
EXPECT_FALSE(sk_TEST_INT_is_sorted(sk.get()));
339-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, two));
351+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, two));
340352
EXPECT_EQ(1u, index);
353+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), two));
341354

342355
// Comparator-based equality, sorted.
343356
sk_TEST_INT_sort(sk.get());
344357
EXPECT_TRUE(sk_TEST_INT_is_sorted(sk.get()));
345-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, two));
358+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, two));
346359
EXPECT_EQ(1u, index);
360+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), two));
347361

348362
// Comparator-based equality, sorted and at the front.
349363
sk_TEST_INT_set_cmp_func(sk.get(), compare_reverse);
350364
sk_TEST_INT_sort(sk.get());
351365
EXPECT_TRUE(sk_TEST_INT_is_sorted(sk.get()));
352-
ASSERT_TRUE(sk_TEST_INT_find(sk.get(), &index, two));
366+
ASSERT_TRUE(sk_TEST_INT_find_awslc(sk.get(), &index, two));
353367
EXPECT_EQ(0u, index);
368+
EXPECT_EQ((int) index, sk_TEST_INT_find(sk.get(), two));
354369
}
355370

356371
// Exhaustively test the binary search.
@@ -385,12 +400,14 @@ TEST(StackTest, BinarySearch) {
385400
ASSERT_TRUE(key);
386401

387402
size_t idx;
388-
int found = sk_TEST_INT_find(sk.get(), &idx, key.get());
403+
int found = sk_TEST_INT_find_awslc(sk.get(), &idx, key.get());
389404
if (i == j) {
390405
EXPECT_FALSE(found);
406+
EXPECT_EQ(-1, sk_TEST_INT_find(sk.get(), key.get()));
391407
} else {
392408
ASSERT_TRUE(found);
393409
EXPECT_EQ(i, idx);
410+
EXPECT_EQ((int) idx, sk_TEST_INT_find(sk.get(), key.get()));
394411
}
395412
}
396413
}

‎crypto/x509/by_dir.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
297297
if (type == X509_LU_CRL && ent->hashes) {
298298
htmp.hash = h;
299299
CRYPTO_STATIC_MUTEX_lock_read(&g_ent_hashes_lock);
300-
if (sk_BY_DIR_HASH_find(ent->hashes, &idx, &htmp)) {
300+
if (sk_BY_DIR_HASH_find_awslc(ent->hashes, &idx, &htmp)) {
301301
hent = sk_BY_DIR_HASH_value(ent->hashes, idx);
302302
k = hent->suffix;
303303
} else {
@@ -340,7 +340,7 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
340340
CRYPTO_MUTEX_lock_write(&xl->store_ctx->objs_lock);
341341
tmp = NULL;
342342
sk_X509_OBJECT_sort(xl->store_ctx->objs);
343-
if (sk_X509_OBJECT_find(xl->store_ctx->objs, &idx, &stmp)) {
343+
if (sk_X509_OBJECT_find_awslc(xl->store_ctx->objs, &idx, &stmp)) {
344344
tmp = sk_X509_OBJECT_value(xl->store_ctx->objs, idx);
345345
}
346346
CRYPTO_MUTEX_unlock_write(&xl->store_ctx->objs_lock);
@@ -354,7 +354,7 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
354354
if (!hent) {
355355
htmp.hash = h;
356356
sk_BY_DIR_HASH_sort(ent->hashes);
357-
if (sk_BY_DIR_HASH_find(ent->hashes, &idx, &htmp)) {
357+
if (sk_BY_DIR_HASH_find_awslc(ent->hashes, &idx, &htmp)) {
358358
hent = sk_BY_DIR_HASH_value(ent->hashes, idx);
359359
}
360360
}

‎crypto/x509/policy.c‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ static X509_POLICY_NODE *x509_policy_level_find(X509_POLICY_LEVEL *level,
167167
X509_POLICY_NODE node;
168168
node.policy = (ASN1_OBJECT *)policy;
169169
size_t idx;
170-
if (!sk_X509_POLICY_NODE_find(level->nodes, &idx, &node)) {
170+
if (!sk_X509_POLICY_NODE_find_awslc(level->nodes, &idx, &node)) {
171171
return NULL;
172172
}
173173
return sk_X509_POLICY_NODE_value(level->nodes, idx);
@@ -211,7 +211,7 @@ static int delete_if_not_in_policies(X509_POLICY_NODE *node, void *data) {
211211
assert(sk_POLICYINFO_is_sorted(policies));
212212
POLICYINFO info;
213213
info.policyid = node->policy;
214-
if (sk_POLICYINFO_find(policies, NULL, &info)) {
214+
if (sk_POLICYINFO_find_awslc(policies, NULL, &info)) {
215215
return 0;
216216
}
217217
x509_policy_node_free(node);
@@ -329,7 +329,7 @@ static int delete_if_mapped(X509_POLICY_NODE *node, void *data) {
329329
assert(sk_POLICY_MAPPING_is_sorted(mappings));
330330
POLICY_MAPPING mapping;
331331
mapping.issuerDomainPolicy = node->policy;
332-
if (!sk_POLICY_MAPPING_find(mappings, /*out_index=*/NULL, &mapping)) {
332+
if (!sk_POLICY_MAPPING_find_awslc(mappings, NULL, &mapping)) {
333333
return 0;
334334
}
335335
x509_policy_node_free(node);
@@ -630,8 +630,7 @@ static int has_explicit_policy(STACK_OF(X509_POLICY_LEVEL) *levels,
630630
// |node|'s parent is anyPolicy and is part of "valid_policy_node_set".
631631
// If it exists in |user_policies|, the intersection is non-empty and we
632632
// can return immediately.
633-
if (sk_ASN1_OBJECT_find(user_policies, /*out_index=*/NULL,
634-
node->policy)) {
633+
if (sk_ASN1_OBJECT_find_awslc(user_policies, NULL, node->policy)) {
635634
return 1;
636635
}
637636
} else if (i > 0) {

‎crypto/x509/x509_lu.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type,
438438

439439
size_t idx;
440440
sk_X509_OBJECT_sort(h);
441-
if (!sk_X509_OBJECT_find(h, &idx, &stmp)) {
441+
if (!sk_X509_OBJECT_find_awslc(h, &idx, &stmp)) {
442442
return -1;
443443
}
444444

@@ -561,7 +561,7 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
561561
X509_OBJECT *x) {
562562
sk_X509_OBJECT_sort(h);
563563
size_t idx;
564-
if (!sk_X509_OBJECT_find(h, &idx, x)) {
564+
if (!sk_X509_OBJECT_find_awslc(h, &idx, x)) {
565565
return NULL;
566566
}
567567
if ((x->type != X509_LU_X509) && (x->type != X509_LU_CRL)) {

‎crypto/x509/x509_trs.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ int X509_TRUST_get_by_id(int id) {
152152
if (!trtable) {
153153
return -1;
154154
}
155-
if (!sk_X509_TRUST_find(trtable, &idx, &tmp)) {
155+
if (!sk_X509_TRUST_find_awslc(trtable, &idx, &tmp)) {
156156
return -1;
157157
}
158158
return idx + X509_TRUST_COUNT;

‎crypto/x509/x_crl.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -455,7 +455,7 @@ static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, ASN1_INTEGER *serial,
455455
CRYPTO_STATIC_MUTEX_unlock_write(&g_crl_sort_lock);
456456
}
457457

458-
if (!sk_X509_REVOKED_find(crl->crl->revoked, &idx, &rtmp)) {
458+
if (!sk_X509_REVOKED_find_awslc(crl->crl->revoked, &idx, &rtmp)) {
459459
return 0;
460460
}
461461
// Need to look for matching name

‎crypto/x509v3/v3_lib.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ const X509V3_EXT_METHOD *X509V3_EXT_get_nid(int nid) {
115115
return NULL;
116116
}
117117

118-
if (!sk_X509V3_EXT_METHOD_find(ext_list, &idx, &tmp)) {
118+
if (!sk_X509V3_EXT_METHOD_find_awslc(ext_list, &idx, &tmp)) {
119119
return NULL;
120120
}
121121
return sk_X509V3_EXT_METHOD_value(ext_list, idx);

‎crypto/x509v3/v3_purp.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ int X509_PURPOSE_get_by_id(int purpose) {
200200
return -1;
201201
}
202202

203-
if (!sk_X509_PURPOSE_find(xptable, &idx, &tmp)) {
203+
if (!sk_X509_PURPOSE_find_awslc(xptable, &idx, &tmp)) {
204204
return -1;
205205
}
206206
return idx + X509_PURPOSE_COUNT;

‎crypto/x509v3/v3_utl.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,7 @@ static int append_ia5(STACK_OF(OPENSSL_STRING) **sk,
659659

660660
// Don't add duplicates
661661
sk_OPENSSL_STRING_sort(*sk);
662-
if (sk_OPENSSL_STRING_find(*sk, NULL, emtmp)) {
662+
if (sk_OPENSSL_STRING_find_awslc(*sk, NULL, emtmp)) {
663663
OPENSSL_free(emtmp);
664664
return 1;
665665
}

‎include/openssl/stack.h‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@
5757
#ifndef OPENSSL_HEADER_STACK_H
5858
#define OPENSSL_HEADER_STACK_H
5959

60+
#include <limits.h>
61+
6062
#include <openssl/base.h>
6163

6264
#include <openssl/type_check.h>
@@ -196,14 +198,17 @@ void sk_SAMPLE_delete_if(STACK_OF(SAMPLE) *sk, sk_SAMPLE_delete_if_func func,
196198
//
197199
// If the stack is sorted (see |sk_SAMPLE_sort|), this function uses a binary
198200
// search. Otherwise it performs a linear search. If it finds a matching
199-
// element, it writes the index to |*out_index| (if |out_index| is not NULL) and
200-
// returns one. Otherwise, it returns zero.
201+
// element, it returns the index of that element. Otherwise, it returns -1.
201202
//
202-
// Note this differs from OpenSSL. The type signature is slightly different, and
203-
// OpenSSL's version will implicitly sort |sk| if it has a comparison function
204-
// defined.
205-
int sk_SAMPLE_find(const STACK_OF(SAMPLE) *sk, size_t *out_index,
206-
const SAMPLE *p);
203+
// Note this differs from OpenSSL in that OpenSSL's version will implicitly
204+
// sort |sk| if it has a comparison function defined.
205+
int sk_SAMPLE_find(const STACK_OF(SAMPLE) *sk, const SAMPLE *p);
206+
207+
// sk_SAMPLE_find_awslc is like |sk_SAMPLE_find|, but if it finds a matching
208+
// element, it writes the index to |*out_index| (if |out_index| is not NULL)
209+
// and returns one. Otherwise, it returns zero.
210+
int sk_SAMPLE_find_awslc(const STACK_OF(SAMPLE) *sk, size_t *out_index,
211+
const SAMPLE *p);
207212

208213
// sk_SAMPLE_shift removes and returns the first element in |sk|, or NULL if
209214
// |sk| is empty.
@@ -492,12 +497,26 @@ BSSL_NAMESPACE_END
492497
(OPENSSL_sk_delete_if_func)func, data); \
493498
} \
494499
\
495-
OPENSSL_INLINE int sk_##name##_find(const STACK_OF(name) *sk, \
500+
/* use 3-arg sk_*_find_awslc when size_t-sized |out_index| needed */ \
501+
OPENSSL_INLINE int sk_##name##_find_awslc(const STACK_OF(name) *sk, \
496502
size_t *out_index, constptrtype p) { \
497503
return OPENSSL_sk_find((const OPENSSL_STACK *)sk, out_index, \
498504
(const void *)p, sk_##name##_call_cmp_func); \
499505
} \
500506
\
507+
/* use 2-arg sk_*_find for OpenSSL compatibility */ \
508+
OPENSSL_INLINE int sk_##name##_find(const STACK_OF(name) *sk, \
509+
constptrtype p) { \
510+
size_t out_index = 0; \
511+
int ok = OPENSSL_sk_find((const OPENSSL_STACK *)sk, &out_index, \
512+
(const void *)p, sk_##name##_call_cmp_func); \
513+
/* return -1 if elt not found or elt index is invalid */ \
514+
if (ok == 0 || out_index > INT_MAX) { \
515+
return -1; \
516+
} \
517+
return (int) out_index; \
518+
} \
519+
\
501520
OPENSSL_INLINE ptrtype sk_##name##_shift(STACK_OF(name) *sk) { \
502521
return (ptrtype)OPENSSL_sk_shift((OPENSSL_STACK *)sk); \
503522
} \

0 commit comments

Comments
 (0)