P J P <ppan...@redhat.com> writes:
> From: Prasad J Pandit <p...@fedoraproject.org>
>
> When invoking qemu-bridge-helper in 'net_bridge_run_helper',
> instead of using fixed sized buffers, use dynamically allocated
> ones initialised and returned by g_strdup_printf().
Does this fix a bug?
> If bridge name 'br_buf' is undefined, pass empty string ("") to
> g_strdup_printf() in its place, to avoid printing "(null)" string.
>
> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
> ---
> net/tap.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> Update v5: add commit message about conditional 'br_buf' argument
> -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06397.html
>
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..fc38029f41 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -498,9 +498,9 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
> }
> if (pid == 0) {
> int open_max = sysconf(_SC_OPEN_MAX), i;
> - char fd_buf[6+10];
> - char br_buf[6+IFNAMSIZ] = {0};
> - char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
> + char *fd_buf = NULL;
Dead initializer.
> + char *br_buf = NULL;
> + char *helper_cmd = NULL;
Another one.
>
> for (i = 3; i < open_max; i++) {
> if (i != sv[1]) {
> @@ -508,17 +508,17 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
> }
> }
>
> - snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
> + fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
Good opportunity to change this to
fd_buf = g_strdup_printf("--fd=%d", sv[1]);
More of the same below.
>
> if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
> /* assume helper is a command */
>
> if (strstr(helper, "--br=") == NULL) {
> - snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> + br_buf = g_strdup_printf("%s%s", "--br=", bridge);
> }
>
> - snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> - helper, "--use-vnet", fd_buf, br_buf);
> + helper_cmd = g_strdup_printf("%s %s %s %s", helper,
> + "--use-vnet", fd_buf, br_buf ? br_buf : "");
>
> parg = args;
> *parg++ = (char *)"sh";
> @@ -527,10 +527,11 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
> *parg++ = NULL;
>
> execv("/bin/sh", args);
> + g_free(helper_cmd);
> } else {
> /* assume helper is just the executable path name */
>
> - snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> + br_buf = g_strdup_printf("%s%s", "--br=", bridge);
>
> parg = args;
> *parg++ = (char *)helper;
> @@ -541,6 +542,8 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
>
> execv(helper, args);
> }
> + g_free(fd_buf);
> + g_free(br_buf);
> _exit(1);
>
> } else {
The commit does what it claims to do, and no more, so
Reviewed-by: Markus Armbruster <arm...@redhat.com>
However, the code is still rather ugly, and I'd be tempted to use the
opportunity to clean up some more. Untested sketch:
diff --git a/net/tap.c b/net/tap.c
index 06af8fb8ad..57bb4c552d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -402,8 +402,7 @@ static void launch_script(const char *setup_script, const
char *ifname,
int fd, Error **errp)
{
int pid, status;
- char *args[3];
- char **parg;
+ const char *argv[3];
/* try to launch network script */
pid = fork();
@@ -413,18 +412,18 @@ static void launch_script(const char *setup_script, const
char *ifname,
return;
}
if (pid == 0) {
- int open_max = sysconf(_SC_OPEN_MAX), i;
+ int open_max = sysconf(_SC_OPEN_MAX);
+ int i;
for (i = 3; i < open_max; i++) {
if (i != fd) {
close(i);
}
}
- parg = args;
- *parg++ = (char *)setup_script;
- *parg++ = (char *)ifname;
- *parg = NULL;
- execv(setup_script, args);
+ argv[0] = setup_script;
+ argv[1] = ifname;
+ argv[2] = NULL;
+ execv(setup_script, (char *const *)argv);
_exit(1);
} else {
while (waitpid(pid, &status, 0) != pid) {
@@ -478,8 +477,7 @@ static int net_bridge_run_helper(const char *helper, const
char *bridge,
{
sigset_t oldmask, mask;
int pid, status;
- char *args[5];
- char **parg;
+ const char *argv[5];
int sv[2];
sigemptyset(&mask);
@@ -498,10 +496,9 @@ static int net_bridge_run_helper(const char *helper, const
char *bridge,
return -1;
}
if (pid == 0) {
- int open_max = sysconf(_SC_OPEN_MAX), i;
- char fd_buf[6+10];
- char br_buf[6+IFNAMSIZ] = {0};
- char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
+ int open_max = sysconf(_SC_OPEN_MAX);
+ int i;
+ char *fd_opt, *br_opt;
for (i = 3; i < open_max; i++) {
if (i != sv[1]) {
@@ -509,39 +506,30 @@ static int net_bridge_run_helper(const char *helper,
const char *bridge,
}
}
- snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
+ fd_opt = g_strdup_printf("--fd=%d", sv[1]);
+ br_opt = g_strdup_printf("--br=%s", bridge);
if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
/* assume helper is a command */
-
- if (strstr(helper, "--br=") == NULL) {
- snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
- }
-
- snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
- helper, "--use-vnet", fd_buf, br_buf);
-
- parg = args;
- *parg++ = (char *)"sh";
- *parg++ = (char *)"-c";
- *parg++ = helper_cmd;
- *parg++ = NULL;
-
- execv("/bin/sh", args);
+ /* FIXME strstr() is lazy and somewhat fragile */
+ bool need_br = !strstr(helper, "--br=");
+
+ argv[0] = "/bin/sh";
+ argv[1] = "-c";
+ argv[2] = g_strdup_printf("%s --use-vnet %s %s",
+ helper, fd_opt,
+ need_br ? br_opt : "");
+ argv[3] = NULL;
} else {
/* assume helper is just the executable path name */
-
- snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
-
- parg = args;
- *parg++ = (char *)helper;
- *parg++ = (char *)"--use-vnet";
- *parg++ = fd_buf;
- *parg++ = br_buf;
- *parg++ = NULL;
-
- execv(helper, args);
+ argv[0] = helper;
+ argv[1] = "--use-vnet";
+ argv[2] = fd_opt;
+ argv[3] = br_opt;
+ argv[4] = NULL;
}
+
+ execv(argv[0], (char *const *)argv);
_exit(1);
} else {
