[PATCH] tools/csr_usb: Fix compilation failure

[PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

GCC's "format-nonliteral" security check is enabled as an error in
recent versions of Fedora. Given the reduced scope of use, mark the
error as ignorable through pragma.

tools/csr_usb.c: In function 'read_value':
tools/csr_usb.c:82:2: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
  n = fscanf(file, format, &value);
  ^
---
 tools/csr_usb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/csr_usb.c b/tools/csr_usb.c
index a1d7324f7..33e9968a2 100644
--- a/tools/csr_usb.c
+++ b/tools/csr_usb.c
@@ -67,6 +67,8 @@ struct usbfs_bulktransfer {
 #define USBFS_IOCTL_CLAIMINTF	_IOR('U', 15, unsigned int)
 #define USBFS_IOCTL_RELEASEINTF	_IOR('U', 16, unsigned int)
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
 static int read_value(const char *name, const char *attr, const char *format)
 {
 	char path[PATH_MAX];
@@ -88,6 +90,7 @@ static int read_value(const char *name, const char *attr, const char *format)
 	fclose(file);
 	return value;
 }
+#pragma GCC diagnostic pop
 
 static char *check_device(const char *name)
 {
-- 
2.14.1

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

On Tue, 2017-09-05 at 10:35 +0200, Bastien Nocera wrote:
> GCC's "format-nonliteral" security check is enabled as an error in
> recent versions of Fedora. Given the reduced scope of use, mark the
> error as ignorable through pragma.
> 
> tools/csr_usb.c: In function 'read_value':
> tools/csr_usb.c:82:2: error: format not a string literal, argument
> types not checked [-Werror=format-nonliteral]
>   n = fscanf(file, format, &value);
>   ^

I also ran into this, which looks suspicious:
obexd/plugins/bluetooth.c: In function 'register_profile':
obexd/plugins/bluetooth.c:310:7: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
       profile->driver->port);
       ^~~~~~~
obexd/plugins/bluetooth.c:314:7: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
       profile->driver->name);
       ^~~~~~~

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Marcel Holtmann]

Hi Bastien,

>> GCC's "format-nonliteral" security check is enabled as an error in
>> recent versions of Fedora. Given the reduced scope of use, mark the
>> error as ignorable through pragma.
>> 
>> tools/csr_usb.c: In function 'read_value':
>> tools/csr_usb.c:82:2: error: format not a string literal, argument
>> types not checked [-Werror=format-nonliteral]
>>  n = fscanf(file, format, &value);
>>  ^
> 
> I also ran into this, which looks suspicious:
> obexd/plugins/bluetooth.c: In function 'register_profile':
> obexd/plugins/bluetooth.c:310:7: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
>       profile->driver->port);
>       ^~~~~~~
> obexd/plugins/bluetooth.c:314:7: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
>       profile->driver->name);
>       ^~~~~~~

we would have to change the code on how to build the XML record information. However here it is the same issue with the compiler not getting that the actual value used is const. So it seems this warning has no logic on traversing any levels down.

I mean for example HFP_HF_RECORD is a static const string. It just gets handed through an interim const char * pointer variable before used as format descriptor. Frankly gcc is stupid here.

Regards

Marcel

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

On Tue, 2017-09-05 at 15:15 +0200, Marcel Holtmann wrote:
> Hi Bastien,
> 
> > > GCC's "format-nonliteral" security check is enabled as an error
> > > in
> > > recent versions of Fedora. Given the reduced scope of use, mark
> > > the
> > > error as ignorable through pragma.
> > > 
> > > tools/csr_usb.c: In function 'read_value':
> > > tools/csr_usb.c:82:2: error: format not a string literal,
> > > argument
> > > types not checked [-Werror=format-nonliteral]
> > >  n = fscanf(file, format, &value);
> > >  ^
> > 
> > I also ran into this, which looks suspicious:
> > obexd/plugins/bluetooth.c: In function 'register_profile':
> > obexd/plugins/bluetooth.c:310:7: error: format not a string
> > literal, argument types not checked [-Werror=format-nonliteral]
> >       profile->driver->port);
> >       ^~~~~~~
> > obexd/plugins/bluetooth.c:314:7: error: format not a string
> > literal, argument types not checked [-Werror=format-nonliteral]
> >       profile->driver->name);
> >       ^~~~~~~
> 
> we would have to change the code on how to build the XML record
> information. However here it is the same issue with the compiler not
> getting that the actual value used is const. So it seems this warning
> has no logic on traversing any levels down.
> 
> I mean for example HFP_HF_RECORD is a static const string. It just
> gets handed through an interim const char * pointer variable before
> used as format descriptor. Frankly gcc is stupid here.

