[PATCH 1/3] build: Add warnings for non-literal strings

[PATCH 1/3] build: Add warnings for non-literal strings

[Bastien Nocera]

---
 acinclude.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 529848357..6ae34b8ae 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -21,7 +21,7 @@ AC_DEFUN([COMPILER_FLAGS], [
 		with_cflags="$with_cflags -Wredundant-decls"
 		with_cflags="$with_cflags -Wcast-align"
 		with_cflags="$with_cflags -Wswitch-enum"
-		with_cflags="$with_cflags -Wformat -Wformat-security"
+		with_cflags="$with_cflags -Wformat -Wformat-security -Wformat-nonliteral"
 		with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
 		with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
 		with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
-- 
2.29.2

[PATCH 2/3] 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 d232d3fd5..66f432d66 100644
--- a/obexd/plugins/bluetooth.c
+++ b/obexd/plugins/bluetooth.c
@@ -258,6 +258,9 @@ static int register_profile(struct bluetooth_profile *profile)
 					&opt);
 	g_dbus_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,
@@ -268,6 +271,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
 		g_dbus_dict_append_entry(&opt, "ServiceRecord",
 						DBUS_TYPE_STRING, &xml);
 		g_free(xml);
-- 
2.29.2

[PATCH 3/3] tools/mesh-cfglient: Work-around compilation failure

[Bastien Nocera]

tools/mesh-cfgclient.c: In function ‘disp_numeric_call’:
tools/mesh-cfgclient.c:543:10: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
  543 |          n);
      |          ^
---
 tools/mesh-cfgclient.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c
