[libnftnl PATCH] internal: fix SNPRINTF_BUFFER_SIZE macro

[libnftnl PATCH] internal: fix SNPRINTF_BUFFER_SIZE macro

[Arturo Borrero Gonzalez]

We need to store in 'offset' the complete amount of characters as returned
from _snprintf. The value means how many characters long needs the buffer to be
in order to store the corresponding string expansion.

Before this patch, in cases where the buffer is smaller than the
expansion, then ret > len, and therefore ret = len.
So when incrementing offset, we do it with a wrong value.

All previous versions of libnftnl are unable to handle this situations: small
buffers (or long string expansion).

BTW, if a caller must reallocate a buffer to the returned value of snprintf, it
should be ret + 1.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/internal.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/internal.h b/src/internal.h
index 6595e70..43d61ca 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -184,9 +184,9 @@ struct nft_set_elem {
 
 #define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)	\
 	size += ret;					\
+	offset += ret;					\
 	if (ret > len)					\
 		ret = len;				\
-	offset += ret;					\
 	len -= ret;
 
 #define div_round_up(n, d)	(((n) + (d) - 1) / (d))

Re: [libnftnl PATCH] internal: fix SNPRINTF_BUFFER_SIZE macro

[Arturo Borrero Gonzalez]

On 9 May 2014 13:58, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> We need to store in 'offset' the complete amount of characters as returned
> from _snprintf. The value means how many characters long needs the buffer to be
> in order to store the corresponding string expansion.
>
> Before this patch, in cases where the buffer is smaller than the
> expansion, then ret > len, and therefore ret = len.
> So when incrementing offset, we do it with a wrong value.
>
> All previous versions of libnftnl are unable to handle this situations: small
> buffers (or long string expansion).
>
> BTW, if a caller must reallocate a buffer to the returned value of snprintf, it
> should be ret + 1.
>

About this issue:

We use mostly fprintf() functions in our test infrastructure. While
_fprintf() functions use internally _snprintf() [0] (thus, allocating
a 4096 sized buffer in the stack), this is done *per object*. A single
object hardly can contain more than 4096 characters when
string-expanded (either JSON or XML).

Also, this problem arises strongly with a big nft_ruleset printed with
nft_ruleset_snprintf(), because there no single buffer is allocated
per object. OTOH, printing the same nft_ruleset with
nft_ruleset_fprintf() will result again in allocating single buffers
per object [1].

Regarding the reallocation [2] of ret + 1, that fix will come in a
different patch. The example provided in the snprintf(3) manpage also
reallocs with +1, as snprintf adds a trailing \0.

regards.

[0] http://git.netfilter.org/libnftnl/tree/src/utils.c#n188
[1] http://git.netfilter.org/libnftnl/tree/src/ruleset.c#n970
[2] http://git.netfilter.org/libnftnl/tree/src/utils.c#n199
-- 
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] internal: fix SNPRINTF_BUFFER_SIZE macro

[Pablo Neira Ayuso]

On Fri, May 09, 2014 at 01:58:06PM +0200, Arturo Borrero Gonzalez wrote:
> We need to store in 'offset' the complete amount of characters as returned
> from _snprintf. The value means how many characters long needs the buffer to be
> in order to store the corresponding string expansion.
> 
> Before this patch, in cases where the buffer is smaller than the
> expansion, then ret > len, and therefore ret = len.
> So when incrementing offset, we do it with a wrong value.
> 
> All previous versions of libnftnl are unable to handle this situations: small
> buffers (or long string expansion).
> 
> BTW, if a caller must reallocate a buffer to the returned value of snprintf, it
> should be ret + 1.

Thanks for looking into this.

> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  src/internal.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/internal.h b/src/internal.h
> index 6595e70..43d61ca 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -184,9 +184,9 @@ struct nft_set_elem {
>  
>  #define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)	\

We should also check if (ret < 0) here in first place, in that case
return ret.

>  	size += ret;					\

You have to move this line above where offset += len was previously.

> +	offset += ret;					\
>  	if (ret > len)					\
>  		ret = len;				\
> -	offset += ret;					\
>  	len -= ret;

After those changes, this macro looks good to me.