[PATCH 0/4] Coverity fixes for tools/xenstore

[PATCH 0/4] Coverity fixes for tools/xenstore

[Andrew Cooper]

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

-- 
1.7.10.4

[PATCH 1/4] tools/xenstore: Fix 15 potential resource leaks in build()

[Andrew Cooper]

Coverity ID: 1055933 1055934

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xenstore/init-xenstore-domain.c |   45 ++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/tools/xenstore/init-xenstore-domain.c b/tools/xenstore/init-xenstore-domain.c
index 35f1aa3..56a3c72 100644
--- a/tools/xenstore/init-xenstore-domain.c
+++ b/tools/xenstore/init-xenstore-domain.c
@@ -18,51 +18,60 @@ static int build(xc_interface *xch, char** argv)
 	char cmdline[512];
 	uint32_t ssid;
 	xen_domain_handle_t handle = { 0 };
-	int rv;
-	int xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
-	struct xc_dom_image *dom;
+	int rv, xs_fd;
+	struct xc_dom_image *dom = NULL;
 	int maxmem = atoi(argv[2]);
 	int limit_kb = (maxmem + 1)*1024;
 
+	xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
+	if (xs_fd == -1) return -1;
+
 	rv = xc_flask_context_to_sid(xch, argv[3], strlen(argv[3]), &ssid);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_domain_create(xch, ssid, handle, 0, &domid);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_domain_max_vcpus(xch, domid, 1);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_domain_setmaxmem(xch, domid, limit_kb);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
-	if (rv) return rv;
+	if (rv) goto err;
 
 	rv = ioctl(xs_fd, IOCTL_XENBUS_BACKEND_SETUP, domid);
-	if (rv < 0) return rv;
+	if (rv < 0) goto err;
 	snprintf(cmdline, 512, "--event %d --internal-db", rv);
 
 	dom = xc_dom_allocate(xch, cmdline, NULL);
 	rv = xc_dom_kernel_file(dom, argv[1]);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_dom_boot_xen_init(dom, xch, domid);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_dom_parse_image(dom);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_dom_mem_init(dom, maxmem);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_dom_boot_mem_init(dom);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_dom_build_image(dom);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_dom_boot_image(dom);
-	if (rv) return rv;
+	if (rv) goto err;
 
 	xc_dom_release(dom);
+	dom = NULL;
 
 	rv = xc_domain_set_virq_handler(xch, domid, VIRQ_DOM_EXC);
-	if (rv) return rv;
+	if (rv) goto err;
 	rv = xc_domain_unpause(xch, domid);
-	if (rv) return rv;
+	if (rv) goto err;
 
 	return 0;
+
+err:
+	if (dom)
+		xc_dom_release(dom);
+	close(xs_fd);
+	return rv;
 }
 
 int main(int argc, char** argv)
-- 
1.7.10.4

[PATCH 2/4] tools/xenstore-rm: Fix memory leaks

[Andrew Cooper]

Coverity ID: 1055935

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xenstore/xenstore_client.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 3ac214b..0ec103f 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -367,10 +367,13 @@ perform(enum mode mode, int optind, int argc, char **argv, struct xs_handle *xsh
 
                         if (list) {
                             free(list);
-                            if (num == 0)
+                            if (num == 0){
+                                free(val);
                                 goto again;
+                            }
                         }
                     }
+                    free(val);
                 }
 
                 free(path);
-- 
1.7.10.4

[PATCH 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Andrew Cooper]

Coverity ID: 1055996 1056002

Use strncpy in preference to strcpy, and use the correct failing path for
error messages.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index ccfdaa3..3c13c64 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1735,13 +1735,12 @@ static void init_sockets(int **psock, int **pro_sock)
 	unlink(xs_daemon_socket_ro());
 
 	addr.sun_family = AF_UNIX;
-	strcpy(addr.sun_path, xs_daemon_socket());
+	strncpy(addr.sun_path, xs_daemon_socket(), sizeof(addr.sun_path));
 	if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
-		barf_perror("Could not bind socket to %s", xs_daemon_socket());
-	strcpy(addr.sun_path, xs_daemon_socket_ro());
+		barf_perror("Could not bind socket to %s", addr.sun_path);
+	strncpy(addr.sun_path, xs_daemon_socket_ro(), sizeof(addr.sun_path));
 	if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
-		barf_perror("Could not bind socket to %s",
-			    xs_daemon_socket_ro());
+		barf_perror("Could not bind socket to %s", addr.sun_path);
 	if (chmod(xs_daemon_socket(), 0600) != 0
 	    || chmod(xs_daemon_socket_ro(), 0660) != 0)
 		barf_perror("Could not chmod sockets");
