| Message ID | 1463735394-25690-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/20/2016 03:09 AM, Paolo Bonzini wrote: > Otherwise unintended results could happen. For example, > Coverity reports a division by zero in qcrypto_afsplit_hash. > While this cannot really happen, it shows that the contract > of qcrypto_hash_digest_len can be improved. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > crypto/hash.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/crypto/hash.c b/crypto/hash.c > index b90af34..2907bff 100644 > --- a/crypto/hash.c > +++ b/crypto/hash.c > @@ -36,9 +36,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = { > > size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg) > { > - if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_size)) { > - return 0; > - } > + assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size)); > return qcrypto_hash_alg_size[alg]; The assertion doesn't protect us if QCryptoHashAlgorithm gains another member but we forget to update qcrypto_hash_alg_size[] to match. Do you want an additional assertion that you are returning a non-zero value?
On Fri, May 20, 2016 at 09:52:36AM -0600, Eric Blake wrote: > On 05/20/2016 03:09 AM, Paolo Bonzini wrote: > > Otherwise unintended results could happen. For example, > > Coverity reports a division by zero in qcrypto_afsplit_hash. > > While this cannot really happen, it shows that the contract > > of qcrypto_hash_digest_len can be improved. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > crypto/hash.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks, will queue this in my crypto-next branch > > diff --git a/crypto/hash.c b/crypto/hash.c > > index b90af34..2907bff 100644 > > --- a/crypto/hash.c > > +++ b/crypto/hash.c > > @@ -36,9 +36,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = { > > > > size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg) > > { > > - if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_size)) { > > - return 0; > > - } > > + assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size)); > > return qcrypto_hash_alg_size[alg]; > > The assertion doesn't protect us if QCryptoHashAlgorithm gains another > member but we forget to update qcrypto_hash_alg_size[] to match. Do you > want an additional assertion that you are returning a non-zero value? It will gain more entries with a patch I have pending, but at the same time the impl of the hash methods will change, so this code won't exist in its current form. So I'll just queue this patch as is. Regards, Daniel
diff --git a/crypto/hash.c b/crypto/hash.c index b90af34..2907bff 100644 --- a/crypto/hash.c +++ b/crypto/hash.c @@ -36,9 +36,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = { size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg) { - if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_size)) { - return 0; - } + assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size)); return qcrypto_hash_alg_size[alg]; }
Otherwise unintended results could happen. For example, Coverity reports a division by zero in qcrypto_afsplit_hash. While this cannot really happen, it shows that the contract of qcrypto_hash_digest_len can be improved. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- crypto/hash.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)