From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3nYf-0006EG-KB for qemu-devel@nongnu.org; Fri, 20 May 2016 12:45:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b3nYb-0006mb-Cx for qemu-devel@nongnu.org; Fri, 20 May 2016 12:45:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33217) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3nYb-0006m8-79 for qemu-devel@nongnu.org; Fri, 20 May 2016 12:45:49 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 04D558E39E for ; Fri, 20 May 2016 16:45:48 +0000 (UTC) Date: Fri, 20 May 2016 17:45:44 +0100 From: "Daniel P. Berrange" Message-ID: <20160520164544.GK699@redhat.com> Reply-To: "Daniel P. Berrange" References: <1463735394-25690-1-git-send-email-pbonzini@redhat.com> <573F32C4.5030303@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <573F32C4.5030303@redhat.com> Subject: Re: [Qemu-devel] [PATCH] crypto: assert that qcrypto_hash_digest_len is in range List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Paolo Bonzini , qemu-devel@nongnu.org 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 > > --- > > crypto/hash.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > Reviewed-by: Eric Blake 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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|