-- 
1.7.10.4

[PATCH 4/4] tools/xenstored: Don't leak a file handle when creating the pidfile

[Andrew Cooper]

Coverity ID: 1055849

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xenstore/xenstored_posix.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/xenstore/xenstored_posix.c b/tools/xenstore/xenstored_posix.c
index 25bdf74..0c93e6d 100644
--- a/tools/xenstore/xenstored_posix.c
+++ b/tools/xenstore/xenstored_posix.c
@@ -44,6 +44,8 @@ void write_pidfile(const char *pidfile)
 	len = snprintf(buf, sizeof(buf), "%ld\n", (long)getpid());
 	if (write(fd, buf, len) != len)
 		barf_perror("Writing pid file %s", pidfile);
+
+	close(fd);
 }
 
 /* Stevens. */
-- 
1.7.10.4

[PATCH 1/4] tools/xenstore: Fix 15 potential resource leaks in build()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 1/4] tools/xenstore: Fix 15 potential resource leaks in build()"):
> Coverity ID: 1055933 1055934
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

This is theoretical (if build fails, main returns), so I shan't
backport it.

Ian.

Re: [PATCH 2/4] tools/xenstore-rm: Fix memory leaks

[Ian Jackson]

Andrew Cooper writes ("[PATCH 2/4] tools/xenstore-rm: Fix memory leaks"):
> Coverity ID: 1055935
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Matthew Daley]

On Tue, Nov 26, 2013 at 12:07 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1055996 1056002
>
> Use strncpy in preference to strcpy, and use the correct failing path for
> error messages.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/xenstore/xenstored_core.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index ccfdaa3..3c13c64 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1735,13 +1735,12 @@ static void init_sockets(int **psock, int **pro_sock)
>         unlink(xs_daemon_socket_ro());
>
>         addr.sun_family = AF_UNIX;
> -       strcpy(addr.sun_path, xs_daemon_socket());
> +       strncpy(addr.sun_path, xs_daemon_socket(), sizeof(addr.sun_path));

Nitpick: Using strncpy in this manner is unsafe as it does not
guarantee the null-termination of the result in the destination buffer
(which would presumably make barf_perror unhappy). For that reason,
and to avoid truncation, I check the source string length directly
instead in commit f220279c14, for example.

- Matthew

>         if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -               barf_perror("Could not bind socket to %s", xs_daemon_socket());
> -       strcpy(addr.sun_path, xs_daemon_socket_ro());
> +               barf_perror("Could not bind socket to %s", addr.sun_path);
> +       strncpy(addr.sun_path, xs_daemon_socket_ro(), sizeof(addr.sun_path));
>         if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -               barf_perror("Could not bind socket to %s",
> -                           xs_daemon_socket_ro());
> +               barf_perror("Could not bind socket to %s", addr.sun_path);
>         if (chmod(xs_daemon_socket(), 0600) != 0
>             || chmod(xs_daemon_socket_ro(), 0660) != 0)
>                 barf_perror("Could not chmod sockets");
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Ian Jackson]

Andrew Cooper writes ("[PATCH 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets"):
> Coverity ID: 1055996 1056002
> 
> Use strncpy in preference to strcpy, and use the correct failing path for
> error messages.
...
>  	addr.sun_family = AF_UNIX;
> -	strcpy(addr.sun_path, xs_daemon_socket());
> +	strncpy(addr.sun_path, xs_daemon_socket(), sizeof(addr.sun_path));
>  	if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -		barf_perror("Could not bind socket to %s", xs_daemon_socket());
> +		barf_perror("Could not bind socket to %s", addr.sun_path);

This latter hunk is not correct.  addr.sun_path might not be
nul-terminated.  xs_daemon_socket() is, but isn't the path actually
attempted.

Also, while this new code avoids UB, it still has the bug that if the
configured socket pathname is too long, xenstored will create a
version with a truncated path.

Perhaps a better approach would be an explicit overlength check.

Ian.

Re: [PATCH 4/4] tools/xenstored: Don't leak a file handle when creating the pidfile

[Ian Jackson]

Andrew Cooper writes ("[PATCH 4/4] tools/xenstored: Don't leak a file handle when creating the pidfile"):
> Coverity ID: 1055849
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

[Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Andrew Cooper]

Coverity ID: 1055996 1056002

Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
re-snprintf()'ing the same path, and add explicit size checks against
addr.sun_path before strcpy()'ing into it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Matthew Daley <mattd@bugfuzz.com>

