| Message ID | 20210225174131.10115-6-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xenstore: Address coverity issues in the LiveUpdate code | expand |
On 25/02/2021 17:41, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Coverity will report unitialized values for every use of xs_state_* > structures in the save part. This can be prevented by using the [0] > rather than [] to define variable length array. > > Coverity-ID: 1472398 > Coverity-ID: 1472397 > Coverity-ID: 1472396 > Coverity-ID: 1472395 > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > > From my understanding, the tools and the hypervisor already rely on GNU > extensions. So the change should be fine. > > If not, we can use the same approach as XEN_FLEX_ARRAY_DIM. Linux has recently purged the use of [0] because it causes sizeof() to do unsafe things. Flexible array members is a C99 standard - are we sure that Coverity is doing something wrong with them? ~Andrew
Hi Andrew, On 25/02/2021 17:47, Andrew Cooper wrote: > On 25/02/2021 17:41, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Coverity will report unitialized values for every use of xs_state_* >> structures in the save part. This can be prevented by using the [0] >> rather than [] to define variable length array. >> >> Coverity-ID: 1472398 >> Coverity-ID: 1472397 >> Coverity-ID: 1472396 >> Coverity-ID: 1472395 >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> --- >> >> From my understanding, the tools and the hypervisor already rely on GNU >> extensions. So the change should be fine. >> >> If not, we can use the same approach as XEN_FLEX_ARRAY_DIM. > > Linux has recently purged the use of [0] because it causes sizeof() to > do unsafe things. Do you have a link to the Linux thread? > > Flexible array members is a C99 standard - are we sure that Coverity is > doing something wrong with them? I have run coverity with one of the structure switched to [0] and it removed the unitialized warning for that specific one. So clearly coverity is not happy with []. Although, I am not sure why. Do you have a suggestion how to approach the problem? Cheers,
On 25.02.21 18:41, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Coverity will report unitialized values for every use of xs_state_* > structures in the save part. This can be prevented by using the [0] > rather than [] to define variable length array. > > Coverity-ID: 1472398 > Coverity-ID: 1472397 > Coverity-ID: 1472396 > Coverity-ID: 1472395 > Signed-off-by: Julien Grall <jgrall@amazon.com> Sorry, but Coverity is clearly wrong here. Should we really modify our code to work around bugs in external static code analyzers? Juergen > > --- > > From my understanding, the tools and the hypervisor already rely on GNU > extensions. So the change should be fine. > > If not, we can use the same approach as XEN_FLEX_ARRAY_DIM. > --- > tools/xenstore/include/xenstore_state.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/xenstore/include/xenstore_state.h b/tools/xenstore/include/xenstore_state.h > index ae0d053c8ffc..407d9e920c0f 100644 > --- a/tools/xenstore/include/xenstore_state.h > +++ b/tools/xenstore/include/xenstore_state.h > @@ -86,7 +86,7 @@ struct xs_state_connection { > uint16_t data_in_len; /* Number of unprocessed bytes read from conn. */ > uint16_t data_resp_len; /* Size of partial response pending for conn. */ > uint32_t data_out_len; /* Number of bytes not yet written to conn. */ > - uint8_t data[]; /* Pending data (read, written) + 0-7 pad bytes. */ > + uint8_t data[0]; /* Pending data (read, written) + 0-7 pad bytes. */ > }; > > /* Watch: */ > @@ -94,7 +94,7 @@ struct xs_state_watch { > uint32_t conn_id; /* Connection this watch is associated with. */ > uint16_t path_length; /* Number of bytes of path watched (incl. 0). */ > uint16_t token_length; /* Number of bytes of watch token (incl. 0). */ > - uint8_t data[]; /* Path bytes, token bytes, 0-7 pad bytes. */ > + uint8_t data[0]; /* Path bytes, token bytes, 0-7 pad bytes. */ > }; > > /* Transaction: */ > @@ -125,7 +125,7 @@ struct xs_state_node { > #define XS_STATE_NODE_TA_WRITTEN 0x0002 > uint16_t perm_n; /* Number of permissions (0 in TA: node deleted). */ > /* Permissions (first is owner, has full access). */ > - struct xs_state_node_perm perms[]; > + struct xs_state_node_perm perms[0]; > /* Path and data follows, plus 0-7 pad bytes. */ > }; > #endif /* XENSTORE_STATE_H */ >
Hi Juergen, On 26/02/2021 07:10, Jürgen Groß wrote: > On 25.02.21 18:41, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Coverity will report unitialized values for every use of xs_state_* >> structures in the save part. This can be prevented by using the [0] >> rather than [] to define variable length array. >> >> Coverity-ID: 1472398 >> Coverity-ID: 1472397 >> Coverity-ID: 1472396 >> Coverity-ID: 1472395 >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Sorry, but Coverity is clearly wrong here. I saw what Andrew wrote but neither of you really provided enough information to infer the same. Care to provide more details? > > Should we really modify our code to work around bugs in external > static code analyzers? I don't think it is OK to have 866 issues (and counting) and keep ignoring them because Coverity may be wrong. We should fix them one way or another. If this means telling Coverity they are reporting false positive, then fine. But for that, I first needs a bit more details why they are clearly wrong. Cheers,
On 26.02.21 09:57, Julien Grall wrote: > Hi Juergen, > > On 26/02/2021 07:10, Jürgen Groß wrote: >> On 25.02.21 18:41, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> Coverity will report unitialized values for every use of xs_state_* >>> structures in the save part. This can be prevented by using the [0] >>> rather than [] to define variable length array. >>> >>> Coverity-ID: 1472398 >>> Coverity-ID: 1472397 >>> Coverity-ID: 1472396 >>> Coverity-ID: 1472395 >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> Sorry, but Coverity is clearly wrong here. > I saw what Andrew wrote but neither of you really provided enough > information to infer the same. Care to provide more details? > >> >> Should we really modify our code to work around bugs in external >> static code analyzers? > > I don't think it is OK to have 866 issues (and counting) and keep > ignoring them because Coverity may be wrong. We should fix them one way > or another. If this means telling Coverity they are reporting false > positive, then fine. > > But for that, I first needs a bit more details why they are clearly wrong. Lets put it this way: why is a[0] not critical, but a[] is? Semantically there is no difference, so Coverity MUST be wrong in some way (either a[] is really not critical, or a[0] should be critical). Juergen
diff --git a/tools/xenstore/include/xenstore_state.h b/tools/xenstore/include/xenstore_state.h index ae0d053c8ffc..407d9e920c0f 100644 --- a/tools/xenstore/include/xenstore_state.h +++ b/tools/xenstore/include/xenstore_state.h @@ -86,7 +86,7 @@ struct xs_state_connection { uint16_t data_in_len; /* Number of unprocessed bytes read from conn. */ uint16_t data_resp_len; /* Size of partial response pending for conn. */ uint32_t data_out_len; /* Number of bytes not yet written to conn. */ - uint8_t data[]; /* Pending data (read, written) + 0-7 pad bytes. */ + uint8_t data[0]; /* Pending data (read, written) + 0-7 pad bytes. */ }; /* Watch: */ @@ -94,7 +94,7 @@ struct xs_state_watch { uint32_t conn_id; /* Connection this watch is associated with. */ uint16_t path_length; /* Number of bytes of path watched (incl. 0). */ uint16_t token_length; /* Number of bytes of watch token (incl. 0). */ - uint8_t data[]; /* Path bytes, token bytes, 0-7 pad bytes. */ + uint8_t data[0]; /* Path bytes, token bytes, 0-7 pad bytes. */ }; /* Transaction: */ @@ -125,7 +125,7 @@ struct xs_state_node { #define XS_STATE_NODE_TA_WRITTEN 0x0002 uint16_t perm_n; /* Number of permissions (0 in TA: node deleted). */ /* Permissions (first is owner, has full access). */ - struct xs_state_node_perm perms[]; + struct xs_state_node_perm perms[0]; /* Path and data follows, plus 0-7 pad bytes. */ }; #endif /* XENSTORE_STATE_H */