diff mbox series

[v4,3/3] net: tap: replace snprintf with g_strdup_printf calls

Message ID 20190723104754.29324-4-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series restrict bridge interface name to IFNAMSIZ | expand

Commit Message

Prasad Pandit July 23, 2019, 10:47 a.m. UTC 
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().

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 net/tap.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Update v4: only replace snprintf with g_strdup_printf calls
  -> https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg00578.html

Comments

Stefan Hajnoczi July 23, 2019, 1:03 p.m. UTC | #1 
On Tue, Jul 23, 2019 at 04:17:54PM +0530, P J P wrote:
> -            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 : "");

The change to the br_buf argument isn't covered in the commit
description.  Why did you change this, was it a bug, etc?
Daniel P. Berrangé July 23, 2019, 1:13 p.m. UTC | #2 
On Tue, Jul 23, 2019 at 04:17:54PM +0530, P J P wrote:
> 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().
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  net/tap.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Li Qiang July 23, 2019, 3:43 p.m. UTC | #3 
Stefan Hajnoczi <stefanha@gmail.com> 于2019年7月23日周二 下午9:03写道：

> On Tue, Jul 23, 2019 at 04:17:54PM +0530, P J P wrote:
> > -            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 : "");
>
> The change to the br_buf argument isn't covered in the commit
> description.  Why did you change this, was it a bug, etc?
>


IIUC, if we pass the NULL argument in g_strdup_printf, the 'helper_cmd'
will contain the '(null)' char.
If we pass "" to g_strdup_printf, there is nothing in 'helper_cmd'. The
original is like this.
So here Prasad has to check the 'fd_buf'.

So:

Reviewed-by: Li Qiang <liq3ea@gmail.com>


Thanks,
Li Qiang
Prasad Pandit July 24, 2019, 5:48 a.m. UTC | #4 
+-- On Tue, 23 Jul 2019, Li Qiang wrote --+
| Stefan Hajnoczi <stefanha@gmail.com> 于2019年7月23日周二 下午9:03写道：
| > On Tue, Jul 23, 2019 at 04:17:54PM +0530, P J P wrote:
| > > -            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 : "");
| >
| > The change to the br_buf argument isn't covered in the commit
| > description.  Why did you change this, was it a bug, etc?
| 
| IIUC, if we pass the NULL argument in g_strdup_printf, the 'helper_cmd' will 
| contain the '(null)' char.

Yep, right.

| Reviewed-by: Li Qiang <liq3ea@gmail.com>

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Stefan Hajnoczi July 29, 2019, 3:04 p.m. UTC | #5 
On Wed, Jul 24, 2019 at 11:18:09AM +0530, P J P wrote:
> +-- On Tue, 23 Jul 2019, Li Qiang wrote --+
> | Stefan Hajnoczi <stefanha@gmail.com> 于2019年7月23日周二 下午9:03写道：
> | > On Tue, Jul 23, 2019 at 04:17:54PM +0530, P J P wrote:
> | > > -            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 : "");
> | >
> | > The change to the br_buf argument isn't covered in the commit
> | > description.  Why did you change this, was it a bug, etc?
> | 
> | IIUC, if we pass the NULL argument in g_strdup_printf, the 'helper_cmd' will 
> | contain the '(null)' char.
> 
> Yep, right.

This change isn't related to the topic of the patch.  It's a separate
bug fix.

Please either document it in the commit description so it's clear the
change is intentional, or send it as a separate patch.

Stefan
Jason Wang July 31, 2019, 4:58 a.m. UTC | #6 
On 2019/7/29 下午11:04, Stefan Hajnoczi wrote:
> On Wed, Jul 24, 2019 at 11:18:09AM +0530, P J P wrote:
>> +-- On Tue, 23 Jul 2019, Li Qiang wrote --+
>> | Stefan Hajnoczi <stefanha@gmail.com> 于2019年7月23日周二 下午9:03写道：
>> | > On Tue, Jul 23, 2019 at 04:17:54PM +0530, P J P wrote:
>> | > > -            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 : "");
>> | >
>> | > The change to the br_buf argument isn't covered in the commit
>> | > description.  Why did you change this, was it a bug, etc?
>> |
>> | IIUC, if we pass the NULL argument in g_strdup_printf, the 'helper_cmd' will
>> | contain the '(null)' char.
>>
>> Yep, right.
> This change isn't related to the topic of the patch.  It's a separate
> bug fix.
>
> Please either document it in the commit description so it's clear the
> change is intentional, or send it as a separate patch.
>
> Stefan


Prasad, please send a patch for this.

Thanks
Prasad Pandit July 31, 2019, 6:42 a.m. UTC | #7 
+-- On Wed, 31 Jul 2019, Jason Wang wrote --+
| On 2019/7/29 下午11:04, Stefan Hajnoczi wrote:
| > This change isn't related to the topic of the patch.  It's a separate bug 
| > fix.
| >
| > Please either document it in the commit description so it's clear the 
| > change is intentional, or send it as a separate patch.
| 
| Prasad, please send a patch for this.

Okay, do I redo the series, or just this one patch is okay? (to confirm)

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Jason Wang July 31, 2019, 6:59 a.m. UTC | #8 
On 2019/7/31 下午2:42, P J P wrote:
> +-- On Wed, 31 Jul 2019, Jason Wang wrote --+
> | On 2019/7/29 下午11:04, Stefan Hajnoczi wrote:
> | > This change isn't related to the topic of the patch.  It's a separate bug
> | > fix.
> | >
> | > Please either document it in the commit description so it's clear the
> | > change is intentional, or send it as a separate patch.
> |
> | Prasad, please send a patch for this.
>
> Okay, do I redo the series, or just this one patch is okay? (to confirm)


The series has been merged. Just need a patch on top and I can queue it 
for next release.

Thanks


>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Prasad Pandit July 31, 2019, 9:23 a.m. UTC | #9 
+-- On Wed, 31 Jul 2019, Jason Wang wrote --+
| The series has been merged. Just need a patch on top and I can queue it for 
| next release.

Sent patch v5. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff mbox series

Patch

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 {