diff mbox series

[for-4.15,5/5] tools/xenstored: Silence coverity when using xs_state_* structures

Message ID 20210225174131.10115-6-julien@xen.org (mailing list archive)
State New
Headers show
Series xenstore: Address coverity issues in the LiveUpdate code | expand

Commit Message

Julien Grall Feb. 25, 2021, 5:41 p.m. UTC
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.
---
 tools/xenstore/include/xenstore_state.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andrew Cooper Feb. 25, 2021, 5:47 p.m. UTC | #1
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
Julien Grall Feb. 25, 2021, 5:53 p.m. UTC | #2
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,
Juergen Gross Feb. 26, 2021, 7:10 a.m. UTC | #3
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 */
>
Julien Grall Feb. 26, 2021, 8:57 a.m. UTC | #4
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,
Juergen Gross Feb. 26, 2021, 9:15 a.m. UTC | #5
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 mbox series

Patch

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 */