Re: [Qemu-devel] [PATCH for 2.10] slirp/smb: Replace constant strings by glib string

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) wrote:
> On 04/07/2017 09:32 AM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > 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 <address@hidden>
> > ---
> >  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 <address@hidden>

Thanks!

Dave

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



--
Dr. David Alan Gilbert / address@hidden / Manchester, UK