From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo71m-0001Od-Bw for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:42:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xo71f-0003mi-OK for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:42:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48200) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo71f-0003mJ-Fp for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:42:11 -0500 Message-ID: <5461CBDB.3010109@redhat.com> Date: Tue, 11 Nov 2014 09:42:03 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-9-git-send-email-mreitz@redhat.com> <54615850.4050809@redhat.com> In-Reply-To: <54615850.4050809@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/21] qcow2: More helpers for refcount modification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi On 2014-11-11 at 01:29, Eric Blake wrote: > On 11/10/2014 06:45 AM, Max Reitz wrote: >> Add helper functions for getting and setting refcounts in a refcount >> array for any possible refcount order, and choose the correct one during >> refcount initialization. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-refcount.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 141 insertions(+), 2 deletions(-) >> >> + >> +static uint64_t get_refcount_ro6(const void *refcount_array, uint64_t index) >> +{ >> + return be64_to_cpu(((const uint64_t *)refcount_array)[index]); >> +} > Should this return int64_t and error out if the user ever exceeded 2**63 > via image fuzzing? I don't know. It's nice that these helper functions cannot return an error and thus we don't have to check for a error. I think checking that the value didn't overflow in qcow2_get_refcount() should be sufficient and relieves the other callers (mainly the image check for its in-memory refcount table/array) which know that the value cannot overflow from error checking. Although I did forget an overflow check after the call to get_refcount() in update_refcount_discard(). >> + >> +static void set_refcount_ro6(void *refcount_array, uint64_t index, >> + uint64_t value) >> +{ >> + ((uint64_t *)refcount_array)[index] = cpu_to_be64(value); >> +} > Should this assert that value <= INT64_MAX, since that's what the rest > of the code caps it to? Yes, that it should most certainly do. Max