index 1eeed2a1a..ab5026e9b 100644
--- a/tools/mesh-cfgclient.c
+++ b/tools/mesh-cfgclient.c
@@ -530,8 +530,11 @@ static struct l_dbus_message *disp_numeric_call(struct l_dbus *dbus,
 	if (action_index < 0)
 		return l_dbus_message_new_error(msg, dbus_err_support, NULL);
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
 	str = l_strdup_printf(display_numeric_table[action_index].description,
 									n);
+#pragma GCC diagnostic pop
 	bt_shell_printf(COLOR_YELLOW "%s\n" COLOR_OFF, str);
 	l_free(str);
 
-- 
2.29.2

RE: [1/3] build: Add warnings for non-literal strings

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=442025

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - FAIL
Output:
build: Add warnings for non-literal strings
3: B6 Body message is missing

obex: Work-around compilation failure
4: B1 Line exceeds max length (123>80): "obexd/plugins/bluetooth.c:310:7: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]"
7: B1 Line exceeds max length (123>80): "obexd/plugins/bluetooth.c:314:7: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]"

tools/mesh-cfglient: Work-around compilation failure
4: B1 Line exceeds max length (121>80): "tools/mesh-cfgclient.c:543:10: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]"


##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

Re: [PATCH 1/3] build: Add warnings for non-literal strings

[Luiz Augusto von Dentz]

Hi Bastien,

On Thu, Mar 4, 2021 at 9:21 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> ---
>  acinclude.m4 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 529848357..6ae34b8ae 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -21,7 +21,7 @@ AC_DEFUN([COMPILER_FLAGS], [
>                 with_cflags="$with_cflags -Wredundant-decls"
>                 with_cflags="$with_cflags -Wcast-align"
>                 with_cflags="$with_cflags -Wswitch-enum"
> -               with_cflags="$with_cflags -Wformat -Wformat-security"
> +               with_cflags="$with_cflags -Wformat -Wformat-security -Wformat-nonliteral"

Does it actually have any benefit of having the format as always
string literal? I'm not really a big fan of using pragmas.

>                 with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
>                 with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
>                 with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> --
> 2.29.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] build: Add warnings for non-literal strings

[Bastien Nocera]

On Thu, 2021-03-04 at 10:35 -0800, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Thu, Mar 4, 2021 at 9:21 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > ---
> >  acinclude.m4 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index 529848357..6ae34b8ae 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -21,7 +21,7 @@ AC_DEFUN([COMPILER_FLAGS], [
> >                 with_cflags="$with_cflags -Wredundant-decls"
> >                 with_cflags="$with_cflags -Wcast-align"
> >                 with_cflags="$with_cflags -Wswitch-enum"
> > -               with_cflags="$with_cflags -Wformat -Wformat-security"
> > +               with_cflags="$with_cflags -Wformat -Wformat-security
> > -Wformat-nonliteral"
> 
> Does it actually have any benefit of having the format as always
> string literal? I'm not really a big fan of using pragmas.

It's a security feature[1], so it's pretty important that we avoid
using non-literals when some of the arguments are user controlled,
especially in a networked daemon. We already enabled
"-Wformat-security", so not that much of a difference.

This warning is also enabled by default on Fedora's GCC, so I get to
see it whether I want to or not.

I'd be happy actually fixing those warnings if you don't want pragmas
at all, it would just be more code movement. If we can get those
patches in, I can do a follow-up.

[1]: Quick search gave me this explanation:
https://owasp.org/www-community/attacks/Format_string_attack

> >                 with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
> >                 with_cflags="$with_cflags -
> > DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
> >                 with_cflags="$with_cflags -
> > DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> > --
> > 2.29.2
> > 
> 
> 

Re: [PATCH 1/3] build: Add warnings for non-literal strings

[Luiz Augusto von Dentz]

Hi Bastien,

On Thu, Mar 4, 2021 at 10:46 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2021-03-04 at 10:35 -0800, Luiz Augusto von Dentz wrote:
> > Hi Bastien,
> >
> > On Thu, Mar 4, 2021 at 9:21 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > ---
> > >  acinclude.m4 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/acinclude.m4 b/acinclude.m4
> > > index 529848357..6ae34b8ae 100644
> > > --- a/acinclude.m4
> > > +++ b/acinclude.m4
> > > @@ -21,7 +21,7 @@ AC_DEFUN([COMPILER_FLAGS], [
> > >                 with_cflags="$with_cflags -Wredundant-decls"
> > >                 with_cflags="$with_cflags -Wcast-align"
> > >                 with_cflags="$with_cflags -Wswitch-enum"
> > > -               with_cflags="$with_cflags -Wformat -Wformat-security"
> > > +               with_cflags="$with_cflags -Wformat -Wformat-security
> > > -Wformat-nonliteral"
> >
> > Does it actually have any benefit of having the format as always
> > string literal? I'm not really a big fan of using pragmas.
>
> It's a security feature[1], so it's pretty important that we avoid
> using non-literals when some of the arguments are user controlled,
> especially in a networked daemon. We already enabled
> "-Wformat-security", so not that much of a difference.
>
> This warning is also enabled by default on Fedora's GCC, so I get to
> see it whether I want to or not.
>
> I'd be happy actually fixing those warnings if you don't want pragmas
> at all, it would just be more code movement. If we can get those
> patches in, I can do a follow-up.
>
> [1]: Quick search gave me this explanation:
> https://owasp.org/www-community/attacks/Format_string_attack

You should probably add the above link in the patch description,
regarding the use of pragma. I'd say we need to convert to use
literals directly then since otherwise we are not actually fixing
anything just returning it back to ignore the error where we don't use
literals.

> > >                 with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
> > >                 with_cflags="$with_cflags -
> > > DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
> > >                 with_cflags="$with_cflags -
> > > DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> > > --
> > > 2.29.2
> > >
> >
> >
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] build: Add warnings for non-literal strings

[Bastien Nocera]

On Thu, 2021-03-04 at 10:54 -0800, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Thu, Mar 4, 2021 at 10:46 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Thu, 2021-03-04 at 10:35 -0800, Luiz Augusto von Dentz wrote:
> > > Hi Bastien,
> > > 
> > > On Thu, Mar 4, 2021 at 9:21 AM Bastien Nocera <hadess@hadess.net>
> > > wrote:
> > > > 
> > > > ---
> > > >  acinclude.m4 | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/acinclude.m4 b/acinclude.m4
> > > > index 529848357..6ae34b8ae 100644
> > > > --- a/acinclude.m4
> > > > +++ b/acinclude.m4
> > > > @@ -21,7 +21,7 @@ AC_DEFUN([COMPILER_FLAGS], [
> > > >                 with_cflags="$with_cflags -Wredundant-decls"
> > > >                 with_cflags="$with_cflags -Wcast-align"
> > > >                 with_cflags="$with_cflags -Wswitch-enum"
> > > > -               with_cflags="$with_cflags -Wformat -Wformat-
> > > > security"
> > > > +               with_cflags="$with_cflags -Wformat -Wformat-
> > > > security
> > > > -Wformat-nonliteral"
> > > 
> > > Does it actually have any benefit of having the format as always
> > > string literal? I'm not really a big fan of using pragmas.
> > 
> > It's a security feature[1], so it's pretty important that we avoid
> > using non-literals when some of the arguments are user controlled,
> > especially in a networked daemon. We already enabled
> > "-Wformat-security", so not that much of a difference.
> > 
> > This warning is also enabled by default on Fedora's GCC, so I get
> > to
> > see it whether I want to or not.
> > 
> > I'd be happy actually fixing those warnings if you don't want
> > pragmas
> > at all, it would just be more code movement. If we can get those
> > patches in, I can do a follow-up.
> > 
> > [1]: Quick search gave me this explanation:
> > https://owasp.org/www-community/attacks/Format_string_attack
> 
> You should probably add the above link in the patch description,
> regarding the use of pragma. I'd say we need to convert to use
> literals directly then since otherwise we are not actually fixing
> anything

We're presumably stopping new non-literals from being introduced...

As I mentioned, I can do a follow-up, but I'm not going to do the work
until this patch series is merged. I've sent it a number of times
already and after 4 years, I'm not sure I want to do the work again
only for it to rot in my repo.

>  just returning it back to ignore the error where we don't use
> literals.
> 
> > > >                 with_cflags="$with_cflags -
> > > > DG_DISABLE_DEPRECATED"
> > > >                 with_cflags="$with_cflags -
> > > > DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
> > > >                 with_cflags="$with_cflags -
> > > > DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> > > > --
> > > > 2.29.2
> > > > 
> > > 
> > > 
> > 
> > 
> 
>