From: Stephen Hemminger <stephen@networkplumber.org>
To: Jiri Pirko <jiri@resnulli.us>
Cc: netdev@vger.kernel.org, sthemmin@microsoft.com,
dsahern@gmail.com, alexanderk@mellanox.com, mlxsw@mellanox.com
Subject: Re: [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch
Date: Fri, 26 Jul 2019 14:35:52 -0700 [thread overview]
Message-ID: <20190726143552.403d22b0@hermes.lan> (raw)
In-Reply-To: <20190723112538.10977-2-jiri@resnulli.us>
On Tue, 23 Jul 2019 13:25:38 +0200
Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@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 <jiri@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 <jiri@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 <jiri@mellanox.com>
Signed-off-by: Stephen Hemminger <stephen@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
next prev parent reply other threads:[~2019-07-26 21:35 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-23 11:25 [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Jiri Pirko
2019-07-23 11:25 ` [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch Jiri Pirko
2019-07-26 21:35 ` Stephen Hemminger [this message]
2019-07-23 17:47 ` [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Stephen Hemminger
2019-07-23 17:54 ` Stephen Hemminger
2019-07-23 19:36 ` Jiri Pirko
2019-07-26 19:00 ` Stephen Hemminger
2019-07-24 9:07 ` David Laight
2019-07-26 19:47 ` Stephen Hemminger
2019-07-27 8:36 ` Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190726143552.403d22b0@hermes.lan \
--to=stephen@networkplumber.org \
--cc=alexanderk@mellanox.com \
--cc=dsahern@gmail.com \
--cc=jiri@resnulli.us \
--cc=mlxsw@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=sthemmin@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).