---
Changes in v2:
 * Use logic similar to f220279c14
---
 tools/xenstore/xenstored_core.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index ccfdaa3..2324e53 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1718,6 +1718,9 @@ static void init_sockets(int **psock, int **pro_sock)
 {
 	struct sockaddr_un addr;
 	int *sock, *ro_sock;
+	const char *soc_str = xs_daemon_socket();
+	const char *soc_str_ro = xs_daemon_socket_ro();
+
 	/* Create sockets for them to listen to. */
 	*psock = sock = talloc(talloc_autofree_context(), int);
 	*sock = socket(PF_UNIX, SOCK_STREAM, 0);
@@ -1731,19 +1734,25 @@ static void init_sockets(int **psock, int **pro_sock)
 	talloc_set_destructor(ro_sock, destroy_fd);
 
 	/* FIXME: Be more sophisticated, don't mug running daemon. */
-	unlink(xs_daemon_socket());
-	unlink(xs_daemon_socket_ro());
+	unlink(soc_str);
+	unlink(soc_str_ro);
 
 	addr.sun_family = AF_UNIX;
-	strcpy(addr.sun_path, xs_daemon_socket());
+
+	if(strlen(soc_str) >= sizeof(addr.sun_path))
+		barf_perror("socket string '%s' too long", soc_str);
+	strcpy(addr.sun_path, soc_str);
 	if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
-		barf_perror("Could not bind socket to %s", xs_daemon_socket());
-	strcpy(addr.sun_path, xs_daemon_socket_ro());
+		barf_perror("Could not bind socket to %s", soc_str);
+
+	if(strlen(soc_str_ro) >= sizeof(addr.sun_path))
+		barf_perror("socket string '%s' too long", soc_str_ro);
+	strcpy(addr.sun_path, soc_str_ro);
 	if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
-		barf_perror("Could not bind socket to %s",
-			    xs_daemon_socket_ro());
-	if (chmod(xs_daemon_socket(), 0600) != 0
-	    || chmod(xs_daemon_socket_ro(), 0660) != 0)
+		barf_perror("Could not bind socket to %s", soc_str_ro);
+
+	if (chmod(soc_str, 0600) != 0
+	    || chmod(soc_str_ro, 0660) != 0)
 		barf_perror("Could not chmod sockets");
 
 	if (listen(*sock, 1) != 0
-- 
1.7.10.4

Re: [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Andrew Cooper]

Ping?  This v2 patch appears to have slipped through the cracks from my
set of Coverity fixes.

~Andrew

On 25/11/13 14:38, Andrew Cooper wrote:
> Coverity ID: 1055996 1056002
>
> Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
> re-snprintf()'ing the same path, and add explicit size checks against
> addr.sun_path before strcpy()'ing into it.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Matthew Daley <mattd@bugfuzz.com>
>
> ---
> Changes in v2:
>  * Use logic similar to f220279c14
> ---
>  tools/xenstore/xenstored_core.c |   27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index ccfdaa3..2324e53 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1718,6 +1718,9 @@ static void init_sockets(int **psock, int **pro_sock)
>  {
>  	struct sockaddr_un addr;
>  	int *sock, *ro_sock;
> +	const char *soc_str = xs_daemon_socket();
> +	const char *soc_str_ro = xs_daemon_socket_ro();
> +
>  	/* Create sockets for them to listen to. */
>  	*psock = sock = talloc(talloc_autofree_context(), int);
>  	*sock = socket(PF_UNIX, SOCK_STREAM, 0);
> @@ -1731,19 +1734,25 @@ static void init_sockets(int **psock, int **pro_sock)
>  	talloc_set_destructor(ro_sock, destroy_fd);
>  
>  	/* FIXME: Be more sophisticated, don't mug running daemon. */
> -	unlink(xs_daemon_socket());
> -	unlink(xs_daemon_socket_ro());
> +	unlink(soc_str);
> +	unlink(soc_str_ro);
>  
>  	addr.sun_family = AF_UNIX;
> -	strcpy(addr.sun_path, xs_daemon_socket());
> +
> +	if(strlen(soc_str) >= sizeof(addr.sun_path))
> +		barf_perror("socket string '%s' too long", soc_str);
> +	strcpy(addr.sun_path, soc_str);
>  	if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -		barf_perror("Could not bind socket to %s", xs_daemon_socket());
> -	strcpy(addr.sun_path, xs_daemon_socket_ro());
> +		barf_perror("Could not bind socket to %s", soc_str);
> +
> +	if(strlen(soc_str_ro) >= sizeof(addr.sun_path))
> +		barf_perror("socket string '%s' too long", soc_str_ro);
> +	strcpy(addr.sun_path, soc_str_ro);
>  	if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -		barf_perror("Could not bind socket to %s",
> -			    xs_daemon_socket_ro());
> -	if (chmod(xs_daemon_socket(), 0600) != 0
> -	    || chmod(xs_daemon_socket_ro(), 0660) != 0)
> +		barf_perror("Could not bind socket to %s", soc_str_ro);
> +
> +	if (chmod(soc_str, 0600) != 0
> +	    || chmod(soc_str_ro, 0660) != 0)
>  		barf_perror("Could not chmod sockets");
>  
>  	if (listen(*sock, 1) != 0

Re: [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Andrew Cooper]

Ping again?

On 02/12/13 13:18, Andrew Cooper wrote:
> Ping?  This v2 patch appears to have slipped through the cracks from my
> set of Coverity fixes.
>
> ~Andrew
>
> On 25/11/13 14:38, Andrew Cooper wrote:
>> Coverity ID: 1055996 1056002
>>
>> Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
>> re-snprintf()'ing the same path, and add explicit size checks against
>> addr.sun_path before strcpy()'ing into it.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Matthew Daley <mattd@bugfuzz.com>
>>
>> ---
>> Changes in v2:
>>  * Use logic similar to f220279c14
>> ---
>>  tools/xenstore/xenstored_core.c |   27 ++++++++++++++++++---------
>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index ccfdaa3..2324e53 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1718,6 +1718,9 @@ static void init_sockets(int **psock, int **pro_sock)
>>  {
>>  	struct sockaddr_un addr;
>>  	int *sock, *ro_sock;
>> +	const char *soc_str = xs_daemon_socket();
>> +	const char *soc_str_ro = xs_daemon_socket_ro();
>> +
>>  	/* Create sockets for them to listen to. */
>>  	*psock = sock = talloc(talloc_autofree_context(), int);
>>  	*sock = socket(PF_UNIX, SOCK_STREAM, 0);
>> @@ -1731,19 +1734,25 @@ static void init_sockets(int **psock, int **pro_sock)
>>  	talloc_set_destructor(ro_sock, destroy_fd);
>>  
>>  	/* FIXME: Be more sophisticated, don't mug running daemon. */
>> -	unlink(xs_daemon_socket());
>> -	unlink(xs_daemon_socket_ro());
>> +	unlink(soc_str);
>> +	unlink(soc_str_ro);
>>  
>>  	addr.sun_family = AF_UNIX;
>> -	strcpy(addr.sun_path, xs_daemon_socket());
>> +
>> +	if(strlen(soc_str) >= sizeof(addr.sun_path))
>> +		barf_perror("socket string '%s' too long", soc_str);
>> +	strcpy(addr.sun_path, soc_str);
>>  	if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
>> -		barf_perror("Could not bind socket to %s", xs_daemon_socket());
>> -	strcpy(addr.sun_path, xs_daemon_socket_ro());
>> +		barf_perror("Could not bind socket to %s", soc_str);
>> +
>> +	if(strlen(soc_str_ro) >= sizeof(addr.sun_path))
>> +		barf_perror("socket string '%s' too long", soc_str_ro);
>> +	strcpy(addr.sun_path, soc_str_ro);
>>  	if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
>> -		barf_perror("Could not bind socket to %s",
>> -			    xs_daemon_socket_ro());
>> -	if (chmod(xs_daemon_socket(), 0600) != 0
>> -	    || chmod(xs_daemon_socket_ro(), 0660) != 0)
>> +		barf_perror("Could not bind socket to %s", soc_str_ro);
>> +
>> +	if (chmod(soc_str, 0600) != 0
>> +	    || chmod(soc_str_ro, 0660) != 0)
>>  		barf_perror("Could not chmod sockets");
>>  
>>  	if (listen(*sock, 1) != 0

Re: [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Andrew Cooper]

Given the spirit today of missed pings on patches,

Third time lucky?

~Andrew

On 09/12/2013 13:32, Andrew Cooper wrote:
> Ping again?
>
> On 02/12/13 13:18, Andrew Cooper wrote:
>> Ping?  This v2 patch appears to have slipped through the cracks from my
>> set of Coverity fixes.
>>
>> ~Andrew
>>
>> On 25/11/13 14:38, Andrew Cooper wrote:
>>> Coverity ID: 1055996 1056002
>>>
>>> Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
>>> re-snprintf()'ing the same path, and add explicit size checks against
>>> addr.sun_path before strcpy()'ing into it.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Matthew Daley <mattd@bugfuzz.com>
>>>
>>> ---
>>> Changes in v2:
>>>  * Use logic similar to f220279c14
>>> ---
>>>  tools/xenstore/xenstored_core.c |   27 ++++++++++++++++++---------
>>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>>> index ccfdaa3..2324e53 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -1718,6 +1718,9 @@ static void init_sockets(int **psock, int **pro_sock)
>>>  {
>>>  	struct sockaddr_un addr;
>>>  	int *sock, *ro_sock;
>>> +	const char *soc_str = xs_daemon_socket();
>>> +	const char *soc_str_ro = xs_daemon_socket_ro();
>>> +
>>>  	/* Create sockets for them to listen to. */
>>>  	*psock = sock = talloc(talloc_autofree_context(), int);
>>>  	*sock = socket(PF_UNIX, SOCK_STREAM, 0);
>>> @@ -1731,19 +1734,25 @@ static void init_sockets(int **psock, int **pro_sock)
>>>  	talloc_set_destructor(ro_sock, destroy_fd);
>>>  
>>>  	/* FIXME: Be more sophisticated, don't mug running daemon. */
>>> -	unlink(xs_daemon_socket());
>>> -	unlink(xs_daemon_socket_ro());
>>> +	unlink(soc_str);
>>> +	unlink(soc_str_ro);
>>>  
>>>  	addr.sun_family = AF_UNIX;
>>> -	strcpy(addr.sun_path, xs_daemon_socket());
>>> +
>>> +	if(strlen(soc_str) >= sizeof(addr.sun_path))
>>> +		barf_perror("socket string '%s' too long", soc_str);
>>> +	strcpy(addr.sun_path, soc_str);
>>>  	if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
>>> -		barf_perror("Could not bind socket to %s", xs_daemon_socket());
>>> -	strcpy(addr.sun_path, xs_daemon_socket_ro());
>>> +		barf_perror("Could not bind socket to %s", soc_str);
>>> +
>>> +	if(strlen(soc_str_ro) >= sizeof(addr.sun_path))
>>> +		barf_perror("socket string '%s' too long", soc_str_ro);
>>> +	strcpy(addr.sun_path, soc_str_ro);
>>>  	if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
>>> -		barf_perror("Could not bind socket to %s",
>>> -			    xs_daemon_socket_ro());
>>> -	if (chmod(xs_daemon_socket(), 0600) != 0
>>> -	    || chmod(xs_daemon_socket_ro(), 0660) != 0)
>>> +		barf_perror("Could not bind socket to %s", soc_str_ro);
>>> +
>>> +	if (chmod(soc_str, 0600) != 0
>>> +	    || chmod(soc_str_ro, 0660) != 0)
>>>  		barf_perror("Could not chmod sockets");
>>>  
>>>  	if (listen(*sock, 1) != 0

Re: [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets [and 1 more messages]

[Ian Jackson]

Andrew Cooper writes ("[Xen-devel] [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets"):
> Coverity ID: 1055996 1056002
> 
> Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
> re-snprintf()'ing the same path, and add explicit size checks against
> addr.sun_path before strcpy()'ing into it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Matthew Daley <mattd@bugfuzz.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Andrew Cooper writes ("Re: [Xen-devel] [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets"):
> Given the spirit today of missed pings on patches,
> 
> Third time lucky?

Sorry about that.  (Coverity is generating a lot of very similar
patches; in this case I had confused this one in my mind with
f220279c14 which you even mention in the commit message.)

Thanks for chasing.

Ian.

Re: [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets [and 1 more messages]

[Andrew Cooper]

On 13/12/2013 18:28, Ian Jackson wrote:
> Andrew Cooper writes ("[Xen-devel] [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets"):
>> Coverity ID: 1055996 1056002
>>
>> Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
>> re-snprintf()'ing the same path, and add explicit size checks against
>> addr.sun_path before strcpy()'ing into it.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Matthew Daley <mattd@bugfuzz.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets"):
>> Given the spirit today of missed pings on patches,
>>
>> Third time lucky?
> Sorry about that.  (Coverity is generating a lot of very similar
> patches; in this case I had confused this one in my mind with
> f220279c14 which you even mention in the commit message.)
>
> Thanks for chasing.
>
> Ian.

Yes - that is a sad fact of all of these similar patches.  Hopefully
they will start thinning out as we get on top of the issues.

I am tracking "what still hasn't been applied" by what `git rebase
staging` tells me has still is still outstanding.  Of course, being my
private working tree, it is not easily exportable information.

On that note, I have some more pings to go.

~Andrew