[nft,2/2] nfnl_osf: Silence string truncation gcc warnings
diff mbox series

Message ID 20190720185226.8876-2-phil@nwl.cc
State Superseded
Delegated to: Pablo Neira
Headers show
Series
  • [nft,1/2] parser_bison: Get rid of (most) bison compiler warnings
Related show

Commit Message

Phil Sutter July 20, 2019, 6:52 p.m. UTC
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(-)

Comments

Florian Westphal July 20, 2019, 7:36 p.m. UTC | #1
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.
Phil Sutter July 20, 2019, 8:22 p.m. UTC | #2
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
Fernando Fernandez Mancera July 21, 2019, 11:15 a.m. UTC | #3
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;
>  	}
>  
>
Phil Sutter July 21, 2019, 11:29 a.m. UTC | #4
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

Patch
diff mbox series

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;
 	}