[Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Laszlo Ersek]

In Windows guests this may make a difference.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 04c6951..2eec712 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -242,17 +242,27 @@ static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
 
 typedef const char * const ccpc;
 
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
 /* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
 static const struct {
     ccpc *forms;
     int oflag_base;
 } guest_file_open_modes[] = {
-    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
-    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
-    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
-    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+    { (ccpc[]){ "r",          NULL }, O_RDONLY                                 },
+    { (ccpc[]){ "rb",         NULL }, O_RDONLY                      | O_BINARY },
+    { (ccpc[]){ "w",          NULL }, O_WRONLY | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a",          NULL }, O_WRONLY | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND | O_BINARY },
+    { (ccpc[]){ "r+",         NULL }, O_RDWR                                   },
+    { (ccpc[]){ "rb+", "r+b", NULL }, O_RDWR                        | O_BINARY },
+    { (ccpc[]){ "w+",         NULL }, O_RDWR   | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a+",         NULL }, O_RDWR   | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND | O_BINARY }
 };
 
 static int
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> In Windows guests this may make a difference.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> In Windows guests this may make a difference.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 04c6951..2eec712 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c

Oh, and only NOW do I notice that this is in a file named
commands-posix.c that doesn't get compiled into the Windows build of qga
(there, we only build commands-win32.c, and THAT file always fails this
command, because no one has ported guest-file-open there yet).  But I
guess there is still the argument that some weirdnix system exists that
isn't quite POSIX compliant and does have a distinct binary mode (maybe
someone plans on compiling qga for Cygwin instead of native windows,
since at least that would be able to open files when the
commands-win32.c variant doesn't?), so I still think the patch is worth
keeping.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Peter Maydell]

On 7 May 2013 18:27, Eric Blake <eblake@redhat.com> wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
>> In Windows guests this may make a difference.
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> Oh, and only NOW do I notice that this is in a file named
> commands-posix.c that doesn't get compiled into the Windows build of qga
> (there, we only build commands-win32.c, and THAT file always fails this
> command, because no one has ported guest-file-open there yet).  But I
> guess there is still the argument that some weirdnix system exists that
> isn't quite POSIX compliant and does have a distinct binary mode (maybe
> someone plans on compiling qga for Cygwin instead of native windows,
> since at least that would be able to open files when the
> commands-win32.c variant doesn't?), so I still think the patch is worth
> keeping.

If we accept this we should at a minimum update the
commit message to say that we're doing this for
theoretical completeness rather than any practical reason.
(Not that "this may make a difference" is a particularly
convincing commit message in the first place :-))

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[mdroth]

On Tue, May 07, 2013 at 11:27:03AM -0600, Eric Blake wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> > In Windows guests this may make a difference.
> > 
> > Suggested-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  qga/commands-posix.c |   22 ++++++++++++++++------
> >  1 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 04c6951..2eec712 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> 
> Oh, and only NOW do I notice that this is in a file named
> commands-posix.c that doesn't get compiled into the Windows build of qga
> (there, we only build commands-win32.c, and THAT file always fails this
> command, because no one has ported guest-file-open there yet).  But I
> guess there is still the argument that some weirdnix system exists that
> isn't quite POSIX compliant and does have a distinct binary mode (maybe
> someone plans on compiling qga for Cygwin instead of native windows,
> since at least that would be able to open files when the
> commands-win32.c variant doesn't?), so I still think the patch is worth
> keeping.

FWIW, I have some rough patches for w32 implementations of guest-file-*
commands queued up that I'll be cleaning up for 1.6, so it's not so much
theoretical as just a tad early :)

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

[Qemu-devel] [PULL for-1.5] qemu-ga CVE-2013-2007 addenda

[Michael Roth]

Hi Anthony,

These are fix-ups for Laszlo's CVE-2013-2007 fix:

http://www.mail-archive.com/qemu-devel@nongnu.org/msg170944.html

The main effect is to avoid cluttering filesystems with empty files if
we hit an error path in the open/create/chmod path.

I'm unable to confirm whether or not these error paths can actually be
triggered in 1.5 or are just theoretical, but I plan to apply these to
1.4.2 to be sure and so I'm also submitting this for 1.5.

If you think it's too late in the cycle to warrant these for 1.5 I can
also cherry-pick them from my QGA tree for 1.4.2 instead.

The following changes since commit 38ebb396c955ceb2ef7e246248ceb7f8bfe1b774:

  target-i386: ROR r8/r16 imm instruction fix (2013-05-10 19:59:54 +0200)

are available in the git repository at:

  http://github.com/mdroth/qemu qga-pull-2013-05-13

for you to fetch changes up to 2b720018060179b394f8ce736983373ab80dd37c:

  qga: unlink just created guest-file if fchmod() or fdopen() fails on it (2013-05-13 09:45:49 -0500)

----------------------------------------------------------------
Laszlo Ersek (2):
      qga: distinguish binary modes in "guest_file_open_modes" map
      qga: unlink just created guest-file if fchmod() or fdopen() fails on it

 qga/commands-posix.c |   25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

In Windows guests this may make a difference.

Since the original patch (commit c689b4f1) sought to be pedantic and to
consider theoretical corner cases of portability, we should fix it up
where it failed to come through in that pursuit.

Suggested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 04c6951..2eec712 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -242,17 +242,27 @@ static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
 
 typedef const char * const ccpc;
 
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
 /* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
 static const struct {
     ccpc *forms;
     int oflag_base;
 } guest_file_open_modes[] = {
-    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
-    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
-    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
-    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+    { (ccpc[]){ "r",          NULL }, O_RDONLY                                 },
+    { (ccpc[]){ "rb",         NULL }, O_RDONLY                      | O_BINARY },
+    { (ccpc[]){ "w",          NULL }, O_WRONLY | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a",          NULL }, O_WRONLY | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND | O_BINARY },
+    { (ccpc[]){ "r+",         NULL }, O_RDWR                                   },
+    { (ccpc[]){ "rb+", "r+b", NULL }, O_RDWR                        | O_BINARY },
+    { (ccpc[]){ "w+",         NULL }, O_RDWR   | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a+",         NULL }, O_RDWR   | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND | O_BINARY }
 };
 
 static int
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] qga: unlink just created guest-file if fchmod() or fdopen() fails on it

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

We shouldn't allow guest filesystem pollution on error paths.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2eec712..e199738 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -355,6 +355,9 @@ safe_open_or_create(const char *path, const char *mode, Error **err)
             }
 
             close(fd);
+            if (oflag & O_CREAT) {
+                unlink(path);
+            }
         }
     }
 
-- 
1.7.9.5