[PATCH] tools: libxenstat: fix format string overflow

[PATCH] tools: libxenstat: fix format string overflow

[Dario Faggioli]

With gcc 7.3.0, the build fails like this:

src/xenstat_linux.c: In function ‘getBridge’
src/xenstat_linux.c:78:34: warning: ‘%s’ directive writing up to 255 bytes into a region of size 241 [-Wformat-overflow=]
     sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
                                  ^~
src/xenstat_linux.c:78:5: note: ‘sprintf’ output between 23 and 278 bytes into a destination of size 256
     sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix using asprintf().

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
In case no one else have noticed and fixed this (I have checked xen-devel and
found nothing)
---
 tools/xenstat/libxenstat/src/xenstat_linux.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c b/tools/xenstat/libxenstat/src/xenstat_linux.c
index 907d65fa63..396357511b 100644
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
@@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result, size_t resultLen)
 	struct dirent *de;
 	DIR *d;
 
-	char tmp[256] = { 0 };
-
 	d = opendir("/sys/class/net");
 	while ((de = readdir(d)) != NULL) {
 		if ((strlen(de->d_name) > 0) && (de->d_name[0] != '.')
 			&& (strstr(de->d_name, excludeName) == NULL)) {
-				sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
+				char *tmp;
+
+				asprintf(&tmp, "/sys/class/net/%s/bridge", de->d_name);
 
 				if (access(tmp, F_OK) == 0) {
 					strncpy(result, de->d_name, resultLen - 1);
 					result[resultLen - 1] = 0;
 				}
+
+				free(tmp);
 		}
 	}
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Andrew Cooper]

On 16/02/18 17:36, Dario Faggioli wrote:
> With gcc 7.3.0, the build fails like this:
>
> src/xenstat_linux.c: In function ‘getBridge’
> src/xenstat_linux.c:78:34: warning: ‘%s’ directive writing up to 255 bytes into a region of size 241 [-Wformat-overflow=]
>      sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
>                                   ^~
> src/xenstat_linux.c:78:5: note: ‘sprintf’ output between 23 and 278 bytes into a destination of size 256
>      sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fix using asprintf().
>
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> In case no one else have noticed and fixed this (I have checked xen-devel and
> found nothing)
> ---
>  tools/xenstat/libxenstat/src/xenstat_linux.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c b/tools/xenstat/libxenstat/src/xenstat_linux.c
> index 907d65fa63..396357511b 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_linux.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
> @@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result, size_t resultLen)
>  	struct dirent *de;
>  	DIR *d;
>  
> -	char tmp[256] = { 0 };
> -
>  	d = opendir("/sys/class/net");
>  	while ((de = readdir(d)) != NULL) {
>  		if ((strlen(de->d_name) > 0) && (de->d_name[0] != '.')
>  			&& (strstr(de->d_name, excludeName) == NULL)) {
> -				sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
> +				char *tmp;
> +
> +				asprintf(&tmp, "/sys/class/net/%s/bridge", de->d_name);
>  
>  				if (access(tmp, F_OK) == 0) {

Possible NULL dereference.

IIRC, you need to set tmp to NULL first, otherwise you may free a
spurious pointer on failure, and check for asprintf() returning < 0.

~Andrew

>  					strncpy(result, de->d_name, resultLen - 1);
>  					result[resultLen - 1] = 0;
>  				}
> +
> +				free(tmp);
>  		}
>  	}
>  
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Wei Liu]

