From: Prasad J Pandit <pjp@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().
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 <pjp@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;
+ char *br_buf = NULL;
+ char *helper_cmd = NULL;
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]);
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 {
--
2.21.0
Patchew URL: https://patchew.org/QEMU/20190731091933.17363-1-ppandit@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls Message-id: 20190731091933.17363-1-ppandit@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190731091933.17363-1-ppandit@redhat.com -> patchew/20190731091933.17363-1-ppandit@redhat.com Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers' Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF' Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2' Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe' Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios' Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware' Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi' Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode' Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios' Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa' Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios' Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot' Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot' Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex' Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp' Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3' Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3' Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into 'capstone'... Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf' Cloning into 'dtc'... Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536' Cloning into 'roms/QemuMacDrivers'... Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266' Cloning into 'roms/SLOF'... Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3' Cloning into 'roms/edk2'... Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54' Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3' Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl' Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'... Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'CryptoPkg/Library/OpensslLib/openssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687' Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl' Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5' Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography' Cloning into 'boringssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f' Cloning into 'krb5'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168' Cloning into 'pyca-cryptography'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f' Cloning into 'roms/ipxe'... Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17' Cloning into 'roms/openbios'... Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174' Cloning into 'roms/openhackware'... Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5' Cloning into 'roms/opensbi'... Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6' Cloning into 'roms/qemu-palcode'... Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f' Cloning into 'roms/seabios'... Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4' Cloning into 'roms/seabios-hppa'... Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b' Cloning into 'roms/sgabios'... Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a' Cloning into 'roms/skiboot'... Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501' Cloning into 'roms/u-boot'... Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff' Cloning into 'roms/u-boot-sam460ex'... Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588' Cloning into 'slirp'... Submodule path 'slirp': checked out 'f0da6726207b740f6101028b2992f918477a4b08' Cloning into 'tests/fp/berkeley-softfloat-3'... Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'tests/fp/berkeley-testfloat-3'... Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3' Cloning into 'ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' Switched to a new branch 'test' === OUTPUT BEGIN === checkpatch.pl: no revisions returned for revlist '1' === OUTPUT END === Test command exited with code: 255 The full log is available at http://patchew.org/logs/20190731091933.17363-1-ppandit@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
P J P <ppandit@redhat.com> writes:
> From: Prasad J Pandit <pjp@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 <pjp@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 <armbru@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 {
+-- On Wed, 31 Jul 2019, Markus Armbruster wrote --+ | However, the code is still rather ugly, and I'd be tempted to use the | opportunity to clean up some more. Untested sketch: Patch v3 did a similar change -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00578.html Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
© 2016 - 2026 Red Hat, Inc.