[PATCH 1/2] tools/csr_usb: Fix compilation failure

[PATCH 1/2] 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 limited formats, pass a boolean
to switch between the 2 different formats.

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.3

[PATCH 2/2] obex: Work-around compilation failure

[Bastien Nocera]

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);
       ^~~~~~~
---
 obexd/plugins/bluetooth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
index 3ee54325f..3e31eaef6 100644
--- a/obexd/plugins/bluetooth.c
+++ b/obexd/plugins/bluetooth.c
@@ -302,6 +302,9 @@ static int register_profile(struct bluetooth_profile *profile)
 					&opt);
 	dict_append_entry(&opt, "AutoConnect", DBUS_TYPE_BOOLEAN,
 								&auto_connect);
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
 	if (profile->driver->record) {
 		if (profile->driver->port != 0)
 			xml = g_markup_printf_escaped(profile->driver->record,
@@ -312,6 +315,7 @@ static int register_profile(struct bluetooth_profile *profile)
 			xml = g_markup_printf_escaped(profile->driver->record,
 						profile->driver->channel,
 						profile->driver->name);
+#pragma GCC diagnostic pop
 		dict_append_entry(&opt, "ServiceRecord", DBUS_TYPE_STRING,
 								&xml);
 		g_free(xml);
-- 
2.14.3

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

[Bastien Nocera]

On Thu, 2017-11-02 at 15:09 +0100, Bastien Nocera wrote:
> GCC's "format-nonliteral" security check is enabled as an error in
> recent versions of Fedora. Given the limited formats, pass a boolean
> to switch between the 2 different formats.
> 
> 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);
>   ^

Sorry, those are re-send because of a command-line typo. They're still
applicable though.

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

[Bastien Nocera]

On Thu, 2017-11-02 at 15:09 +0100, Bastien Nocera wrote:
> GCC's "format-nonliteral" security check is enabled as an error in
> recent versions of Fedora. Given the limited formats, pass a boolean
> to switch between the 2 different formats.
> 
> 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);
>   ^

This still applies.

Re: [PATCH 2/2] obex: Work-around compilation failure

[Bastien Nocera]

On Thu, 2017-11-02 at 15:09 +0100, Bastien Nocera wrote:
> 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);
>        ^~~~~~~


And so does this one.

Re: [PATCH 2/2] obex: Work-around compilation failure

[Luiz Augusto von Dentz]

Hi Bastien,

On Fri, Nov 24, 2017 at 4:06 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2017-11-02 at 15:09 +0100, Bastien Nocera wrote:
>> 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);
>>        ^~~~~~~
>
>
> And so does this one.

I really hope we can avoid GCC specific pragmas, there got to be a
better way avoid this error or just disable
-Werror=format-nonliteral, actually is that our error or
g_markup_printf_escaped?

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] obex: Work-around compilation failure

[Bastien Nocera]

On Fri, 2017-11-24 at 16:48 +0200, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Fri, Nov 24, 2017 at 4:06 PM, Bastien Nocera <hadess@hadess.net>
> wrote:
> > On Thu, 2017-11-02 at 15:09 +0100, Bastien Nocera wrote:
> > > 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);
> > >        ^~~~~~~
> > 
> > 
> > And so does this one.
> 
> I really hope we can avoid GCC specific pragmas,

I'm pretty sure it just won't do anything with other compilers.

>  there got to be a
> better way avoid this error

A lot of code refactoring.

>  or just disable
> -Werror=format-nonliteral, actually is that our error or
> g_markup_printf_escaped?

It's the g_markup_printf_escaped() usage that incorrect. The first
argument of printf-style functions are supposed to be literals,
otherwise the compiler doesn't know whether the arguments passed match
the format.

It's a potential security bug, depending on where the format comes
from, and how it's generated.

Removing the warning/error doesn't remove the bug, it just hides it.
And so does this patch. I don't know the codebase enough to refactor
it.

Cheers