On Fri, Feb 16, 2018 at 06:36:51PM +0100, Dario Faggioli wrote:
> With gcc 7.3.0, the build fails like this:
> 
> src/xenstat_linux.c: In function ‘getBridge’
> src/xenstat_linux.c:78:34: warning: ‘%s’ directive writing up to 255 bytes into a region of size 241 [-Wformat-overflow=]
>      sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
>                                   ^~
> src/xenstat_linux.c:78:5: note: ‘sprintf’ output between 23 and 278 bytes into a destination of size 256
>      sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix using asprintf().
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> In case no one else have noticed and fixed this (I have checked xen-devel and
> found nothing)
> ---
>  tools/xenstat/libxenstat/src/xenstat_linux.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c b/tools/xenstat/libxenstat/src/xenstat_linux.c
> index 907d65fa63..396357511b 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_linux.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
> @@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result, size_t resultLen)
>  	struct dirent *de;
>  	DIR *d;
>  
> -	char tmp[256] = { 0 };
> -
>  	d = opendir("/sys/class/net");
>  	while ((de = readdir(d)) != NULL) {
>  		if ((strlen(de->d_name) > 0) && (de->d_name[0] != '.')
>  			&& (strstr(de->d_name, excludeName) == NULL)) {
> -				sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
> +				char *tmp;
> +
> +				asprintf(&tmp, "/sys/class/net/%s/bridge", de->d_name);

Need to check the return value of asprintf.

Preferably you also need to define _GNU_SOURCE at the beginning of this
file. This file is Linux only makes it less of a problem.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Wei Liu]

On Fri, Feb 16, 2018 at 05:44:05PM +0000, Wei Liu wrote:
> On Fri, Feb 16, 2018 at 06:36:51PM +0100, Dario Faggioli wrote:
> > With gcc 7.3.0, the build fails like this:
> > 
> > src/xenstat_linux.c: In function ‘getBridge’
> > src/xenstat_linux.c:78:34: warning: ‘%s’ directive writing up to 255 bytes into a region of size 241 [-Wformat-overflow=]
> >      sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
> >                                   ^~
> > src/xenstat_linux.c:78:5: note: ‘sprintf’ output between 23 and 278 bytes into a destination of size 256
> >      sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
> >      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Fix using asprintf().
> > 
> > Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > In case no one else have noticed and fixed this (I have checked xen-devel and
> > found nothing)
> > ---
> >  tools/xenstat/libxenstat/src/xenstat_linux.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c b/tools/xenstat/libxenstat/src/xenstat_linux.c
> > index 907d65fa63..396357511b 100644
> > --- a/tools/xenstat/libxenstat/src/xenstat_linux.c
> > +++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
> > @@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result, size_t resultLen)
> >  	struct dirent *de;
> >  	DIR *d;
> >  
> > -	char tmp[256] = { 0 };
> > -
> >  	d = opendir("/sys/class/net");
> >  	while ((de = readdir(d)) != NULL) {
> >  		if ((strlen(de->d_name) > 0) && (de->d_name[0] != '.')
> >  			&& (strstr(de->d_name, excludeName) == NULL)) {
> > -				sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
> > +				char *tmp;
> > +
> > +				asprintf(&tmp, "/sys/class/net/%s/bridge", de->d_name);
> 
> Need to check the return value of asprintf.
> 
> Preferably you also need to define _GNU_SOURCE at the beginning of this
> file. This file is Linux only makes it less of a problem.

To be precise: not necessarily at the beginning of this file, just
before the actual inclusion of stdio.h.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1191 bytes --]

On Fri, 2018-02-16 at 17:44 +0000, Wei Liu wrote:
> On Fri, Feb 16, 2018 at 06:36:51PM +0100, Dario Faggioli wrote:
> > 
> > --- a/tools/xenstat/libxenstat/src/xenstat_linux.c
> > +++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
> > @@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result,
> > size_t resultLen)
> >  	struct dirent *de;
> >  	DIR *d;
> >  
> > -	char tmp[256] = { 0 };
> > -
> >  	d = opendir("/sys/class/net");
> >  	while ((de = readdir(d)) != NULL) {
> >  		if ((strlen(de->d_name) > 0) && (de->d_name[0] !=
> > '.')
> >  			&& (strstr(de->d_name, excludeName) ==
> > NULL)) {
> > -				sprintf(tmp,
> > "/sys/class/net/%s/bridge", de->d_name);
> > +				char *tmp;
> > +
> > +				asprintf(&tmp,
> > "/sys/class/net/%s/bridge", de->d_name);
> 
> Need to check the return value of asprintf.
> 
Right! And what do I do if it fails, 'continue' the while(), I guess?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1191 bytes --]

On Fri, 2018-02-16 at 17:44 +0000, Wei Liu wrote:
> On Fri, Feb 16, 2018 at 06:36:51PM +0100, Dario Faggioli wrote:
> > 
> > --- a/tools/xenstat/libxenstat/src/xenstat_linux.c
> > +++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
> > @@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result,
> > size_t resultLen)
> >  	struct dirent *de;
> >  	DIR *d;
> >  
> > -	char tmp[256] = { 0 };
> > -
> >  	d = opendir("/sys/class/net");
> >  	while ((de = readdir(d)) != NULL) {
> >  		if ((strlen(de->d_name) > 0) && (de->d_name[0] !=
> > '.')
> >  			&& (strstr(de->d_name, excludeName) ==
> > NULL)) {
> > -				sprintf(tmp,
> > "/sys/class/net/%s/bridge", de->d_name);
> > +				char *tmp;
> > +
> > +				asprintf(&tmp,
> > "/sys/class/net/%s/bridge", de->d_name);
> 
> Need to check the return value of asprintf.
> 
Right! And what do I do if it fails, 'continue' the while(), I guess?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Wei Liu]

On Fri, Feb 16, 2018 at 06:55:08PM +0100, Dario Faggioli wrote:
> On Fri, 2018-02-16 at 17:44 +0000, Wei Liu wrote:
> > On Fri, Feb 16, 2018 at 06:36:51PM +0100, Dario Faggioli wrote:
> > > 
> > > --- a/tools/xenstat/libxenstat/src/xenstat_linux.c
> > > +++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
> > > @@ -69,18 +69,20 @@ void getBridge(char *excludeName, char *result,
> > > size_t resultLen)
> > >  	struct dirent *de;
> > >  	DIR *d;
> > >  
> > > -	char tmp[256] = { 0 };
> > > -
> > >  	d = opendir("/sys/class/net");
> > >  	while ((de = readdir(d)) != NULL) {
> > >  		if ((strlen(de->d_name) > 0) && (de->d_name[0] !=
> > > '.')
> > >  			&& (strstr(de->d_name, excludeName) ==
> > > NULL)) {
> > > -				sprintf(tmp,
> > > "/sys/class/net/%s/bridge", de->d_name);
> > > +				char *tmp;
> > > +
> > > +				asprintf(&tmp,
> > > "/sys/class/net/%s/bridge", de->d_name);
> > 
> > Need to check the return value of asprintf.
> > 
> Right! And what do I do if it fails, 'continue' the while(), I guess?
> 

Looking at the error message again, can you just increase the buffer
size to 512? That should do the job?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools: libxenstat: fix format string overflow

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 724 bytes --]

On Fri, 2018-02-16 at 17:58 +0000, Wei Liu wrote:
> On Fri, Feb 16, 2018 at 06:55:08PM +0100, Dario Faggioli wrote:
> > On Fri, 2018-02-16 at 17:44 +0000, Wei Liu wrote:
> > > 
> > Right! And what do I do if it fails, 'continue' the while(), I
> > guess?
> > 
> 
> Looking at the error message again, can you just increase the buffer
> size to 512?
>
Sure I can.

>  That should do the job?
> 
Yeah, less "elegant", IMHO, but less tricky indeed. :-D

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel