diff mbox series

[06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

Message ID 20210212153953.4582-7-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series tools: Support to use abi-dumper on libraries | expand

Commit Message

Andrew Cooper Feb. 12, 2021, 3:39 p.m. UTC
Various version of gcc, when compiling with -Og, complain:

  xenstored_control.c: In function ‘lu_read_state’:
  xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
  function [-Werror=uninitialized]
    if (state.size == 0)
        ~~~~~^~~~~
  xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
    pre = state.buf;
    ~~~~^~~~~~~~~~~
  xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
     (void *)head - state.buf < state.size;
                    ~~~~~^~~~
  xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
     (void *)head - state.buf < state.size;
                                ~~~~~^~~~~

Interestingly, this is only in the stubdom build.  I can't identify any
relevant differences vs the regular tools build.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_control.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Juergen Gross Feb. 12, 2021, 4:08 p.m. UTC | #1
On 12.02.21 16:39, Andrew Cooper wrote:
> Various version of gcc, when compiling with -Og, complain:
> 
>    xenstored_control.c: In function ‘lu_read_state’:
>    xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
>    function [-Werror=uninitialized]
>      if (state.size == 0)
>          ~~~~~^~~~~
>    xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
>    this function [-Werror=maybe-uninitialized]
>      pre = state.buf;
>      ~~~~^~~~~~~~~~~
>    xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
>    this function [-Werror=maybe-uninitialized]
>       (void *)head - state.buf < state.size;
>                      ~~~~~^~~~
>    xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
>    this function [-Werror=maybe-uninitialized]
>       (void *)head - state.buf < state.size;
>                                  ~~~~~^~~~~
> 
> Interestingly, this is only in the stubdom build.  I can't identify any
> relevant differences vs the regular tools build.

But me. :-)

lu_get_dump_state() is empty for the stubdom case (this will change when
LU is implemented for stubdom, too). In the daemon case this function is
setting all the fields which are relevant.

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Andrew Cooper Feb. 12, 2021, 5:01 p.m. UTC | #2
On 12/02/2021 16:08, Jürgen Groß wrote:
> On 12.02.21 16:39, Andrew Cooper wrote:
>> Various version of gcc, when compiling with -Og, complain:
>>
>>    xenstored_control.c: In function ‘lu_read_state’:
>>    xenstored_control.c:540:11: error: ‘state.size’ is used
>> uninitialized in this
>>    function [-Werror=uninitialized]
>>      if (state.size == 0)
>>          ~~~~~^~~~~
>>    xenstored_control.c:543:6: error: ‘state.buf’ may be used
>> uninitialized in
>>    this function [-Werror=maybe-uninitialized]
>>      pre = state.buf;
>>      ~~~~^~~~~~~~~~~
>>    xenstored_control.c:550:23: error: ‘state.buf’ may be used
>> uninitialized in
>>    this function [-Werror=maybe-uninitialized]
>>       (void *)head - state.buf < state.size;
>>                      ~~~~~^~~~
>>    xenstored_control.c:550:35: error: ‘state.size’ may be used
>> uninitialized in
>>    this function [-Werror=maybe-uninitialized]
>>       (void *)head - state.buf < state.size;
>>                                  ~~~~~^~~~~
>>
>> Interestingly, this is only in the stubdom build.  I can't identify any
>> relevant differences vs the regular tools build.
>
> But me. :-)
>
> lu_get_dump_state() is empty for the stubdom case (this will change when
> LU is implemented for stubdom, too). In the daemon case this function is
> setting all the fields which are relevant.

So I spotted that.  This instance of lu_read_state() is already within
the ifdefary, so doesn't get to see the empty stub (I think).

Also, I'd expect the compiler to complain at -O2 if it spotted that code.

>
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks,

~Andrew
Juergen Gross Feb. 13, 2021, 9:36 a.m. UTC | #3
On 12.02.21 18:01, Andrew Cooper wrote:
> On 12/02/2021 16:08, Jürgen Groß wrote:
>> On 12.02.21 16:39, Andrew Cooper wrote:
>>> Various version of gcc, when compiling with -Og, complain:
>>>
>>>     xenstored_control.c: In function ‘lu_read_state’:
>>>     xenstored_control.c:540:11: error: ‘state.size’ is used
>>> uninitialized in this
>>>     function [-Werror=uninitialized]
>>>       if (state.size == 0)
>>>           ~~~~~^~~~~
>>>     xenstored_control.c:543:6: error: ‘state.buf’ may be used
>>> uninitialized in
>>>     this function [-Werror=maybe-uninitialized]
>>>       pre = state.buf;
>>>       ~~~~^~~~~~~~~~~
>>>     xenstored_control.c:550:23: error: ‘state.buf’ may be used
>>> uninitialized in
>>>     this function [-Werror=maybe-uninitialized]
>>>        (void *)head - state.buf < state.size;
>>>                       ~~~~~^~~~
>>>     xenstored_control.c:550:35: error: ‘state.size’ may be used
>>> uninitialized in
>>>     this function [-Werror=maybe-uninitialized]
>>>        (void *)head - state.buf < state.size;
>>>                                   ~~~~~^~~~~
>>>
>>> Interestingly, this is only in the stubdom build.  I can't identify any
>>> relevant differences vs the regular tools build.
>>
>> But me. :-)
>>
>> lu_get_dump_state() is empty for the stubdom case (this will change when
>> LU is implemented for stubdom, too). In the daemon case this function is
>> setting all the fields which are relevant.
> 
> So I spotted that.  This instance of lu_read_state() is already within
> the ifdefary, so doesn't get to see the empty stub (I think).

