Re: [Xen-devel] [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli <raistlin.df@xxxxxxxxx>]

On gio, 2014-06-12 at 11:30 +0200, Dario Faggioli wrote:
> On gio, 2014-06-12 at 10:18 +0100, Ian Campbell wrote:
> > On Wed, 2014-06-11 at 14:01 +0200, Dario Faggioli wrote:
> > > for which gcc 4.9.0 complains about, like this:
> > > 
> > > block-qcow.c: In function âget_cluster_offsetâ:
> > > block-qcow.c:431:3: error: âtmp_ptrâ may be used uninitialized in this 
> > > function [-Werror=maybe-uninitialized]
> > >    memcpy(tmp_ptr, l1_ptr, 4096);
> > >    ^
> > > block-qcow.c:606:7: error: âtmp_ptr2â may be used uninitialized in this 
> > > function [-Werror=maybe-uninitialized]
> > >    if (write(s->fd, tmp_ptr2, 4096) != 4096) {
> > 
> > You initialise both of these to NULL as they are defined, but the
> > compiler has apparently found a path where these values can be used
> > without subsequently being initialised, so you are passing NULL to
> > memcpy/write, which can't be good.
> > 
> > If you've proved that the compiler is wrong/confused and this cannot
> > happen please explain the how/why it is wrong here.
> > 

> However, I understand, and actually agree, that it really is a bad
> practice to suppress warnings like this... Let me have a deeper look and
> see if I can propose a better fix.
> 
So, I did try, and could came up with the patch below.

The thing I'm unsure about is whether I should `return 0' in both the
cases (I'm quite sure about the first, less about the second). I think
that is ok, and that's by looking at the code of the function itself, at
it's doc comment ("return 0 if not allocated"), and at the various call
sites.

Fact is the function does not look that consistent to me, as far as
error (if they're error!) reporting is concerned. In fact, the return
type is uint64_t, and there are places where it returns 0, places where
it returns -1 and, finally, one place when it returns cluster_offset (at
the end).

As said above, looking at the doc comment and at the various callers, 0
seems to be the answer, for instance because all the callers only check
whether the function returns 0 or !0, without any further investigation
of the !0 case... However, if the returning of -1 from an uint64_t
function is done on purpose --e.g., to let the caller know that the
allocation did happen, but using (uint64_y)-1 as a signal of "but, hey,
something went wrong!"-- I really can't tell. It really does not look
like so (and that's why I'm going for 'return 0' in both cases), but I
know too few about blocktap, qcow2, etc, to be positive about it. :-(

So, I'd have to ask to someone who's more into that code to advise, I'm
afraid. :-/

Thanks and Regards,
Dario

---
diff --git a/tools/blktap2/drivers/block-qcow.c 
b/tools/blktap2/drivers/block-qcow.c
index d5053d4..b45bcaa 100644
--- a/tools/blktap2/drivers/block-qcow.c
+++ b/tools/blktap2/drivers/block-qcow.c
@@ -427,6 +427,7 @@ static uint64_t get_cluster_offset(struct tdqcow_state *s,
 
                if (posix_memalign((void **)&tmp_ptr, 4096, 4096) != 0) {
                        DPRINTF("ERROR allocating memory for L1 table\n");
+                        return 0;
                }
                memcpy(tmp_ptr, l1_ptr, 4096);
 
@@ -600,6 +601,7 @@ found:
                
                if (posix_memalign((void **)&tmp_ptr2, 4096, 4096) != 0) {
                        DPRINTF("ERROR allocating memory for L1 table\n");
+                        return 0;
                }
                memcpy(tmp_ptr2, l2_ptr, 4096);
                lseek(s->fd, l2_offset + (l2_sector << 12), SEEK_SET);

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel