[nft PATCH 1/2] parser_bison: Get rid of (most) bison compiler warnings

[nft PATCH 1/2] parser_bison: Get rid of (most) bison compiler warnings

[Phil Sutter]

Shut the complaints about POSIX incompatibility by passing -Wno-yacc to
bison. An alternative would be to not pass -y, but that caused seemingly
unsolveable problems with automake and expected file names.

Fix two warnings about deprecated '%pure-parser' and '%error-verbose'
statements by replacing them with what bison suggests.

A third warning sadly left in place: Replacing '%name-prefix' by what
is suggested leads to compilation errors.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/Makefile.am    | 2 +-
 src/parser_bison.y | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index e2b531390cefb..740c21f2cac85 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -22,7 +22,7 @@ AM_CFLAGS = -Wall								\
 	    -Waggregate-return -Wunused -Wwrite-strings ${GCC_FVISIBILITY_HIDDEN}
 
 
-AM_YFLAGS = -d
+AM_YFLAGS = -d -Wno-yacc
 
 BUILT_SOURCES = parser_bison.h
 
diff --git a/src/parser_bison.y b/src/parser_bison.y
index c90de47e88f74..12e499b4dd025 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -116,12 +116,12 @@ int nft_lex(void *, void *, void *);
 
 %name-prefix "nft_"
 %debug
-%pure-parser
+%define api.pure
 %parse-param		{ struct nft_ctx *nft }
 %parse-param		{ void *scanner }
 %parse-param		{ struct parser_state *state }
 %lex-param		{ scanner }
-%error-verbose
+%define parse.error verbose
 %locations
 
 %initial-action {
-- 
2.22.0

[nft PATCH 2/2] nfnl_osf: Silence string truncation gcc warnings

[Phil Sutter]

Albeit a bit too enthusiastic, gcc is right in that these strings may be
truncated since the destination buffer is smaller than the source one.
Get rid of the warnings (and the potential problem) by specifying a
string "precision" of one character less than the destination. This
ensures a terminating nul-character may be written as well.

Fixes: af00174af3ef4 ("src: osf: import nfnl_osf.c to load osf fingerprints")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/nfnl_osf.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c
index be3fd8100b665..bed9ba64b65c6 100644
--- a/src/nfnl_osf.c
+++ b/src/nfnl_osf.c
@@ -289,32 +289,34 @@ static int osf_load_line(char *buffer, int len, int del,
 	pend = nf_osf_strchr(pbeg, OSFPDEL);
 	if (pend) {
 		*pend = '\0';
-		cnt = snprintf(obuf, sizeof(obuf), "%s,", pbeg);
+		i = sizeof(obuf);
+		cnt = snprintf(obuf, i, "%.*s,", i - 2, pbeg);
 		pbeg = pend + 1;
 	}
 
 	pend = nf_osf_strchr(pbeg, OSFPDEL);
 	if (pend) {
 		*pend = '\0';
+		i = sizeof(f.genre);
 		if (pbeg[0] == '@' || pbeg[0] == '*')
-			cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg + 1);
-		else
-			cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg);
+			pbeg++;
+		cnt = snprintf(f.genre, i, "%.*s", i - 1, pbeg + 1);
 		pbeg = pend + 1;
 	}
 
 	pend = nf_osf_strchr(pbeg, OSFPDEL);
 	if (pend) {
 		*pend = '\0';
-		cnt = snprintf(f.version, sizeof(f.version), "%s", pbeg);
+		i = sizeof(f.version);
+		cnt = snprintf(f.version, i, "%.*s", i - 1, pbeg);
 		pbeg = pend + 1;
 	}
 
 	pend = nf_osf_strchr(pbeg, OSFPDEL);
 	if (pend) {
 		*pend = '\0';
-		cnt =
-		    snprintf(f.subtype, sizeof(f.subtype), "%s", pbeg);
+		i = sizeof(f.subtype);
+		cnt = snprintf(f.subtype, i, "%.*s", i - 1, pbeg);
 		pbeg = pend + 1;
 	}
 
-- 
2.22.0

Re: [nft PATCH 2/2] nfnl_osf: Silence string truncation gcc warnings

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Albeit a bit too enthusiastic, gcc is right in that these strings may be
> truncated since the destination buffer is smaller than the source one.
> Get rid of the warnings (and the potential problem) by specifying a
> string "precision" of one character less than the destination. This
> ensures a terminating nul-character may be written as well.

Fernando sent a patch for this already, with the notable difference
of altering the size of the destination buffer by one.

Re: [nft PATCH 1/2] parser_bison: Get rid of (most) bison compiler warnings

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Shut the complaints about POSIX incompatibility by passing -Wno-yacc to
> bison. An alternative would be to not pass -y, but that caused seemingly
> unsolveable problems with automake and expected file names.
> 
> Fix two warnings about deprecated '%pure-parser' and '%error-verbose'
> statements by replacing them with what bison suggests.
> 
> A third warning sadly left in place: Replacing '%name-prefix' by what
> is suggested leads to compilation errors.

Can you add those warnings to the changelog before pushing?

I don't see them, even without this patch.

Re: [nft PATCH 1/2] parser_bison: Get rid of (most) bison compiler warnings

[Phil Sutter]

