[blktap2] fix two 'maybe uninitialized' variables

[blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli]

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) {
       ^
cc1: all warnings being treated as errors
/home/dario/Sources/xen/xen/xen.git/tools/blktap2/drivers/../../../tools/Rules.mk:89: recipe for target 'block-qcow.o' failed
make[5]: *** [block-qcow.o] Error 1

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
diff --git a/tools/blktap2/drivers/block-qcow.c b/tools/blktap2/drivers/block-qcow.c
index d5053d4..c677bc9 100644
--- a/tools/blktap2/drivers/block-qcow.c
+++ b/tools/blktap2/drivers/block-qcow.c
@@ -383,8 +383,8 @@ static uint64_t get_cluster_offset(struct tdqcow_state *s,
                                    int n_start, int n_end)
 {
 	int min_index, i, j, l1_index, l2_index, l2_sector, l1_sector;
-	char *tmp_ptr2, *l2_ptr, *l1_ptr;
-	uint64_t *tmp_ptr;
+	char *tmp_ptr2 = NULL, *l2_ptr, *l1_ptr;
+	uint64_t *tmp_ptr = NULL;
 	uint64_t l2_offset, *l2_table, cluster_offset, tmp;
 	uint32_t min_count;
 	int new_l2_table;

-- 
<<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)



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

Re: [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1939 bytes --]

Mmm... a better subject line for this would have been:

  [PATCH] blktap2: fix two 'maybe uninitialized' variables


Sorry! :-/ Let me know it you want me to resent like that.

Regards,
Dario

On mer, 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) {
>        ^
> cc1: all warnings being treated as errors
> /home/dario/Sources/xen/xen/xen.git/tools/blktap2/drivers/../../../tools/Rules.mk:89: recipe for target 'block-qcow.o' failed
> make[5]: *** [block-qcow.o] Error 1
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> diff --git a/tools/blktap2/drivers/block-qcow.c b/tools/blktap2/drivers/block-qcow.c
> index d5053d4..c677bc9 100644
> --- a/tools/blktap2/drivers/block-qcow.c
> +++ b/tools/blktap2/drivers/block-qcow.c
> @@ -383,8 +383,8 @@ static uint64_t get_cluster_offset(struct tdqcow_state *s,
>                                     int n_start, int n_end)
>  {
>  	int min_index, i, j, l1_index, l2_index, l2_sector, l1_sector;
> -	char *tmp_ptr2, *l2_ptr, *l1_ptr;
> -	uint64_t *tmp_ptr;
> +	char *tmp_ptr2 = NULL, *l2_ptr, *l1_ptr;
> +	uint64_t *tmp_ptr = NULL;
>  	uint64_t l2_offset, *l2_table, cluster_offset, tmp;
>  	uint32_t min_count;
>  	int new_l2_table;
> 

-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [blktap2] fix two 'maybe uninitialized' variables

[Ian Campbell]

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.

For the preferred $subject I always just look in the git log for the
appropriate file/directory.

Ian.


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

Re: [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1619 bytes --]

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.
> 
Your are right, sorry for this. Being super-unfamiliar with that code, I
was sort of relying on the fact that it is correct, especially
considering the nature of the warning message.

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.

Thanks and Regards,
Dario

-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3850 bytes --]

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli]

[Adding Thanos (I hope I used the right email address) and Dave (or
should I add someone else, from the XS team perhaps?)

On Thu, Jun 12, 2014 at 1:40 PM, Dario Faggioli <raistlin.df@gmail.com> wrote:
> 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. :-/
>
Any ideas? It's all but urgent, I guess, but there looks to be people
starting to hit this (yeah, well, only 1 guy for now, but still... :-)
)

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)

Re: [blktap2] fix two 'maybe uninitialized' variables

[Ian Campbell]

On Thu, 2014-06-12 at 13:40 +0200, Dario Faggioli wrote:

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

There is no such person I'm afraid and hasn't been for some years.

In the absence of a blktap2 maintainer and/or the absence of any voices
to the contrary I'm inclined to take this, it seems to return the same
error code as other failures in this function.

Please resubmit without the spacespace damage and with a commit log and
S-o-b.

> 
> 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);
> 

Re: [blktap2] fix two 'maybe uninitialized' variables

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1010 bytes --]

On mer, 2014-06-18 at 15:02 +0100, Ian Campbell wrote:
> On Thu, 2014-06-12 at 13:40 +0200, Dario Faggioli wrote:
> 
> > So, I'd have to ask to someone who's more into that code to advise, I'm
> > afraid. :-/
> 
> There is no such person I'm afraid and hasn't been for some years.
> 
:-(

> In the absence of a blktap2 maintainer and/or the absence of any voices
> to the contrary I'm inclined to take this, it seems to return the same
> error code as other failures in this function.
> 
> Please resubmit without the spacespace damage and with a commit log and
> S-o-b.
> 
Done:
  http://lists.xen.org/archives/html/xen-devel/2014-06/msg02790.html
  Message-ID: <20140620140854.19823.79938.stgit@Solace>


Regards,
Dario

-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [blktap2] fix two 'maybe uninitialized' variables

[Ian Jackson]

Dario Faggioli writes ("Re: [Xen-devel] [blktap2] fix two 'maybe uninitialized' variables"):
> On mer, 2014-06-18 at 15:02 +0100, Ian Campbell wrote:
> > On Thu, 2014-06-12 at 13:40 +0200, Dario Faggioli wrote:
> > 
> > > So, I'd have to ask to someone who's more into that code to advise, I'm
> > > afraid. :-/
> > 
> > There is no such person I'm afraid and hasn't been for some years.
> > 
> :-(
> 
> > In the absence of a blktap2 maintainer and/or the absence of any voices
> > to the contrary I'm inclined to take this, it seems to return the same
> > error code as other failures in this function.
> > 
> > Please resubmit without the spacespace damage and with a commit log and
> > S-o-b.
> > 
> Done:
>   http://lists.xen.org/archives/html/xen-devel/2014-06/msg02790.html
>   Message-ID: <20140620140854.19823.79938.stgit@Solace>

Applied, thanks.

Ian.

Re: [blktap2] fix two 'maybe uninitialized' variables

[Ian Jackson]

Dario Faggioli writes ("Re: [Xen-devel] [blktap2] fix two 'maybe uninitialized' variables"):
> On mer, 2014-06-18 at 15:02 +0100, Ian Campbell wrote:
> > Please resubmit without the spacespace damage and with a commit log and
> > S-o-b.
> > 
> Done:
>   http://lists.xen.org/archives/html/xen-devel/2014-06/msg02790.html
>   Message-ID: <20140620140854.19823.79938.stgit@Solace>

I have added this to my backport list.

Ian.

Re: [blktap2] fix two 'maybe uninitialized' variables

[Ian Jackson]

Ian Jackson writes ("Re: [Xen-devel] [blktap2] fix two 'maybe uninitialized' variables"):
> Dario Faggioli writes ("Re: [Xen-devel] [blktap2] fix two 'maybe uninitialized' variables"):
> > On mer, 2014-06-18 at 15:02 +0100, Ian Campbell wrote:
> > > Please resubmit without the spacespace damage and with a commit log and
> > > S-o-b.
> > > 
> > Done:
> >   http://lists.xen.org/archives/html/xen-devel/2014-06/msg02790.html
> >   Message-ID: <20140620140854.19823.79938.stgit@Solace>
> 
> I have added this to my backport list.

Backported to 4.4, 4.3, 4.2.

Ian.