diff options
author | Andy Polyakov <appro@openssl.org> | 2017-04-15 15:53:50 +0200 |
---|---|---|
committer | Andy Polyakov <appro@openssl.org> | 2017-04-17 21:10:27 +0200 |
commit | 786b6a45fbecc068d0fb8b05252a9228e0661c63 (patch) | |
tree | 80fca98d045718ac14b21e03648613390e405b61 /crypto/asn1 | |
parent | 5c8e9d531ba54d26e4bcbe66710c7c75bf0fc4e9 (diff) |
asn1/a_int.c: clean up asn1_get_int64.
Trouble was that integer negation wasn't producing *formally* correct
result in platform-neutral sense. Formally correct thing to do is
-(int64_t)u, but this triggers undefined behaviour for one value that
would still be representable in ASN.1. The trigger was masked with
(int64_t)(0-u), but this is formally inappropriate for values other
than the problematic one. [Also reorder branches to favour most-likely
paths and harmonize asn1_string_set_int64 with asn1_get_int64].]
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3231)
Diffstat (limited to 'crypto/asn1')
-rw-r--r-- | crypto/asn1/a_int.c | 29 |
1 files changed, 20 insertions, 9 deletions
diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c index fe700b22f7..e154343925 100644 --- a/crypto/asn1/a_int.c +++ b/crypto/asn1/a_int.c @@ -229,12 +229,10 @@ static size_t asn1_put_uint64(unsigned char b[sizeof(uint64_t)], uint64_t r) } /* - * Absolute value of INT64_MIN: we can't just use -INT64_MIN as it produces + * Absolute value of INT64_MIN: we can't just use -INT64_MIN as gcc produces * overflow warnings. */ - -#define ABS_INT64_MIN \ - ((uint64_t)INT64_MAX + (uint64_t)(-(INT64_MIN + INT64_MAX))) +#define ABS_INT64_MIN ((uint64_t)INT64_MAX + (-(INT64_MIN + INT64_MAX))) /* signed version of asn1_get_uint64 */ static int asn1_get_int64(int64_t *pr, const unsigned char *b, size_t blen, @@ -244,17 +242,25 @@ static int asn1_get_int64(int64_t *pr, const unsigned char *b, size_t blen, if (asn1_get_uint64(&r, b, blen) == 0) return 0; if (neg) { - if (r > ABS_INT64_MIN) { + if (r <= INT64_MAX) { + /* Most significant bit is guaranteed to be clear, negation + * is guaranteed to be meaningful in platform-neutral sense. */ + *pr = -(int64_t)r; + } else if (r == ABS_INT64_MIN) { + /* This never happens if INT64_MAX == ABS_INT64_MIN, e.g. + * on ones'-complement system. */ + *pr = (int64_t)(0 - r); + } else { ASN1err(ASN1_F_ASN1_GET_INT64, ASN1_R_TOO_SMALL); return 0; } - *pr = 0 - (uint64_t)r; } else { - if (r > INT64_MAX) { + if (r <= INT64_MAX) { + *pr = (int64_t)r; + } else { ASN1err(ASN1_F_ASN1_GET_INT64, ASN1_R_TOO_LARGE); return 0; } - *pr = (int64_t)r; } return 1; } @@ -319,7 +325,12 @@ static int asn1_string_set_int64(ASN1_STRING *a, int64_t r, int itype) a->type = itype; if (r < 0) { - off = asn1_put_uint64(tbuf, -r); + /* Most obvious '-r' triggers undefined behaviour for most + * common INT64_MIN. Even though below '0 - (uint64_t)r' can + * appear two's-complement centric, it does produce correct/ + * expected result even on one's-complement. This is because + * cast to unsigned has to change bit pattern... */ + off = asn1_put_uint64(tbuf, 0 - (uint64_t)r); a->type |= V_ASN1_NEG; } else { off = asn1_put_uint64(tbuf, r); |