On Sat, Jul 20, 2019 at 09:43:22PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Shut the complaints about POSIX incompatibility by passing -Wno-yacc to
> > bison. An alternative would be to not pass -y, but that caused seemingly
> > unsolveable problems with automake and expected file names.
> > 
> > Fix two warnings about deprecated '%pure-parser' and '%error-verbose'
> > statements by replacing them with what bison suggests.
> > 
> > A third warning sadly left in place: Replacing '%name-prefix' by what
> > is suggested leads to compilation errors.
> 
> Can you add those warnings to the changelog before pushing?

Sure!

> I don't see them, even without this patch.

I started seeing them just recently, maybe after the last bison update
(to 3.4.1).

Cheers, Phil

Re: [nft PATCH 2/2] nfnl_osf: Silence string truncation gcc warnings

[Phil Sutter]

On Sat, Jul 20, 2019 at 09:36:28PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Albeit a bit too enthusiastic, gcc is right in that these strings may be
> > truncated since the destination buffer is smaller than the source one.
> > Get rid of the warnings (and the potential problem) by specifying a
> > string "precision" of one character less than the destination. This
> > ensures a terminating nul-character may be written as well.
> 
> Fernando sent a patch for this already, with the notable difference
> of altering the size of the destination buffer by one.

Ah, thanks! I missed it. Replied to it pointing at my patch, also
spotted a typo in his patch. :)

Cheers, Phil

Re: [nft PATCH 2/2] nfnl_osf: Silence string truncation gcc warnings

[Fernando Fernandez Mancera]

Hi Phil,

I am porting your changes to my patch and I have few comments, please
see below.

On 7/20/19 8:52 PM, Phil Sutter wrote:
> Albeit a bit too enthusiastic, gcc is right in that these strings may be
> truncated since the destination buffer is smaller than the source one.
> Get rid of the warnings (and the potential problem) by specifying a
> string "precision" of one character less than the destination. This
> ensures a terminating nul-character may be written as well.
> 
> Fixes: af00174af3ef4 ("src: osf: import nfnl_osf.c to load osf fingerprints")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/nfnl_osf.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c
> index be3fd8100b665..bed9ba64b65c6 100644
> --- a/src/nfnl_osf.c
> +++ b/src/nfnl_osf.c
> @@ -289,32 +289,34 @@ static int osf_load_line(char *buffer, int len, int del,
>  	pend = nf_osf_strchr(pbeg, OSFPDEL);
>  	if (pend) {
>  		*pend = '\0';
> -		cnt = snprintf(obuf, sizeof(obuf), "%s,", pbeg);
> +		i = sizeof(obuf);
> +		cnt = snprintf(obuf, i, "%.*s,", i - 2, pbeg);
>  		pbeg = pend + 1;
>  	}
>  
>  	pend = nf_osf_strchr(pbeg, OSFPDEL);
>  	if (pend) {
>  		*pend = '\0';
> +		i = sizeof(f.genre);
>  		if (pbeg[0] == '@' || pbeg[0] == '*')
> -			cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg + 1);
> -		else
> -			cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg);
> +			pbeg++;
> +		cnt = snprintf(f.genre, i, "%.*s", i - 1, pbeg + 1);
>  		pbeg = pend + 1;
>  	}

I am not including this because the pbeg pointer is being modified if
the condition is true which is not what we want. Note that pbeg is being
used below. Also, we cannot do pbeg++ and at the same time shift the
pointer passed to snprintf with pbeg + 1.

I propose to let the if statement as it is and modify only the snprintf().

What do you think? Am I missing something here? Thanks Phil!

>  
>  	pend = nf_osf_strchr(pbeg, OSFPDEL);
>  	if (pend) {
>  		*pend = '\0';
> -		cnt = snprintf(f.version, sizeof(f.version), "%s", pbeg);
> +		i = sizeof(f.version);
> +		cnt = snprintf(f.version, i, "%.*s", i - 1, pbeg);
>  		pbeg = pend + 1;
>  	}
>  
>  	pend = nf_osf_strchr(pbeg, OSFPDEL);
>  	if (pend) {
>  		*pend = '\0';
> -		cnt =
> -		    snprintf(f.subtype, sizeof(f.subtype), "%s", pbeg);
> +		i = sizeof(f.subtype);
> +		cnt = snprintf(f.subtype, i, "%.*s", i - 1, pbeg);
>  		pbeg = pend + 1;
>  	}
>  
> 

Re: [nft PATCH 2/2] nfnl_osf: Silence string truncation gcc warnings

[Phil Sutter]

Hi,

On Sun, Jul 21, 2019 at 01:15:58PM +0200, Fernando Fernandez Mancera wrote:
[...]
> >  	pend = nf_osf_strchr(pbeg, OSFPDEL);
> >  	if (pend) {
> >  		*pend = '\0';
> > +		i = sizeof(f.genre);
> >  		if (pbeg[0] == '@' || pbeg[0] == '*')
> > -			cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg + 1);
> > -		else
> > -			cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg);
> > +			pbeg++;
> > +		cnt = snprintf(f.genre, i, "%.*s", i - 1, pbeg + 1);
> >  		pbeg = pend + 1;
> >  	}
> 
> I am not including this because the pbeg pointer is being modified if
> the condition is true which is not what we want. Note that pbeg is being
> used below. Also, we cannot do pbeg++ and at the same time shift the
> pointer passed to snprintf with pbeg + 1.

Oh, sorry that 'pbeg + 1' in my added code is a bug. I guess
incrementing pbeg if it starts with @ or * is fine because after the
call to snprintf() it is reset ('pbeg = pend + 1') without reusing its
old value.

Cheers, Phil