* Re: [dpdk-dev] [PATCH] test: fix sprintf with snprintf
2019-02-08 13:27 [dpdk-dev] [PATCH] test: fix sprintf with snprintf Pallantla Poornima
@ 2019-02-08 14:04 ` Bruce Richardson
2019-02-08 16:21 ` Stephen Hemminger
2019-02-08 17:04 ` Aaron Conole
2 siblings, 0 replies; 5+ messages in thread
From: Bruce Richardson @ 2019-02-08 14:04 UTC (permalink / raw)
To: Pallantla Poornima; +Cc: dev, reshma.pattan, ferruh.yigit, stable
On Fri, Feb 08, 2019 at 01:27:37PM +0000, Pallantla Poornima wrote:
> sprintf function is not secure as it doesn't check the length of string.
> More secure function snprintf is used.
>
> Fixes: 727909c592 ("app/test: introduce dynamic commands list")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
> ---
> test/test/commands.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/test/test/commands.c b/test/test/commands.c
> index 94fbc310e..5aeb35498 100644
> --- a/test/test/commands.c
> +++ b/test/test/commands.c
> @@ -367,6 +367,8 @@ int commands_init(void)
> struct test_command *t;
> char *commands, *ptr;
> int commands_len = 0;
> + int total_written = 0;
> + int count = 0;
>
> TAILQ_FOREACH(t, &commands_list, next) {
> commands_len += strlen(t->command) + 1;
> @@ -378,7 +380,10 @@ int commands_init(void)
>
> ptr = commands;
> TAILQ_FOREACH(t, &commands_list, next) {
> - ptr += sprintf(ptr, "%s#", t->command);
> + count = snprintf(ptr, commands_len - total_written - 1, "%s#",
> + t->command);
> + ptr += count;
> + total_written += count;
> }
I don't think the "-1" should be necessary here. Also, I think you should
check the return value of snprintf to check for truncation, and abort the
loop if so.
/Bruce
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [dpdk-dev] [PATCH] test: fix sprintf with snprintf
2019-02-08 13:27 [dpdk-dev] [PATCH] test: fix sprintf with snprintf Pallantla Poornima
2019-02-08 14:04 ` Bruce Richardson
@ 2019-02-08 16:21 ` Stephen Hemminger
2019-02-08 17:04 ` Aaron Conole
2 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2019-02-08 16:21 UTC (permalink / raw)
To: Pallantla Poornima; +Cc: dev, reshma.pattan, ferruh.yigit, stable
On Fri, 8 Feb 2019 13:27:37 +0000
Pallantla Poornima <pallantlax.poornima@intel.com> wrote:
> diff --git a/test/test/commands.c b/test/test/commands.c
> index 94fbc310e..5aeb35498 100644
> --- a/test/test/commands.c
> +++ b/test/test/commands.c
> @@ -367,6 +367,8 @@ int commands_init(void)
> struct test_command *t;
> char *commands, *ptr;
> int commands_len = 0;
> + int total_written = 0;
> + int count = 0;
>
> TAILQ_FOREACH(t, &commands_list, next) {
> commands_len += strlen(t->command) + 1;
> @@ -378,7 +380,10 @@ int commands_init(void)
>
> ptr = commands;
> TAILQ_FOREACH(t, &commands_list, next) {
> - ptr += sprintf(ptr, "%s#", t->command);
> + count = snprintf(ptr, commands_len - total_written - 1, "%s#",
> + t->command);
> + ptr += count;
> + total_written += count;
You know snprintf is dangerous in this case as well.
It returns the number of bytes that would have been written.
That is why the linux kernel introduced scnprintf.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [dpdk-dev] [PATCH] test: fix sprintf with snprintf
2019-02-08 13:27 [dpdk-dev] [PATCH] test: fix sprintf with snprintf Pallantla Poornima
2019-02-08 14:04 ` Bruce Richardson
2019-02-08 16:21 ` Stephen Hemminger
@ 2019-02-08 17:04 ` Aaron Conole
2019-02-08 17:35 ` Stephen Hemminger
2 siblings, 1 reply; 5+ messages in thread
From: Aaron Conole @ 2019-02-08 17:04 UTC (permalink / raw)
To: Pallantla Poornima; +Cc: dev, reshma.pattan, ferruh.yigit, stable
Pallantla Poornima <pallantlax.poornima@intel.com> writes:
> sprintf function is not secure as it doesn't check the length of string.
> More secure function snprintf is used.
>
> Fixes: 727909c592 ("app/test: introduce dynamic commands list")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
> ---
> test/test/commands.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/test/test/commands.c b/test/test/commands.c
> index 94fbc310e..5aeb35498 100644
> --- a/test/test/commands.c
> +++ b/test/test/commands.c
> @@ -367,6 +367,8 @@ int commands_init(void)
> struct test_command *t;
> char *commands, *ptr;
> int commands_len = 0;
> + int total_written = 0;
> + int count = 0;
>
> TAILQ_FOREACH(t, &commands_list, next) {
> commands_len += strlen(t->command) + 1;
> @@ -378,7 +380,10 @@ int commands_init(void)
>
> ptr = commands;
> TAILQ_FOREACH(t, &commands_list, next) {
> - ptr += sprintf(ptr, "%s#", t->command);
> + count = snprintf(ptr, commands_len - total_written - 1, "%s#",
> + t->command);
> + ptr += count;
This code is wrong. From the manpage:
Upon successful completion, the snprintf() function shall return
the number of bytes that would be written to s had n been
sufficiently large excluding the terminating null byte.
This code you've placed will improperly increment the number of bytes
taken, since you don't actually check it.
Additionally, the correct size is calculated in the preceeding blocks,
and then the appropriately sized block is allocated. It doesn't make
any sense to make the change this way.
If you are intent on changing this code, I suggest something like the
following (completely untested code). The rte_xsprintf() function can
be used in other areas where you're proposing these refactors (but again
see my earlier comments about whether these are actual concerns, or just
'I dislike the sprintf call').
---
diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
index 9a2a1ff90..3554c496a 100644
--- a/lib/librte_eal/common/include/rte_string_fns.h
+++ b/lib/librte_eal/common/include/rte_string_fns.h
@@ -98,6 +98,41 @@ rte_strlcpy(char *dst, const char *src, size_t size)
ssize_t
rte_strscpy(char *dst, const char *src, size_t dsize);
+/* Find a better place for this? */
+#define ALWAYS_ASSERT(expr) \
+ if (!(expr)) rte_panic("Assert failed: %s", #expr)
+
+/**
+ * Allocates an appropriately sized string and fills it with
+ * formatted output. Aborts if no memory can be allocated.
+ *
+ * @param fmt
+ * The format string. See the documentation for printf
+ * for valid format strings.
+ *
+ * @return
+ * A string filled with formatted output. Caller must release
+ * this memory with a call to free().
+ */
+static inline char *
+rte_xsprintf(const char *fmt, ...)
+{
+ va_list args,args2;
+ size_t needed;
+ char *result;
+
+ va_start(args, fmt);
+ va_copy(args2, args);
+ needed = vsnprintf(NULL, 0, fmt, args);
+ result = malloc(needed+1);
+ ALWAYS_ASSERT(result != NULL);
+ vsnprintf(result, needed+1, fmt, args2);
+ va_end(args2);
+ va_end(args);
+
+ return result;
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/test/test/commands.c b/test/test/commands.c
index 94fbc310e..eba96b9c9 100644
--- a/test/test/commands.c
+++ b/test/test/commands.c
@@ -365,24 +365,19 @@ cmdline_parse_ctx_t main_ctx[] = {
int commands_init(void)
{
struct test_command *t;
- char *commands, *ptr;
- int commands_len = 0;
+ char *ptr = NULL, *old;
TAILQ_FOREACH(t, &commands_list, next) {
- commands_len += strlen(t->command) + 1;
- }
-
- commands = malloc(commands_len + 1);
- if (!commands)
- return -1;
+ if (!ptr) {
+ ptr = xprintf("%s", t->command);
+ continue;
+ }
- ptr = commands;
- TAILQ_FOREACH(t, &commands_list, next) {
- ptr += sprintf(ptr, "%s#", t->command);
+ old = ptr;
+ ptr = xprintf("%s#%s", old, t->command);
+ free(old);
}
- ptr--;
- ptr[0] = '\0';
- cmd_autotest_autotest.string_data.str = commands;
+ cmd_autotest_autotest.string_data.str = ptr;
return 0;
}
---
^ permalink raw reply [flat|nested] 5+ messages in thread