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;
#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;
}
--
2.12.2
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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
* 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
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
* 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
© 2016 - 2024 Red Hat, Inc.