Message ID | 1432811743-2423-1-git-send-email-mjt@msgid.tls.msk.ru |
---|---|
State | New |
Headers | show |
Michael Tokarev <mjt@tls.msk.ru> writes: > In this version I used mkdtemp(3) which is: > > _BSD_SOURCE > || /* Since glibc 2.10: */ > (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700) In short, it's POSIX.1-2008. > so should be available on systems we care about. Yes. > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Michael Tokarev <mjt@tls.msk.ru> writes: > In this version I used mkdtemp(3) which is: > > _BSD_SOURCE > || /* Since glibc 2.10: */ > (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700) > > so should be available on systems we care about. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > net/slirp.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/slirp.c b/net/slirp.c > index 9bbed74..0ad32ad 100644 > --- a/net/slirp.c > +++ b/net/slirp.c > @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s) > static int slirp_smb(SlirpState* s, const char *exported_dir, > struct in_addr vserver_addr) > { > - static int instance; > char smb_conf[128]; > char smb_cmdline[128]; > struct passwd *passwd; > @@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir, > return -1; > } > > - snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d", > - (long)getpid(), instance++); > - if (mkdir(s->smb_dir, 0700) < 0) { > + strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX"); > + if (!mkdtemp(s->smb_dir)) { > error_report("could not create samba server dir '%s'", s->smb_dir); > return -1; > } Uh, I have to suspend my R-by. This clobbers s->smb_dir[] when mkdtemp() fails. Is that safe? Explain why, and get my R-by back. The alternative "[PATCH] net: fix insecure temporary file creation in SLiRP" doesn't clobber it. http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg00005.html
01.06.2015 11:01, Markus Armbruster wrote: [] >> - snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d", >> - (long)getpid(), instance++); >> - if (mkdir(s->smb_dir, 0700) < 0) { >> + strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX"); >> + if (!mkdtemp(s->smb_dir)) { >> error_report("could not create samba server dir '%s'", s->smb_dir); >> return -1; >> } > > Uh, I have to suspend my R-by. This clobbers s->smb_dir[] when > mkdtemp() fails. Is that safe? Explain why, and get my R-by back. At least this is not different from the original behavour. The only place where this variable is used is in the slirp_smb_cleanup() function, which, if smb_dir[0] != 0, does rm -rf on it. I think it will be better to add s->smb_dir[0] = 0 here before return. On the other hand this isn't really that important given when we can expect mkdtemp() to fail. Thanks, /mjt
Michael Tokarev <mjt@tls.msk.ru> writes: > 01.06.2015 11:01, Markus Armbruster wrote: > [] >>> - snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d", >>> - (long)getpid(), instance++); >>> - if (mkdir(s->smb_dir, 0700) < 0) { >>> + strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX"); >>> + if (!mkdtemp(s->smb_dir)) { >>> error_report("could not create samba server dir '%s'", s->smb_dir); >>> return -1; >>> } >> >> Uh, I have to suspend my R-by. This clobbers s->smb_dir[] when >> mkdtemp() fails. Is that safe? Explain why, and get my R-by back. > > At least this is not different from the original behavour. Fair enough, R-by unsuspended. > The only place where this variable is used is in the slirp_smb_cleanup() > function, which, if smb_dir[0] != 0, does rm -rf on it. > I think it will be better to add s->smb_dir[0] = 0 here before return. Makes sense. > On the other hand this isn't really that important given when we can > expect mkdtemp() to fail. Not sure I got this part. Not sure I need to :)
On Thu, May 28, 2015 at 02:15:43PM +0300, Michael Tokarev wrote: > In this version I used mkdtemp(3) which is: > > _BSD_SOURCE > || /* Since glibc 2.10: */ > (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700) > > so should be available on systems we care about. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > net/slirp.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/slirp.c b/net/slirp.c > index 9bbed74..0ad32ad 100644 > --- a/net/slirp.c > +++ b/net/slirp.c > @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s) > static int slirp_smb(SlirpState* s, const char *exported_dir, > struct in_addr vserver_addr) > { > - static int instance; > char smb_conf[128]; > char smb_cmdline[128]; > struct passwd *passwd; > @@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir, > return -1; > } > > - snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d", > - (long)getpid(), instance++); > - if (mkdir(s->smb_dir, 0700) < 0) { > + strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX"); > + if (!mkdtemp(s->smb_dir)) { > error_report("could not create samba server dir '%s'", s->smb_dir); > return -1; > } > -- > 2.1.4 > > I suggest to go with this patch as: 1) It was sent first 2) Is simplier 3) Keep original behavior Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>
diff --git a/net/slirp.c b/net/slirp.c index 9bbed74..0ad32ad 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s) static int slirp_smb(SlirpState* s, const char *exported_dir, struct in_addr vserver_addr) { - static int instance; char smb_conf[128]; char smb_cmdline[128]; struct passwd *passwd; @@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir, return -1; } - snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d", - (long)getpid(), instance++); - if (mkdir(s->smb_dir, 0700) < 0) { + strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX"); + if (!mkdtemp(s->smb_dir)) { error_report("could not create samba server dir '%s'", s->smb_dir); return -1; }
In this version I used mkdtemp(3) which is: _BSD_SOURCE || /* Since glibc 2.10: */ (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700) so should be available on systems we care about. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- net/slirp.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)