There is only one instance of lu_read_state().


Juergen
Andrew Cooper Feb. 15, 2021, 2:12 p.m. UTC | #4
On 13/02/2021 09:36, Jürgen Groß wrote:
> On 12.02.21 18:01, Andrew Cooper wrote:
>> On 12/02/2021 16:08, Jürgen Groß wrote:
>>> On 12.02.21 16:39, Andrew Cooper wrote:
>>>> Various version of gcc, when compiling with -Og, complain:
>>>>
>>>>     xenstored_control.c: In function ‘lu_read_state’:
>>>>     xenstored_control.c:540:11: error: ‘state.size’ is used
>>>> uninitialized in this
>>>>     function [-Werror=uninitialized]
>>>>       if (state.size == 0)
>>>>           ~~~~~^~~~~
>>>>     xenstored_control.c:543:6: error: ‘state.buf’ may be used
>>>> uninitialized in
>>>>     this function [-Werror=maybe-uninitialized]
>>>>       pre = state.buf;
>>>>       ~~~~^~~~~~~~~~~
>>>>     xenstored_control.c:550:23: error: ‘state.buf’ may be used
>>>> uninitialized in
>>>>     this function [-Werror=maybe-uninitialized]
>>>>        (void *)head - state.buf < state.size;
>>>>                       ~~~~~^~~~
>>>>     xenstored_control.c:550:35: error: ‘state.size’ may be used
>>>> uninitialized in
>>>>     this function [-Werror=maybe-uninitialized]
>>>>        (void *)head - state.buf < state.size;
>>>>                                   ~~~~~^~~~~
>>>>
>>>> Interestingly, this is only in the stubdom build.  I can't identify
>>>> any
>>>> relevant differences vs the regular tools build.
>>>
>>> But me. :-)
>>>
>>> lu_get_dump_state() is empty for the stubdom case (this will change
>>> when
>>> LU is implemented for stubdom, too). In the daemon case this
>>> function is
>>> setting all the fields which are relevant.
>>
>> So I spotted that.  This instance of lu_read_state() is already within
>> the ifdefary, so doesn't get to see the empty stub (I think).
>
> There is only one instance of lu_read_state().

Ahh - I got the NO_LIVE_UPDATE and __MINIOS__ ifdefary inverted.  (It is
very confusing to follow).

I'll reword the commit message, but leave the fix the same.

~Andrew
Ian Jackson Feb. 16, 2021, 4:10 p.m. UTC | #5
Andrew Cooper writes ("[PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   xenstored_control.c: In function ‘lu_read_state’:
>   xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
>   function [-Werror=uninitialized]
>     if (state.size == 0)
>         ~~~~~^~~~~
>   xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
>   this function [-Werror=maybe-uninitialized]
>     pre = state.buf;
>     ~~~~^~~~~~~~~~~
>   xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
>   this function [-Werror=maybe-uninitialized]
>      (void *)head - state.buf < state.size;
>                     ~~~~~^~~~
>   xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
>   this function [-Werror=maybe-uninitialized]
>      (void *)head - state.buf < state.size;
>                                 ~~~~~^~~~~
> 
> Interestingly, this is only in the stubdom build.  I can't identify any
> relevant differences vs the regular tools build.

  #ifdef __MINIOS__
  static void lu_get_dump_state(struct lu_dump_state *state)
  {
  }

So the compiler is right to complain that

  lu_get_dump_state(&state);
  if (state.size == 0)
          barf_perror("No state found after live-update");

this will use state.size uninitialised.

It's probably just luck that this works at all, if it does,
anywhere...

Reviewed-by: Ian Jackson <iwj@xenproject.org>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Ian.
diff mbox series

Patch

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 1f733e0a04..f10beaf85e 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -530,7 +530,7 @@  static const char *lu_dump_state(const void *ctx, struct connection *conn)
 
 void lu_read_state(void)
 {
-	struct lu_dump_state state;
+	struct lu_dump_state state = {};
 	struct xs_state_record_header *head;
 	void *ctx = talloc_new(NULL); /* Work context for subfunctions. */
 	struct xs_state_preamble *pre;