[libnftnl PATCH] utils: fix buffer reallocation of nft_fprinft()

[libnftnl PATCH] utils: fix buffer reallocation of nft_fprinft()

[Arturo Borrero Gonzalez]

When _snprintf() reports it would print n characters, that n doesn't include
the trailing \0 that snprintf adds.

Thus, we need to [re]allocate n+1 characters.

While at it, change the reallocation trigger. If the length of the buffer we
used is equals to the expanded string length, the output has been truncated.
In other words, if ret == bufsiz, then the trailing \0 is missing.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/utils.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/utils.c b/src/utils.c
index 18917f5..b8094aa 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -195,12 +195,13 @@ int nft_fprintf(FILE *fp, void *obj, uint32_t type, uint32_t flags,
 	int ret;
 
 	ret = snprintf_cb(buf, bufsiz, obj, type, flags);
-	if (ret > NFT_SNPRINTF_BUFSIZ) {
-		buf = calloc(1, ret);
+	if (ret >= NFT_SNPRINTF_BUFSIZ) {
+		bufsiz = ret + 1;
+
+		buf = calloc(1, bufsiz);
 		if (buf == NULL)
 			return -1;
 
-		bufsiz = ret;
 		ret = snprintf_cb(buf, bufsiz, obj, type, flags);
 	}
 

Re: [libnftnl PATCH] utils: fix buffer reallocation of nft_fprinft()

[Pablo Neira Ayuso]

Hi Arturo,

On Fri, May 09, 2014 at 06:45:47PM +0200, Arturo Borrero Gonzalez wrote:
> When _snprintf() reports it would print n characters, that n doesn't include
> the trailing \0 that snprintf adds.
> 
> Thus, we need to [re]allocate n+1 characters.
> 
> While at it, change the reallocation trigger. If the length of the buffer we
> used is equals to the expanded string length, the output has been truncated.
> In other words, if ret == bufsiz, then the trailing \0 is missing.
> 
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  src/utils.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/utils.c b/src/utils.c
> index 18917f5..b8094aa 100644
> --- a/src/utils.c
> +++ b/src/utils.c
> @@ -195,12 +195,13 @@ int nft_fprintf(FILE *fp, void *obj, uint32_t type, uint32_t flags,
>  	int ret;
>  
>  	ret = snprintf_cb(buf, bufsiz, obj, type, flags);

I think we should also check here if snprintf returns -1 now that you
have fixed the SNPRINTF_ macro.

> -	if (ret > NFT_SNPRINTF_BUFSIZ) {
> -		buf = calloc(1, ret);
> +	if (ret >= NFT_SNPRINTF_BUFSIZ) {
> +		bufsiz = ret + 1;
> +
> +		buf = calloc(1, bufsiz);

You can use malloc instead. Just make sure that the string is always
nul-terminated before printing, something like:

                bufsiz = ret + 1;
                buf = malloc(1, bufsiz);
                if (buf == NULL)
                        return -1;

                ret = snprintf(...
                if (ret < 0)
                        ...
        }

        buf[ret] = '\0';
        ... = fprintf(...

Thanks.

Re: [libnftnl PATCH] utils: fix buffer reallocation of nft_fprinft()

[Arturo Borrero Gonzalez]

On 12 May 2014 17:54, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> You can use malloc instead. Just make sure that the string is always
> nul-terminated before printing, something like:
>
>                 bufsiz = ret + 1;
>                 buf = malloc(1, bufsiz);
>                 if (buf == NULL)
>                         return -1;
>
>                 ret = snprintf(...
>                 if (ret < 0)
>                         ...
>         }
>
>         buf[ret] = '\0';
>         ... = fprintf(...
>

From my man pages, I understand that snprintf() null-terminate the
string. This seems a bit redundant.
I think it's safe to don't include calloc() neither buf[ret] = '\0'.

I'm resending this patch with this assumption and your other request.

regards.

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [libnftnl PATCH] utils: fix buffer reallocation of nft_fprinft()

[Pablo Neira Ayuso]

On Tue, May 13, 2014 at 11:11:24AM +0200, Arturo Borrero Gonzalez wrote:
> On 12 May 2014 17:54, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > You can use malloc instead. Just make sure that the string is always
> > nul-terminated before printing, something like:
> >
> >                 bufsiz = ret + 1;
> >                 buf = malloc(1, bufsiz);
> >                 if (buf == NULL)
> >                         return -1;
> >
> >                 ret = snprintf(...
> >                 if (ret < 0)
> >                         ...
> >         }
> >
> >         buf[ret] = '\0';
> >         ... = fprintf(...
> >
> 
> From my man pages, I understand that snprintf() null-terminate the
> string. This seems a bit redundant.

If the amount of written bytes is smaller than the buffer size,
snprintf always nul-terminate it. But if the amount of bytes returned
equals the buffer size, then you have to explicitly nul-terminate the
string.

> I think it's safe to don't include calloc() neither buf[ret] = '\0'.

Right, if you use a buffer size of ret + 1, then you can guarantee
that snprintf always have room to append the nul-termination, as the
returned value will be ret at maximum.

> I'm resending this patch with this assumption and your other request.

OK, thanks.