The problem isn't just the string being const or not, it's also the
compiler having no way to know the possible values for that string.

Maybe the string can only be a couple of hardcoded values, but that's
already stretching the limits of what analysis the compiler can do.

For example, how is it to know that when the port is 0, there's this
many arguments to the printf()-style function in the record string, and
this many when port is non-0?

If this is only used in the synce and pcsuite plugins, I'd do the
printf() in the plugins, instead of indirectly accessing the ->record
string. Would that be OK?

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Marcel Holtmann]

Hi Bastien,

> GCC's "format-nonliteral" security check is enabled as an error in
> recent versions of Fedora. Given the reduced scope of use, mark the
> error as ignorable through pragma.
> 
> tools/csr_usb.c: In function 'read_value':
> tools/csr_usb.c:82:2: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
>  n = fscanf(file, format, &value);
>  ^
> ---
> tools/csr_usb.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/tools/csr_usb.c b/tools/csr_usb.c
> index a1d7324f7..33e9968a2 100644
> --- a/tools/csr_usb.c
> +++ b/tools/csr_usb.c
> @@ -67,6 +67,8 @@ struct usbfs_bulktransfer {
> #define USBFS_IOCTL_CLAIMINTF	_IOR('U', 15, unsigned int)
> #define USBFS_IOCTL_RELEASEINTF	_IOR('U', 16, unsigned int)
> 
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> static int read_value(const char *name, const char *attr, const char *format)
> {
> 	char path[PATH_MAX];
> @@ -88,6 +90,7 @@ static int read_value(const char *name, const char *attr, const char *format)
> 	fclose(file);
> 	return value;
> }
> +#pragma GCC diagnostic pop

can’t we just use __attribute__((format)) somehow to declare this a format specify?

Regards

Marcel

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

On Tue, 2017-09-05 at 13:51 +0200, Marcel Holtmann wrote:
> Hi Bastien,
> 
> > GCC's "format-nonliteral" security check is enabled as an error in
> > recent versions of Fedora. Given the reduced scope of use, mark the
> > error as ignorable through pragma.
> > 
> > tools/csr_usb.c: In function 'read_value':
> > tools/csr_usb.c:82:2: error: format not a string literal, argument
> > types not checked [-Werror=format-nonliteral]
> >  n = fscanf(file, format, &value);
> >  ^
> > ---
> > tools/csr_usb.c | 3 +++
> > 1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/csr_usb.c b/tools/csr_usb.c
> > index a1d7324f7..33e9968a2 100644
> > --- a/tools/csr_usb.c
> > +++ b/tools/csr_usb.c
> > @@ -67,6 +67,8 @@ struct usbfs_bulktransfer {
> > #define USBFS_IOCTL_CLAIMINTF	_IOR('U', 15, unsigned int)
> > #define USBFS_IOCTL_RELEASEINTF	_IOR('U', 16, unsigned int)
> > 
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > static int read_value(const char *name, const char *attr, const
> > char *format)
> > {
> > 	char path[PATH_MAX];
> > @@ -88,6 +90,7 @@ static int read_value(const char *name, const
> > char *attr, const char *format)
> > 	fclose(file);
> > 	return value;
> > }
> > +#pragma GCC diagnostic pop
> 
> can’t we just use __attribute__((format)) somehow to declare this a
> format specify?

Sorry, I'm not sure I understand.

gcc already knows that this is a format, but because the format changes
depending on the caller, it cannot actually assert whether the format
is safe for the arguments passed.

So this would work:
vendor = read_value(name, "idVendor", "%04x");

But the compiler can't see that:
vendor = read_value(name, "idVendor", "%s");
would fail at compile time.

Would you prefer something like this (untested, but should work):
diff --git a/tools/csr_usb.c b/tools/csr_usb.c
index a1d7324f7..c5a39899a 100644
--- a/tools/csr_usb.c
+++ b/tools/csr_usb.c
@@ -67,7 +67,7 @@ struct usbfs_bulktransfer {
 #define USBFS_IOCTL_CLAIMINTF  _IOR('U', 15, unsigned int)
 #define USBFS_IOCTL_RELEASEINTF        _IOR('U', 16, unsigned int)
 
-static int read_value(const char *name, const char *attr, const char *format)
+static int read_value(const char *name, const char *attr, bool hex_number)
 {
        char path[PATH_MAX];
        FILE *file;
@@ -79,7 +79,7 @@ static int read_value(const char *name, const char *attr, const char *format)
        if (!file)
                return -1;
 
-       n = fscanf(file, format, &value);
+       n = fscanf(file, hex_number ? "%d" : "%04x", &value);
        if (n != 1) {
                fclose(file);
                return -1;
@@ -94,21 +94,21 @@ static char *check_device(const char *name)
        char path[PATH_MAX];
        int busnum, devnum, vendor, product;
 
-       busnum = read_value(name, "busnum", "%d");
+       busnum = read_value(name, "busnum", false);
        if (busnum < 0)
                return NULL;
 
-       devnum = read_value(name, "devnum", "%d");
+       devnum = read_value(name, "devnum", false);
        if (devnum < 0)
                return NULL;
 
        snprintf(path, sizeof(path), "/dev/bus/usb/%03u/%03u", busnum, devnum);
 
-       vendor = read_value(name, "idVendor", "%04x");
+       vendor = read_value(name, "idVendor", true);
        if (vendor < 0)
                return NULL;
 
-       product = read_value(name, "idProduct", "%04x");
+       product = read_value(name, "idProduct", true);
        if (product < 0)
                return NULL;

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Marcel Holtmann]

Hi Bastien,

>>> GCC's "format-nonliteral" security check is enabled as an error in
>>> recent versions of Fedora. Given the reduced scope of use, mark the
>>> error as ignorable through pragma.
>>> 
>>> tools/csr_usb.c: In function 'read_value':
>>> tools/csr_usb.c:82:2: error: format not a string literal, argument
>>> types not checked [-Werror=format-nonliteral]
>>> n = fscanf(file, format, &value);
>>> ^
>>> ---
>>> tools/csr_usb.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/tools/csr_usb.c b/tools/csr_usb.c
>>> index a1d7324f7..33e9968a2 100644
>>> --- a/tools/csr_usb.c
>>> +++ b/tools/csr_usb.c
>>> @@ -67,6 +67,8 @@ struct usbfs_bulktransfer {
>>> #define USBFS_IOCTL_CLAIMINTF	_IOR('U', 15, unsigned int)
>>> #define USBFS_IOCTL_RELEASEINTF	_IOR('U', 16, unsigned int)
>>> 
>>> +#pragma GCC diagnostic push
>>> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
>>> static int read_value(const char *name, const char *attr, const
>>> char *format)
>>> {
>>> 	char path[PATH_MAX];
>>> @@ -88,6 +90,7 @@ static int read_value(const char *name, const
>>> char *attr, const char *format)
>>> 	fclose(file);
>>> 	return value;
>>> }
>>> +#pragma GCC diagnostic pop
>> 
>> can’t we just use __attribute__((format)) somehow to declare this a
>> format specify?
> 
> Sorry, I'm not sure I understand.
> 
> gcc already knows that this is a format, but because the format changes
> depending on the caller, it cannot actually assert whether the format
> is safe for the arguments passed.
> 
> So this would work:
> vendor = read_value(name, "idVendor", "%04x");
> 
> But the compiler can't see that:
> vendor = read_value(name, "idVendor", "%s");
> would fail at compile time.

since these values are all marked as const, this compiler error is than pretty purely implemented. It could actually traverse this since they are const format strings.

> Would you prefer something like this (untested, but should work):
> diff --git a/tools/csr_usb.c b/tools/csr_usb.c
> index a1d7324f7..c5a39899a 100644
> --- a/tools/csr_usb.c
> +++ b/tools/csr_usb.c
> @@ -67,7 +67,7 @@ struct usbfs_bulktransfer {
> #define USBFS_IOCTL_CLAIMINTF  _IOR('U', 15, unsigned int)
> #define USBFS_IOCTL_RELEASEINTF        _IOR('U', 16, unsigned int)
> 
> -static int read_value(const char *name, const char *attr, const char *format)
> +static int read_value(const char *name, const char *attr, bool hex_number)
> {
>        char path[PATH_MAX];
>        FILE *file;
> @@ -79,7 +79,7 @@ static int read_value(const char *name, const char *attr, const char *format)
>        if (!file)
>                return -1;
> 
> -       n = fscanf(file, format, &value);
> +       n = fscanf(file, hex_number ? "%d" : "%04x", &value);
>        if (n != 1) {
>                fclose(file);
>                return -1;
> @@ -94,21 +94,21 @@ static char *check_device(const char *name)
>        char path[PATH_MAX];
>        int busnum, devnum, vendor, product;
> 
> -       busnum = read_value(name, "busnum", "%d");
> +       busnum = read_value(name, "busnum", false);
>        if (busnum < 0)
>                return NULL;
> 
> -       devnum = read_value(name, "devnum", "%d");
> +       devnum = read_value(name, "devnum", false);
>        if (devnum < 0)
>                return NULL;
> 
>        snprintf(path, sizeof(path), "/dev/bus/usb/%03u/%03u", busnum, devnum);
> 
> -       vendor = read_value(name, "idVendor", "%04x");
> +       vendor = read_value(name, "idVendor", true);
>        if (vendor < 0)
>                return NULL;
> 
> -       product = read_value(name, "idProduct", "%04x");
> +       product = read_value(name, "idProduct", true);
>        if (product < 0)
>                return NULL;

So something with your mailer went bogus here. It should be all proper tabs.

Anyhow, I am fine doing that, but I disliked the true or false parameters if they are not obvious. So maybe adding some like this would be good.

#define read_hex_value(name, file) read_value((name), (file), true)
#define read_num_value(name, file) read_value((name), (file), false)

Then the calling code becomes easy to understand again you don’t have to guess what the boolean value is for.

Regards

Marcel

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

On Tue, 2017-09-05 at 15:05 +0200, Marcel Holtmann wrote:
> Hi Bastien,
> 
> > > > GCC's "format-nonliteral" security check is enabled as an error
> > > > in
> > > > recent versions of Fedora. Given the reduced scope of use, mark
> > > > the
> > > > error as ignorable through pragma.
> > > > 
> > > > tools/csr_usb.c: In function 'read_value':
> > > > tools/csr_usb.c:82:2: error: format not a string literal,
> > > > argument
> > > > types not checked [-Werror=format-nonliteral]
> > > > n = fscanf(file, format, &value);
> > > > ^
> > > > ---
> > > > tools/csr_usb.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/tools/csr_usb.c b/tools/csr_usb.c
> > > > index a1d7324f7..33e9968a2 100644
> > > > --- a/tools/csr_usb.c
> > > > +++ b/tools/csr_usb.c
> > > > @@ -67,6 +67,8 @@ struct usbfs_bulktransfer {
> > > > #define USBFS_IOCTL_CLAIMINTF	_IOR('U', 15, unsigned
> > > > int)
> > > > #define USBFS_IOCTL_RELEASEINTF	_IOR('U', 16, unsigned
> > > > int)
> > > > 
> > > > +#pragma GCC diagnostic push
> > > > +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > > > static int read_value(const char *name, const char *attr, const
> > > > char *format)
> > > > {
> > > > 	char path[PATH_MAX];
> > > > @@ -88,6 +90,7 @@ static int read_value(const char *name, const
> > > > char *attr, const char *format)
> > > > 	fclose(file);
> > > > 	return value;
> > > > }
> > > > +#pragma GCC diagnostic pop
> > > 
> > > can’t we just use __attribute__((format)) somehow to declare this
> > > a
> > > format specify?
> > 
> > Sorry, I'm not sure I understand.
> > 
> > gcc already knows that this is a format, but because the format
> > changes
> > depending on the caller, it cannot actually assert whether the
> > format
> > is safe for the arguments passed.
> > 
> > So this would work:
> > vendor = read_value(name, "idVendor", "%04x");
> > 
> > But the compiler can't see that:
> > vendor = read_value(name, "idVendor", "%s");
> > would fail at compile time.
> 
> since these values are all marked as const, this compiler error is
> than pretty purely implemented. It could actually traverse this since
> they are const format strings.
> 
> > Would you prefer something like this (untested, but should work):
> > diff --git a/tools/csr_usb.c b/tools/csr_usb.c
> > index a1d7324f7..c5a39899a 100644
> > --- a/tools/csr_usb.c
> > +++ b/tools/csr_usb.c
> > @@ -67,7 +67,7 @@ struct usbfs_bulktransfer {
> > #define USBFS_IOCTL_CLAIMINTF  _IOR('U', 15, unsigned int)
> > #define USBFS_IOCTL_RELEASEINTF        _IOR('U', 16, unsigned int)
> > 
> > -static int read_value(const char *name, const char *attr, const
> > char *format)
> > +static int read_value(const char *name, const char *attr, bool
> > hex_number)
> > {
> >        char path[PATH_MAX];
> >        FILE *file;
> > @@ -79,7 +79,7 @@ static int read_value(const char *name, const
> > char *attr, const char *format)
> >        if (!file)
> >                return -1;
> > 
> > -       n = fscanf(file, format, &value);
> > +       n = fscanf(file, hex_number ? "%d" : "%04x", &value);
> >        if (n != 1) {
> >                fclose(file);
> >                return -1;
> > @@ -94,21 +94,21 @@ static char *check_device(const char *name)
> >        char path[PATH_MAX];
> >        int busnum, devnum, vendor, product;
> > 
> > -       busnum = read_value(name, "busnum", "%d");
> > +       busnum = read_value(name, "busnum", false);
> >        if (busnum < 0)
> >                return NULL;
> > 
> > -       devnum = read_value(name, "devnum", "%d");
> > +       devnum = read_value(name, "devnum", false);
> >        if (devnum < 0)
> >                return NULL;
> > 
> >        snprintf(path, sizeof(path), "/dev/bus/usb/%03u/%03u",
> > busnum, devnum);
> > 
> > -       vendor = read_value(name, "idVendor", "%04x");
> > +       vendor = read_value(name, "idVendor", true);
> >        if (vendor < 0)
> >                return NULL;
> > 
> > -       product = read_value(name, "idProduct", "%04x");
> > +       product = read_value(name, "idProduct", true);
> >        if (product < 0)
> >                return NULL;
> 
> So something with your mailer went bogus here. It should be all
> proper tabs.

I copy/pasted so the patch could be read, not to apply it. It wouldn't
have compiled (missing stdbool.h include).

I've sent an updated patch in v3.

[PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

GCC's "format-nonliteral" security check is enabled as an error in
recent versions of Fedora. Given the reduced scope of use, mark the
error as ignorable through pragma.

tools/csr_usb.c: In function 'read_value':
tools/csr_usb.c:82:2: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
  n = fscanf(file, format, &value);
  ^
---
 tools/csr_usb.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/tools/csr_usb.c b/tools/csr_usb.c
index a1d7324f7..f1ffb0086 100644
--- a/tools/csr_usb.c
+++ b/tools/csr_usb.c
@@ -31,6 +31,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <string.h>
 #include <dirent.h>
 #include <limits.h>
@@ -67,7 +68,7 @@ struct usbfs_bulktransfer {
 #define USBFS_IOCTL_CLAIMINTF	_IOR('U', 15, unsigned int)
 #define USBFS_IOCTL_RELEASEINTF	_IOR('U', 16, unsigned int)
 
-static int read_value(const char *name, const char *attr, const char *format)
+static int read_value(const char *name, const char *attr, bool hex_number)
 {
 	char path[PATH_MAX];
 	FILE *file;
@@ -79,7 +80,7 @@ static int read_value(const char *name, const char *attr, const char *format)
 	if (!file)
 		return -1;
 
-	n = fscanf(file, format, &value);
+	n = fscanf(file, hex_number ? "%d" : "%04x", &value);
 	if (n != 1) {
 		fclose(file);
 		return -1;
@@ -89,26 +90,29 @@ static int read_value(const char *name, const char *attr, const char *format)
 	return value;
 }
 
+#define read_hex_value(name, file) read_value((name), (file), true)
+#define read_num_value(name, file) read_value((name), (file), false)
+
 static char *check_device(const char *name)
 {
 	char path[PATH_MAX];
 	int busnum, devnum, vendor, product;
 
-	busnum = read_value(name, "busnum", "%d");
+	busnum = read_num_value(name, "busnum");
 	if (busnum < 0)
 		return NULL;
 
-	devnum = read_value(name, "devnum", "%d");
+	devnum = read_num_value(name, "devnum");
 	if (devnum < 0)
 		return NULL;
 
 	snprintf(path, sizeof(path), "/dev/bus/usb/%03u/%03u", busnum, devnum);
 
-	vendor = read_value(name, "idVendor", "%04x");
+	vendor = read_hex_value(name, "idVendor");
 	if (vendor < 0)
 		return NULL;
 
-	product = read_value(name, "idProduct", "%04x");
+	product = read_hex_value(name, "idProduct");
 	if (product < 0)
 		return NULL;
 
-- 
2.14.1

Re: [PATCH] tools/csr_usb: Fix compilation failure

[Bastien Nocera]

On Tue, 2017-09-05 at 16:32 +0200, Bastien Nocera wrote:
> GCC's "format-nonliteral" security check is enabled as an error in
> recent versions of Fedora. Given the reduced scope of use, mark the
> error as ignorable through pragma.

Please ignore, forgot to change the commit message.