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