[PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code

[PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

The AWS coverity instance spotted a few issues that could either
leak memory and derefence NULL pointer.

All the patches are candidate for 4.15 as they are hardening XenStored
code. The changes are low risks.

Cheers,

Julien Grall (5):
  tools/xenstored: Avoid unnecessary talloc_strdup() in do_control_lu()
  tools/xenstored: Avoid unnecessary talloc_strdup() in do_lu_start()
  tools/xenstored: control: Store the save filename in lu_dump_state
  tools/xenstore-control: Don't leak buf in live_update_start()
  tools/xenstored: Silence coverity when using xs_state_* structures

 tools/xenstore/include/xenstore_state.h |  6 +++---
 tools/xenstore/xenstore_control.c       |  4 +++-
 tools/xenstore/xenstored_control.c      | 26 +++++++++++--------------
 3 files changed, 17 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH for-4.15 1/5] tools/xenstored: Avoid unnecessary talloc_strdup() in do_control_lu()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, the return of talloc_strdup() is not checked. This means
we may dereference a NULL pointer if the allocation failed.

However, it is pointless to allocate the memory as send_reply() will
copy the data to a different buffer. So drop the use of talloc_strdup().

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: fecab256d474 ("tools/xenstore: add basic live-update command parsing")
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_control.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index f10beaf85eb4..e8a501acdb62 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -691,7 +691,6 @@ static const char *lu_start(const void *ctx, struct connection *conn,
 static int do_control_lu(void *ctx, struct connection *conn,
 			 char **vec, int num)
 {
-	const char *resp;
 	const char *ret = NULL;
 	unsigned int i;
 	bool force = false;
@@ -734,8 +733,7 @@ static int do_control_lu(void *ctx, struct connection *conn,
 
 	if (!ret)
 		ret = "OK";
-	resp = talloc_strdup(ctx, ret);
-	send_reply(conn, XS_CONTROL, resp, strlen(resp) + 1);
+	send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
 	return 0;
 }
 #endif
-- 
2.17.1

[PATCH for-4.15 2/5] tools/xenstored: Avoid unnecessary talloc_strdup() in do_lu_start()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, the return of talloc_strdup() is not checked. This means
we may dereference a NULL pointer if the allocation failed.

However, it is pointless to allocate the memory as send_reply() will
copy the data to a different buffer. So drop the use of talloc_strdup().

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: af216a99fb4a ("tools/xenstore: add the basic framework for doing the live update")
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_control.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index e8a501acdb62..8eb57827765c 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -638,7 +638,6 @@ static bool do_lu_start(struct delayed_request *req)
 {
 	time_t now = time(NULL);
 	const char *ret;
-	char *resp;
 
 	if (!lu_check_lu_allowed()) {
 		if (now < lu_status->started_at + lu_status->timeout)
@@ -660,8 +659,7 @@ static bool do_lu_start(struct delayed_request *req)
  out:
 	talloc_free(lu_status);
 
-	resp = talloc_strdup(req->in, ret);
-	send_reply(lu_status->conn, XS_CONTROL, resp, strlen(resp) + 1);
+	send_reply(lu_status->conn, XS_CONTROL, ret, strlen(ret) + 1);
 
 	return true;
 }
-- 
2.17.1

[PATCH for-4.15 3/5] tools/xenstored: control: Store the save filename in lu_dump_state

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The function lu_close_dump_state() will use talloc_asprintf() without
checking whether the allocation succeeded. In the unlikely case we are
out of memory, we would dereference a NULL pointer.

As we already computed the filename in lu_get_dump_state(), we can store
the name in the lu_dump_state. This is avoiding to deal with memory file
in the close path and also reduce the risk to use the different
filename.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: c0dc6a3e7c41 ("tools/xenstore: read internal state when doing live upgrade")
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_control.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 8eb57827765c..653890f2d9e0 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -16,6 +16,7 @@ Interactive commands for Xen Store Daemon.
     along with this program; If not, see <http://www.gnu.org/licenses/>.
 */
 
+#include <assert.h>
 #include <ctype.h>
 #include <errno.h>
 #include <stdarg.h>
@@ -74,6 +75,7 @@ struct lu_dump_state {
 	unsigned int size;
 #ifndef __MINIOS__
 	int fd;
+	char *filename;
 #endif
 };
 
@@ -399,17 +401,16 @@ static void lu_dump_close(FILE *fp)
 
 static void lu_get_dump_state(struct lu_dump_state *state)
 {
-	char *filename;
 	struct stat statbuf;
 
 	state->size = 0;
 
-	filename = talloc_asprintf(NULL, "%s/state_dump", xs_daemon_rootdir());
-	if (!filename)
+	state->filename = talloc_asprintf(NULL, "%s/state_dump",
+					  xs_daemon_rootdir());
+	if (!state->filename)
 		barf("Allocation failure");
 
-	state->fd = open(filename, O_RDONLY);
-	talloc_free(filename);
+	state->fd = open(state->filename, O_RDONLY);
 	if (state->fd < 0)
 		return;
 	if (fstat(state->fd, &statbuf) != 0)
@@ -431,14 +432,13 @@ static void lu_get_dump_state(struct lu_dump_state *state)
 
 static void lu_close_dump_state(struct lu_dump_state *state)
 {
-	char *filename;
+	assert(state->filename != NULL);
 
 	munmap(state->buf, state->size);
 	close(state->fd);
 
-	filename = talloc_asprintf(NULL, "%s/state_dump", xs_daemon_rootdir());
-	unlink(filename);
-	talloc_free(filename);
+	unlink(state->filename);
+	talloc_free(state->filename);
 }
 
 static char *lu_exec(const void *ctx, int argc, char **argv)
-- 
2.17.1

[PATCH for-4.15 4/5] tools/xenstore-control: Don't leak buf in live_update_start()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

All the error paths but one will free buf. Cover the remaining path so
buf can't be leaked.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: 7f97193e6aa8 ("tools/xenstore: add live update command to xenstore-control")
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstore_control.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstore_control.c b/tools/xenstore/xenstore_control.c
index f6f4626c0656..548363ee7094 100644
--- a/tools/xenstore/xenstore_control.c
+++ b/tools/xenstore/xenstore_control.c
@@ -44,8 +44,10 @@ static int live_update_start(struct xs_handle *xsh, bool force, unsigned int to)
         return 1;
 
     ret = strdup("BUSY");
-    if (!ret)
+    if (!ret) {
+        free(buf);
         return 1;
+    }
 
     for (time_start = time(NULL); time(NULL) - time_start < to;) {
         free(ret);
-- 
2.17.1

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

[Julien Grall]

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

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

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

[Andrew Cooper]

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

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

[Julien Grall]

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,

-- 
Julien Grall

[PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code

[Ian Jackson]

Julien Grall writes ("[PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code"):
>   tools/xenstored: Avoid unnecessary talloc_strdup() in do_control_lu()
>   tools/xenstored: Avoid unnecessary talloc_strdup() in do_lu_start()
>   tools/xenstored: control: Store the save filename in lu_dump_state
>   tools/xenstore-control: Don't leak buf in live_update_start()

These four are actual bugfixes:

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

>   tools/xenstored: Silence coverity when using xs_state_* structures

For this I can't see a reason to give a release ack ?  See also Andy's
comments.

Ian.

Re: [PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code

[Julien Grall]

Hi Ian,

On 25/02/2021 17:54, Ian Jackson wrote:
> Julien Grall writes ("[PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code"):
>>    tools/xenstored: Avoid unnecessary talloc_strdup() in do_control_lu()
>>    tools/xenstored: Avoid unnecessary talloc_strdup() in do_lu_start()
>>    tools/xenstored: control: Store the save filename in lu_dump_state
>>    tools/xenstore-control: Don't leak buf in live_update_start()
> 
> These four are actual bugfixes:
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks!

> 
>>    tools/xenstored: Silence coverity when using xs_state_* structures
> 
> For this I can't see a reason to give a release ack ?  See also Andy's
> comments.

I don't have a reason for this one as it is so far just silencing 
Coverity. Sorry I should have mention that this one is not really 4.15 
material.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code

[Ian Jackson]

Julien Grall writes ("Re: [PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code"):
> On 25/02/2021 17:54, Ian Jackson wrote:
> > Julien Grall writes ("[PATCH for-4.15 0/5] xenstore: Address coverity issues in the LiveUpdate code"):
> >>    tools/xenstored: Silence coverity when using xs_state_* structures
> > 
> > For this I can't see a reason to give a release ack ?  See also Andy's
> > comments.
> 
> I don't have a reason for this one as it is so far just silencing 
> Coverity. Sorry I should have mention that this one is not really 4.15 
> material.

No problem, thanks for the fixes!

Ian.

Re: [PATCH for-4.15 1/5] tools/xenstored: Avoid unnecessary talloc_strdup() in do_control_lu()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 695 bytes --]

On 25.02.21 18:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the return of talloc_strdup() is not checked. This means
> we may dereference a NULL pointer if the allocation failed.
> 
> However, it is pointless to allocate the memory as send_reply() will
> copy the data to a different buffer. So drop the use of talloc_strdup().
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: fecab256d474 ("tools/xenstore: add basic live-update command parsing")
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH for-4.15 2/5] tools/xenstored: Avoid unnecessary talloc_strdup() in do_lu_start()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 705 bytes --]

On 25.02.21 18:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the return of talloc_strdup() is not checked. This means
> we may dereference a NULL pointer if the allocation failed.
> 
> However, it is pointless to allocate the memory as send_reply() will
> copy the data to a different buffer. So drop the use of talloc_strdup().
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: af216a99fb4a ("tools/xenstore: add the basic framework for doing the live update")
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH for-4.15 3/5] tools/xenstored: control: Store the save filename in lu_dump_state

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 847 bytes --]

On 25.02.21 18:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The function lu_close_dump_state() will use talloc_asprintf() without
> checking whether the allocation succeeded. In the unlikely case we are
> out of memory, we would dereference a NULL pointer.
> 
> As we already computed the filename in lu_get_dump_state(), we can store
> the name in the lu_dump_state. This is avoiding to deal with memory file
> in the close path and also reduce the risk to use the different
> filename.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: c0dc6a3e7c41 ("tools/xenstore: read internal state when doing live upgrade")
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH for-4.15 4/5] tools/xenstore-control: Don't leak buf in live_update_start()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 507 bytes --]

On 25.02.21 18:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> All the error paths but one will free buf. Cover the remaining path so
> buf can't be leaked.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: 7f97193e6aa8 ("tools/xenstore: add live update command to xenstore-control")
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2563 bytes --]

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


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1393 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]