Change sk_*_find signature to 2-arg for OpenSSL comapat #1429
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1429 +/- ##
==========================================
- Coverage 76.88% 76.83% -0.06%
==========================================
Files 425 425
Lines 71527 71526 -1
==========================================
- Hits 54991 54954 -37
- Misses 16536 16572 +36 ☔ View full report in Codecov by Sentry. |
WillChilds-Klein
force-pushed
the
sk_find
branch
4 times, most recently
from
c4e5e31 to
5f1e186
Compare
include/openssl/stack.h
Outdated
| /* use 2-arg sk_*_find for OpenSSL compatibility */ \ | ||
| OPENSSL_INLINE int sk_##name##_find(const STACK_OF(name) *sk, \ | ||
| constptrtype p) { \ | ||
| const size_t mask = sizeof(size_t) > sizeof(int) \ |
There was a problem hiding this comment.
The mask should also cover the sign bit, even if sizeof(size_t) == sizeof(int):
const size_t mask = (~((size_t) 0)) << (sizeof(int) * 8 - 1);
I don't think it's the case for any supported platform that
sizeof(size_t) < sizeof(int)
There was a problem hiding this comment.
The mask should also cover the sign bit, even if
sizeof(size_t) == sizeof(int)
size_t is always unsigned, no?
I don't think it's the case for any supported platform that
sizeof(size_t) < sizeof(int)
I didn't think so either, but am concerned with the sizeof(size_t) == sizeof(int) case, hence the hard inequality. Initially I'd written this as sizeof(size_t) != sizeof(int), used < to guard against cases I hadn't considered.
In any case, something is wrong with my implementation here. The ubuntu1604_gcc5x_x86 test is failing with a compile error:
../include/openssl/stack.h:509:46: error: left shift count >= width of type [-Werror=shift-count-overflow]
? (~((size_t) 0)) << (sizeof(int) * 8) \
^
Same with centos7_gcc4x_x86_64_fips. I'll look into how earlier versions of GCC handle bit shifting.
There was a problem hiding this comment.
I ended up dropping the masking logic altogether in favor of checking INT_MAX and casting.
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
WillChilds-Klein
force-pushed
the
sk_find
branch
from
8654bd2 to
cf4cc69
Compare
| int ok = OPENSSL_sk_find((const OPENSSL_STACK *)sk, &out_index, \ | ||
| (const void *)p, sk_##name##_call_cmp_func); \ |
There was a problem hiding this comment.
Nit: Can this call sk_##name##_find_awslc? No strong preference though
There was a problem hiding this comment.
that would work, but IMO we should put any checks, etc. we want shared by both sk_*_find and sk_*_find_awslc in the concrete function OPENSSL_sk_find rather than one of the wrapping macros.
Description of changes:
OpenSSL omits 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|.Call-outs:
|sk_*_find|function. Consuming code will need to be updated to either use the new|sk_*_find|function or to drop the second positional|out_index|parameter from their function call(s).Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.