| Message ID | 20140714124335.13205.77982.stgit@nfdev.cica.es |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
On Mon, Jul 14, 2014 at 02:43:35PM +0200, Arturo Borrero Gonzalez wrote: > Previous to this patch, the code path can potentially print an empty > buffer with the \0 at the end of the buffer. > > Be more strict and place the \0 character in the first position if the > buffer is empty. > > Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> > --- > src/common.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/common.c b/src/common.c > index 1b600f1..c81a1d8 100644 > --- a/src/common.c > +++ b/src/common.c > @@ -114,9 +114,10 @@ int nft_event_header_snprintf(char *buf, size_t size, uint32_t type, > int nft_event_header_fprintf(FILE *fp, uint32_t type, uint32_t flags) > { > char buf[64]; /* enough for the maximum string length above */ > + int ret; > > - nft_event_header_snprintf(buf, sizeof(buf), type, flags); > - buf[sizeof(buf) - 1] = '\0'; > + ret = nft_event_header_snprintf(buf, sizeof(buf), type, flags); > + buf[ret] = '\0'; ret can be 64 at worst case, then you have ret[64] would be an off-by-one memory access (out of bound). BTW, could you review all_snprintf functions? We should retain the snprintf semantics, ie. always nul-terminate strings if offset < buffer_size Otherwise, return the string without the nul-termination. -- 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
On 15 July 2014 18:42, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > BTW, could you review all_snprintf functions? We should retain the > snprintf semantics, ie. always nul-terminate strings if > > offset < buffer_size > > Otherwise, return the string without the nul-termination. Sure, thanks.
diff --git a/src/common.c b/src/common.c index 1b600f1..c81a1d8 100644 --- a/src/common.c +++ b/src/common.c @@ -114,9 +114,10 @@ int nft_event_header_snprintf(char *buf, size_t size, uint32_t type, int nft_event_header_fprintf(FILE *fp, uint32_t type, uint32_t flags) { char buf[64]; /* enough for the maximum string length above */ + int ret; - nft_event_header_snprintf(buf, sizeof(buf), type, flags); - buf[sizeof(buf) - 1] = '\0'; + ret = nft_event_header_snprintf(buf, sizeof(buf), type, flags); + buf[ret] = '\0'; return fprintf(fp, "%s", buf); } @@ -140,9 +141,10 @@ int nft_event_footer_snprintf(char *buf, size_t size, uint32_t type, int nft_event_footer_fprintf(FILE *fp, uint32_t type, uint32_t flags) { char buf[32]; /* enough for the maximum string length above */ + int ret; - nft_event_footer_snprintf(buf, sizeof(buf), type, flags); - buf[sizeof(buf) - 1] = '\0'; + ret = nft_event_footer_snprintf(buf, sizeof(buf), type, flags); + buf[ret] = '\0'; return fprintf(fp, "%s", buf); }
Previous to this patch, the code path can potentially print an empty buffer with the \0 at the end of the buffer. Be more strict and place the \0 character in the first position if the buffer is empty. Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> --- src/common.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) -- 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