internal: fix SNPRINTF_BUFFER_SIZE macro
PATCH 1/1 internal: fix SNPRINTF_BUFFER_SIZE macro
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.
--- src/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
src/internal.h +1 -1
@@ -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))
Quoted 8 lines · 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.
Quoted 6 lines
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
Signature · 5 lines
Arturo Borrero González -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso
maintainerQuoted 8 lines · 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.
Quoted 5 lines
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.
src/internal.h struct nft_set_elem { · line 185
Quoted 12 lines
Signed-off-by Arturo Borrero Gonzalez <[email protected]>--- src/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)src/internal.h +0
-0@@ -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.
src/internal.h struct nft_set_elem { · line 186
Quoted 1 line
src/internal.h +0
-0size += ret; \
You have to move this line above where offset += len was previously.
src/internal.h struct nft_set_elem { · line 190
Quoted 5 lines
src/internal.h +1
-1+ offset += ret; \if (ret > len) \ ret = len; \ - offset += ret; \ len -= ret;
After those changes, this macro looks good to me.