On Tue, 23 Jul 2019 13:25:38 +0200
Jiri Pirko <j...@resnulli.us> wrote:
> From: Jiri Pirko <j...@mellanox.com>
>
> When getcmdline fails, there is no valid string in line_next.
> So change the flow and don't process it. Alongside with that,
> free the previous line buffer and prevent memory leak.
>
> Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions")
> Signed-off-by: Jiri Pirko <j...@mellanox.com>
This is not sufficient to avoid valgrind detected uninitialized memory.
The following changes to your patch (#2 alone) is enough to fix
the issue.
The logic here is still a mess and needs to be cleaned up to avoid
future related bugs.
From bbcc22899566556ced9692e4811aea2a38817834 Mon Sep 17 00:00:00 2001
From: Jiri Pirko <j...@mellanox.com>
Date: Tue, 23 Jul 2019 13:25:38 +0200
Subject: [PATCH] tc: batch: fix line/line_next processing in batch
When getcmdline fails, there is no valid string in line_next.
So change the flow and don't process it. Alongside with that,
free the previous line buffer and prevent memory leak.
Also, avoid passing uninitialized memory to filters.
Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions")
Signed-off-by: Jiri Pirko <j...@mellanox.com>
Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
---
tc/tc.c | 84 +++++++++++++++++++++++++++++----------------------------
1 file changed, 43 insertions(+), 41 deletions(-)
diff --git a/tc/tc.c b/tc/tc.c
index 64e342dd85bf..95e5481955ad 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -325,11 +325,10 @@ static int batch(const char *name)
{
struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
char *largv[100], *largv_next[100];
- char *line, *line_next = NULL;
+ char *line = NULL, *line_next = NULL;
bool bs_enabled = false;
bool lastline = false;
int largc, largc_next;
- bool bs_enabled_saved;
bool bs_enabled_next;
int batchsize = 0;
size_t len = 0;
@@ -360,47 +359,40 @@ static int batch(const char *name)
largc = makeargs(line, largv, 100);
bs_enabled = batchsize_enabled(largc, largv);
do {
- if (getcmdline(&line_next, &len, stdin) == -1)
+ if (getcmdline(&line_next, &len, stdin) == -1) {
lastline = true;
-
- largc_next = makeargs(line_next, largv_next, 100);
- bs_enabled_next = batchsize_enabled(largc_next, largv_next);
- if (bs_enabled) {
- struct batch_buf *buf;
-
- buf = get_batch_buf(&buf_pool, &head, &tail);
- if (!buf) {
- fprintf(stderr,
- "failed to allocate batch_buf\n");
- return -1;
- }
- ++batchsize;
- }
-
- /*
- * In batch mode, if we haven't accumulated enough commands
- * and this is not the last command and this command & next
- * command both support the batchsize feature, don't send the
- * message immediately.
- */
- if (!lastline && bs_enabled && bs_enabled_next
- && batchsize != MSG_IOV_MAX)
- send = false;
- else
send = true;
+ } else {
+ largc_next = makeargs(line_next, largv_next, 100);
+ bs_enabled_next = batchsize_enabled(largc_next,
largv_next);
+ if (bs_enabled) {
+ struct batch_buf *buf;
+
+ buf = get_batch_buf(&buf_pool, &head, &tail);
+ if (!buf) {
+ fprintf(stderr,
+ "failed to allocate
batch_buf\n");
+ return -1;
+ }
+ ++batchsize;
+ }
- line = line_next;
- line_next = NULL;
- len = 0;
- bs_enabled_saved = bs_enabled;
- bs_enabled = bs_enabled_next;
-
- if (largc == 0) {
- largc = largc_next;
- memcpy(largv, largv_next, largc * sizeof(char *));
- continue; /* blank line */
+ /*
+ * In batch mode, if we haven't accumulated enough
+ * commands and this is not the last command and this
+ * command & next command both support the batchsize
+ * feature, don't send the message immediately.
+ */
+ if (bs_enabled && bs_enabled_next
+ && batchsize != MSG_IOV_MAX)
+ send = false;
+ else
+ send = true;
}
+ if (largc == 0)
+ goto to_next_line; /* blank line */
+
err = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf,
tail == NULL ? 0 : sizeof(tail->buf));
fflush(stdout);
@@ -411,10 +403,8 @@ static int batch(const char *name)
if (!force)
break;
}
- largc = largc_next;
- memcpy(largv, largv_next, largc * sizeof(char *));
- if (send && bs_enabled_saved) {
+ if (send && bs_enabled) {
struct iovec *iov, *iovs;
struct batch_buf *buf;
struct nlmsghdr *n;
@@ -438,6 +428,18 @@ static int batch(const char *name)
}
batchsize = 0;
}
+
+to_next_line:
+ if (lastline)
+ continue;
+ free(line);
+ line = line_next;
+ line_next = NULL;
+ len = 0;
+ bs_enabled = bs_enabled_next;
+ largc = largc_next;
+ memcpy(largv, largv_next, largc * sizeof(char *));
+
} while (!lastline);
free_batch_bufs(&buf_pool);
--
2.20.1
