diff mbox

[for,2.10] slirp/smb: Replace constant strings by glib string

Message ID 20170407143254.22061-1-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert April 7, 2017, 2:32 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

gcc 7 (on fedora 26) objects to many of the snprintf's
in the smb path and command creation because it can't
figure out that the smb_dir (i.e. the /tmp dir for the configuration)
is known to be short.

Replace all these fixed length buffers by g_str* functions that dynamically
allocate and use g_dir_make_tmp to make the directory.
(It's fairly new glib but we have a compat function for it).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 net/slirp.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

Comments

Eric Blake April 7, 2017, 2:54 p.m. UTC | #1 
On 04/07/2017 09:32 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> gcc 7 (on fedora 26) objects to many of the snprintf's
> in the smb path and command creation because it can't
> figure out that the smb_dir (i.e. the /tmp dir for the configuration)
> is known to be short.
> 
> Replace all these fixed length buffers by g_str* functions that dynamically
> allocate and use g_dir_make_tmp to make the directory.
> (It's fairly new glib but we have a compat function for it).
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  net/slirp.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/net/slirp.c b/net/slirp.c
> index f97ec23345..9f6521190b 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -80,7 +80,7 @@ typedef struct SlirpState {
>      Slirp *slirp;
>      Notifier exit_notifier;
>  #ifndef _WIN32
> -    char smb_dir[128];
> +    gchar *smb_dir;

Does it really have to be gchar? That's one of the more pointless
typedefs in glib; and I think 'char *' is just fine.


>  
> -    snprintf(smb_cmdline, sizeof(smb_cmdline), "%s -l %s -s %s",
> +    smb_cmdline = g_strdup_printf("%s -l %s -s %s",
>               CONFIG_SMBD_COMMAND, s->smb_dir, smb_conf);

Gross that we're parsing a command line through a shell, but
pre-existing and looks like CONFIG_SMBD_COMMAND, s->smb_dir, and
smb_conf should all be enough under our control to ensure that there are
no shell metacharacters and therefore exploits possible.

The cleanup is useful, and resolves one of the build issues I pointed
out earlier on Rawhide (looks like it is now Fedora 26 in addition to
Rawhide that have new-enough gcc).  In that thread, we argued that it's
not going to be essential to get this in for 2.9, but as more and more
people move to newer gcc, it will probably be a candidate for
qemu-stable for 2.9.1 in addition to 2.10.

Reviewed-by: Eric Blake <eblake@redhat.com>
Dr. David Alan Gilbert April 7, 2017, 2:56 p.m. UTC | #2 
* Eric Blake (eblake@redhat.com) wrote:
> On 04/07/2017 09:32 AM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > gcc 7 (on fedora 26) objects to many of the snprintf's
> > in the smb path and command creation because it can't
> > figure out that the smb_dir (i.e. the /tmp dir for the configuration)
> > is known to be short.
> > 
> > Replace all these fixed length buffers by g_str* functions that dynamically
> > allocate and use g_dir_make_tmp to make the directory.
> > (It's fairly new glib but we have a compat function for it).
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  net/slirp.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/net/slirp.c b/net/slirp.c
> > index f97ec23345..9f6521190b 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -80,7 +80,7 @@ typedef struct SlirpState {
> >      Slirp *slirp;
> >      Notifier exit_notifier;
> >  #ifndef _WIN32
> > -    char smb_dir[128];
> > +    gchar *smb_dir;
> 
> Does it really have to be gchar? That's one of the more pointless
> typedefs in glib; and I think 'char *' is just fine.

Yes I'm happy either way.

> > -    snprintf(smb_cmdline, sizeof(smb_cmdline), "%s -l %s -s %s",
> > +    smb_cmdline = g_strdup_printf("%s -l %s -s %s",
> >               CONFIG_SMBD_COMMAND, s->smb_dir, smb_conf);
> 
> Gross that we're parsing a command line through a shell, but
> pre-existing and looks like CONFIG_SMBD_COMMAND, s->smb_dir, and
> smb_conf should all be enough under our control to ensure that there are
> no shell metacharacters and therefore exploits possible.

Yes, it's quite hacky.

> The cleanup is useful, and resolves one of the build issues I pointed
> out earlier on Rawhide (looks like it is now Fedora 26 in addition to
> Rawhide that have new-enough gcc).  In that thread, we argued that it's
> not going to be essential to get this in for 2.9, but as more and more
> people move to newer gcc, it will probably be a candidate for
> qemu-stable for 2.9.1 in addition to 2.10.

Ah which thread is that?
I just posted:
https://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg01294.html

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Dave

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake April 7, 2017, 3:03 p.m. UTC | #3 
On 04/07/2017 09:56 AM, Dr. David Alan Gilbert wrote:

> 
>> The cleanup is useful, and resolves one of the build issues I pointed
>> out earlier on Rawhide (looks like it is now Fedora 26 in addition to
>> Rawhide that have new-enough gcc).  In that thread, we argued that it's
>> not going to be essential to get this in for 2.9, but as more and more
>> people move to newer gcc, it will probably be a candidate for
>> qemu-stable for 2.9.1 in addition to 2.10.
> 
> Ah which thread is that?

https://lists.gnu.org/archive/html/qemu-devel/2017-03/threads.html#04552
in particular
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04567.html

> I just posted:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg01294.html

Yes, I just saw that. And it is indeed hacky, but maybe it will spur
some proper patches.
Dr. David Alan Gilbert April 7, 2017, 3:14 p.m. UTC | #4 
* Eric Blake (eblake@redhat.com) wrote:
> On 04/07/2017 09:56 AM, Dr. David Alan Gilbert wrote:
> 
> > 
> >> The cleanup is useful, and resolves one of the build issues I pointed
> >> out earlier on Rawhide (looks like it is now Fedora 26 in addition to
> >> Rawhide that have new-enough gcc).  In that thread, we argued that it's
> >> not going to be essential to get this in for 2.9, but as more and more
> >> people move to newer gcc, it will probably be a candidate for
> >> qemu-stable for 2.9.1 in addition to 2.10.
> > 
> > Ah which thread is that?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/threads.html#04552
> in particular
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04567.html

Yep, feels about right.

> > I just posted:
> > https://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg01294.html
> 
> Yes, I just saw that. And it is indeed hacky, but maybe it will spur
> some proper patches.

Yes, and some of those it might be best to see what the gcc people say
as to whether they'll fix them.

Dave

> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Samuel Thibault April 8, 2017, 9:55 p.m. UTC | #5 
Hello,

Applied to my tree, thanks!

Samuel
diff mbox

Patch

diff --git a/net/slirp.c b/net/slirp.c
index f97ec23345..9f6521190b 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -80,7 +80,7 @@  typedef struct SlirpState {
     Slirp *slirp;
     Notifier exit_notifier;
 #ifndef _WIN32
-    char smb_dir[128];
+    gchar *smb_dir;
 #endif
 } SlirpState;
 
@@ -558,11 +558,10 @@  int net_slirp_redir(const char *redir_str)
 /* automatic user mode samba server configuration */
 static void slirp_smb_cleanup(SlirpState *s)
 {
-    char cmd[128];
     int ret;
 
-    if (s->smb_dir[0] != '\0') {
-        snprintf(cmd, sizeof(cmd), "rm -rf %s", s->smb_dir);
+    if (s->smb_dir) {
+        gchar *cmd = g_strdup_printf("rm -rf %s", s->smb_dir);
         ret = system(cmd);
         if (ret == -1 || !WIFEXITED(ret)) {
             error_report("'%s' failed.", cmd);
@@ -570,15 +569,17 @@  static void slirp_smb_cleanup(SlirpState *s)
             error_report("'%s' failed. Error code: %d",
                          cmd, WEXITSTATUS(ret));
         }
-        s->smb_dir[0] = '\0';
+        g_free(cmd);
+        g_free(s->smb_dir);
+        s->smb_dir = NULL;
     }
 }
 
 static int slirp_smb(SlirpState* s, const char *exported_dir,
                      struct in_addr vserver_addr)
 {
-    char smb_conf[128];
-    char smb_cmdline[128];
+    char *smb_conf;
+    char *smb_cmdline;
     struct passwd *passwd;
     FILE *f;
 
@@ -600,19 +601,19 @@  static int slirp_smb(SlirpState* s, const char *exported_dir,
         return -1;
     }
 
-    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.XXXXXX");
-    if (!mkdtemp(s->smb_dir)) {
-        error_report("could not create samba server dir '%s'", s->smb_dir);
-        s->smb_dir[0] = 0;
+    s->smb_dir = g_dir_make_tmp("qemu-smb.XXXXXX", NULL);
+    if (!s->smb_dir) {
+        error_report("could not create samba server dir");
         return -1;
     }
-    snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf");
+    smb_conf = g_strdup_printf("%s/%s", s->smb_dir, "smb.conf");
 
     f = fopen(smb_conf, "w");
     if (!f) {
         slirp_smb_cleanup(s);
         error_report("could not create samba server configuration file '%s'",
                      smb_conf);
+        g_free(smb_conf);
         return -1;
     }
     fprintf(f,
@@ -651,15 +652,18 @@  static int slirp_smb(SlirpState* s, const char *exported_dir,
             );
     fclose(f);
 
-    snprintf(smb_cmdline, sizeof(smb_cmdline), "%s -l %s -s %s",
+    smb_cmdline = g_strdup_printf("%s -l %s -s %s",
              CONFIG_SMBD_COMMAND, s->smb_dir, smb_conf);
+    g_free(smb_conf);
 
     if (slirp_add_exec(s->slirp, 0, smb_cmdline, &vserver_addr, 139) < 0 ||
         slirp_add_exec(s->slirp, 0, smb_cmdline, &vserver_addr, 445) < 0) {
         slirp_smb_cleanup(s);
+        g_free(smb_cmdline);
         error_report("conflicting/invalid smbserver address");
         return -1;
     }
+    g_free(smb_cmdline);
     return 0;
 }