Richard W.M. Jones
2012-Feb-10 10:37 UTC
[Libguestfs] [PATCH 0/3] Fix guestfish edit command.
This is a further, more comprehensive fix for https://bugzilla.redhat.com/show_bug.cgi?id=788641 The guestfish 'edit' command (aka 'emacs', 'vi') suffered from the same problems as virt-edit and more. It could have failed and left a partially overwritten file, and it didn't preserve permissions etc from the original file. These three patches fix all this. The first is a code cleanup. The second causes 'edit' to write to a temporary file which is atomically moved over the original. The third copies permissions etc from the original to the new file. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
Richard W.M. Jones
2012-Feb-10 10:38 UTC
[Libguestfs] [PATCH 1/3] fish: Refactor error handling in the 'edit' command.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v -------------- next part -------------->From ab0a50971f494a2ace8d70c5cb7a925e7b9e9c1b Mon Sep 17 00:00:00 2001From: "Richard W.M. Jones" <rjones at redhat.com> Date: Fri, 10 Feb 2012 09:31:34 +0000 Subject: [PATCH 1/3] fish: Refactor error handling in the 'edit' command. This is just code motion. --- fish/edit.c | 41 +++++++++++++++++------------------------ 1 files changed, 17 insertions(+), 24 deletions(-) diff --git a/fish/edit.c b/fish/edit.c index b615ef4..e0204b0 100644 --- a/fish/edit.c +++ b/fish/edit.c @@ -43,7 +43,7 @@ run_edit (const char *cmd, size_t argc, char *argv[]) if (argc != 1) { fprintf (stderr, _("use '%s filename' to edit a file\n"), cmd); - return -1; + goto error0; } /* Choose an editor. */ @@ -60,38 +60,31 @@ run_edit (const char *cmd, size_t argc, char *argv[]) /* Handle 'win:...' prefix. */ remotefilename = win_prefix (argv[0]); if (remotefilename == NULL) - return -1; + goto error0; /* Download the file and write it to a temporary. */ fd = mkstemp (filename); if (fd == -1) { perror ("mkstemp"); - free (remotefilename); - return -1; + goto error1; } snprintf (buf, sizeof buf, "/dev/fd/%d", fd); if (guestfs_download (g, remotefilename, buf) == -1) { close (fd); - unlink (filename); - free (remotefilename); - return -1; + goto error2; } if (close (fd) == -1) { perror (filename); - unlink (filename); - free (remotefilename); - return -1; + goto error2; } /* Get the old stat. */ if (stat (filename, &oldstat) == -1) { perror (filename); - unlink (filename); - free (remotefilename); - return -1; + goto error2; } /* Edit it. */ @@ -101,17 +94,13 @@ run_edit (const char *cmd, size_t argc, char *argv[]) r = system (buf); if (r != 0) { perror (buf); - unlink (filename); - free (remotefilename); - return -1; + goto error2; } /* Get the new stat. */ if (stat (filename, &newstat) == -1) { perror (filename); - unlink (filename); - free (remotefilename); - return -1; + goto error2; } /* Changed? */ @@ -123,13 +112,17 @@ run_edit (const char *cmd, size_t argc, char *argv[]) } /* Write new content. */ - if (guestfs_upload (g, filename, remotefilename) == -1) { - unlink (filename); - free (remotefilename); - return -1; - } + if (guestfs_upload (g, filename, remotefilename) == -1) + goto error2; unlink (filename); free (remotefilename); return 0; + + error2: + unlink (filename); + error1: + free (remotefilename); + error0: + return -1; } -- 1.7.9
Richard W.M. Jones
2012-Feb-10 10:39 UTC
[Libguestfs] [PATCH 2/3] fish: In edit command, upload to a new file.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v -------------- next part -------------->From 8a1762d865cca34f7625985ed67d060367544057 Mon Sep 17 00:00:00 2001From: "Richard W.M. Jones" <rjones at redhat.com> Date: Fri, 10 Feb 2012 10:12:45 +0000 Subject: [PATCH 2/3] fish: In edit command, upload to a new file. If the upload fails, this means we don't leave a partially written file. Also add a test for the edit command. --- fish/Makefile.am | 1 + fish/edit.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-- fish/fish.c | 3 ++ fish/test-edit.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 3 deletions(-) create mode 100755 fish/test-edit.sh diff --git a/fish/Makefile.am b/fish/Makefile.am index 8a6f88b..badb04d 100644 --- a/fish/Makefile.am +++ b/fish/Makefile.am @@ -247,6 +247,7 @@ TESTS = \ if ENABLE_APPLIANCE TESTS += \ test-copy.sh \ + test-edit.sh \ test-find0.sh \ test-read_file.sh \ test-remote.sh \ diff --git a/fish/edit.c b/fish/edit.c index e0204b0..908a3a3 100644 --- a/fish/edit.c +++ b/fish/edit.c @@ -26,9 +26,12 @@ #include <inttypes.h> #include <sys/types.h> #include <sys/stat.h> +#include <assert.h> #include "fish.h" +static char *generate_random_name (const char *filename); + /* guestfish edit command, suggested by J?n Ondrej, implemented by RWMJ */ int @@ -37,7 +40,7 @@ run_edit (const char *cmd, size_t argc, char *argv[]) TMP_TEMPLATE_ON_STACK (filename); char buf[256]; const char *editor; - char *remotefilename; + char *remotefilename, *newname; struct stat oldstat, newstat; int r, fd; @@ -111,14 +114,29 @@ run_edit (const char *cmd, size_t argc, char *argv[]) return 0; } - /* Write new content. */ - if (guestfs_upload (g, filename, remotefilename) == -1) + /* Upload to a new file in the same directory, so if it fails we + * don't end up with a partially written file. Give the new file + * a completely random name so we have only a tiny chance of + * overwriting some existing file. + */ + newname = generate_random_name (remotefilename); + if (!newname) goto error2; + /* Write new content. */ + if (guestfs_upload (g, filename, newname) == -1) + goto error3; + + if (guestfs_mv (g, newname, remotefilename) == -1) + goto error3; + + free (newname); unlink (filename); free (remotefilename); return 0; + error3: + free (newname); error2: unlink (filename); error1: @@ -126,3 +144,37 @@ run_edit (const char *cmd, size_t argc, char *argv[]) error0: return -1; } + +static char +random_char (void) +{ + char c[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + return c[random () % (sizeof c - 1)]; +} + +static char * +generate_random_name (const char *filename) +{ + char *ret, *p; + size_t i; + + ret = malloc (strlen (filename) + 16); + if (!ret) { + perror ("malloc"); + return NULL; + } + strcpy (ret, filename); + + p = strrchr (ret, '/'); + assert (p); + p++; + + /* Because of "+ 16" above, there should be enough space in the + * output buffer to write 8 random characters here. + */ + for (i = 0; i < 8; ++i) + *p++ = random_char (); + *p++ = '\0'; + + return ret; /* caller will free */ +} diff --git a/fish/fish.c b/fish/fish.c index e388832..575fe99 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -163,6 +163,9 @@ main (int argc, char *argv[]) bindtextdomain (PACKAGE, LOCALEBASEDIR); textdomain (PACKAGE); + /* We use random(3) in edit.c. */ + srandom (time (NULL)); + parse_config (); enum { HELP_OPTION = CHAR_MAX + 1 }; diff --git a/fish/test-edit.sh b/fish/test-edit.sh new file mode 100755 index 0000000..ff38d1c --- /dev/null +++ b/fish/test-edit.sh @@ -0,0 +1,51 @@ +#!/bin/bash - +# libguestfs +# Copyright (C) 2012 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +# Test guestfish edit command. + +# This test fails on some versions of mock which lack /dev/fd +# directory. Skip this test in that case. + +test -d /dev/fd || { + echo "$0: Skipping this test because /dev/fd is missing." + exit 0 +} + +set -e + +rm -f test1.img + +# The command will be 'echo ... >>/tmp/tmpfile' +export EDITOR="echo second line of text >>" + +output=$( +../fish/guestfish -N fs -m /dev/sda1 <<EOF +write /file.txt "this is a test\n" +edit /file.txt +cat /file.txt +EOF +) + +if [ "$output" != "this is a test +second line of text" ]; then + echo "$0: error: output of guestfish after edit command did not match expected output" + echo "$output" + exit 1 +fi + +rm -f test1.img -- 1.7.9
Richard W.M. Jones
2012-Feb-10 10:39 UTC
[Libguestfs] [PATCH 3/3] fish: edit: Preserve permissions, UID, GID, SELinux context when editing files (RHBZ#788641).
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org -------------- next part -------------->From 0c24a2dd904a2a33f75c6580e671633f1ee80b90 Mon Sep 17 00:00:00 2001From: "Richard W.M. Jones" <rjones at redhat.com> Date: Fri, 10 Feb 2012 10:32:55 +0000 Subject: [PATCH 3/3] fish: edit: Preserve permissions, UID, GID, SELinux context when editing files (RHBZ#788641). --- fish/edit.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++ fish/test-edit.sh | 11 ++++++- 2 files changed, 92 insertions(+), 1 deletions(-) diff --git a/fish/edit.c b/fish/edit.c index 908a3a3..517c098 100644 --- a/fish/edit.c +++ b/fish/edit.c @@ -31,6 +31,8 @@ #include "fish.h" static char *generate_random_name (const char *filename); +static int copy_attributes (const char *src, const char *dest); +static int feature_available (guestfs_h *g, const char *feature); /* guestfish edit command, suggested by J?n Ondrej, implemented by RWMJ */ @@ -127,6 +129,12 @@ run_edit (const char *cmd, size_t argc, char *argv[]) if (guestfs_upload (g, filename, newname) == -1) goto error3; + /* Set the permissions, UID, GID and SELinux context of the new + * file to match the old file (RHBZ#788641). + */ + if (copy_attributes (remotefilename, newname) == -1) + goto error3; + if (guestfs_mv (g, newname, remotefilename) == -1) goto error3; @@ -178,3 +186,77 @@ generate_random_name (const char *filename) return ret; /* caller will free */ } + +static int +copy_attributes (const char *src, const char *dest) +{ + struct guestfs_stat *stat; + int has_linuxxattrs; + char *selinux_context = NULL; + size_t selinux_context_size; + + has_linuxxattrs = feature_available (g, "linuxxattrs"); + + /* Get the mode. */ + stat = guestfs_stat (g, src); + if (stat == NULL) + return -1; + + /* Get the SELinux context. XXX Should we copy over other extended + * attributes too? + */ + if (has_linuxxattrs) { + guestfs_error_handler_cb old_error_cb; + void *old_error_data; + old_error_cb = guestfs_get_error_handler (g, &old_error_data); + guestfs_set_error_handler (g, NULL, NULL); + + selinux_context = guestfs_getxattr (g, src, "security.selinux", + &selinux_context_size); + /* selinux_context could be NULL. This isn't an error. */ + + guestfs_set_error_handler (g, old_error_cb, old_error_data); + } + + /* Set the permissions (inc. sticky and set*id bits), UID, GID. */ + if (guestfs_chmod (g, stat->mode & 07777, dest) == -1) { + guestfs_free_stat (stat); + return -1; + } + if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1) { + guestfs_free_stat (stat); + return -1; + } + guestfs_free_stat (stat); + + /* Set the SELinux context. */ + if (has_linuxxattrs && selinux_context) { + if (guestfs_setxattr (g, "security.selinux", selinux_context, + (int) selinux_context_size, dest) == -1) { + free (selinux_context); + return -1; + } + } + free (selinux_context); + + return 0; +} + +static int +feature_available (guestfs_h *g, const char *feature) +{ + /* If there's an error we should ignore it, so to do that we have to + * temporarily replace the error handler with a null one. + */ + guestfs_error_handler_cb old_error_cb; + void *old_error_data; + old_error_cb = guestfs_get_error_handler (g, &old_error_data); + guestfs_set_error_handler (g, NULL, NULL); + + const char *groups[] = { feature, NULL }; + int r = guestfs_available (g, (char * const *) groups); + + guestfs_set_error_handler (g, old_error_cb, old_error_data); + + return r == 0 ? 1 : 0; +} diff --git a/fish/test-edit.sh b/fish/test-edit.sh index ff38d1c..93b2e90 100755 --- a/fish/test-edit.sh +++ b/fish/test-edit.sh @@ -36,13 +36,22 @@ export EDITOR="echo second line of text >>" output=$( ../fish/guestfish -N fs -m /dev/sda1 <<EOF write /file.txt "this is a test\n" +chmod 0600 /file.txt +chown 10 11 /file.txt edit /file.txt cat /file.txt +stat /file.txt | grep mode: +stat /file.txt | grep uid: +stat /file.txt | grep gid: EOF ) if [ "$output" != "this is a test -second line of text" ]; then +second line of text + +mode: 33152 +uid: 10 +gid: 11" ]; then echo "$0: error: output of guestfish after edit command did not match expected output" echo "$output" exit 1 -- 1.7.9