Libguestfs - May 2014 - Re: [PATCH 1/4] generator: add always-available optgroups

On Tuesday 20 May 2014 15:56:16 Richard W.M. Jones
wrote:
> On Tue, May 20, 2014 at 03:33:31PM +0200, Pino Toscano wrote:
> > Resolve the given path within the chroot, so scrub can be invoked
> > outside the chroot on an already-resolved path.
> > Given that realpath is used, its availability is checked manually,
> > since scrub-file already depends on the "scrub" feature.
Slightly
> > ugly, but on the other hand realpath is generally available
> > nowadays, so the check should not be failing.
> > 
> > Add few tests in scrub-file for this and other similar issues.
> 
> The realpath test was added (commit a86eb0e0d2c67e2) at a time when we
> thought it would be a good idea to have the daemon run on Windows.
> Since no one thinks that's a good idea any longer, I think you could
> prepend this commit with one which removes tests for realpath /
> HAVE_REALPATH and hard-codes optgroup_realpath_available() == 1.
I didn't know the story behind this, and went for the conservative 
route.
Interesting enough, realpath has been unconditionally used already, so I 
will pull the plug to the conditional usage in some other parts, then.
> Is it possible to make this change more generic so in future it could
> be applied to other functions?
> 
> - Would it make sense to have a 'sysroot_realpath' library function
to
> replace 'sysroot_path' in certain functions?
Good idea. Do you have also an idea which daemon functions, other than 
scrub-files and realpath itself, might need such handling?

-- 
Pino Toscano

Support the possibility to have optional groups always enabled (e.g.
because they were present in the past, and they need to be kept for
users).
Add and use few helper optgroups-related functions to deal also with
them.
---
 generator/daemon.ml    | 24 ++++++++++++++++++++----
 generator/optgroups.ml | 26 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/generator/daemon.ml b/generator/daemon.ml
index 548982b..65d910a 100644
--- a/generator/daemon.ml
+++ b/generator/daemon.ml
@@ -713,11 +713,27 @@ and generate_daemon_optgroups_c ()    pr "#include
\"optgroups.h\"\n";
   pr "\n";
 
+  if optgroups_available <> [] then (
+    pr "static int\n";
+    pr "dummy_available (void)\n";
+    pr "{\n";
+    pr "  return 1;\n";
+    pr "}\n";
+    pr "\n";
+
+    List.iter (
+      fun group ->
+        pr "#define optgroup_%s_available dummy_available\n" group;
+    ) optgroups_available;
+
+    pr "\n";
+  );
+
   pr "struct optgroup optgroups[] = {\n";
   List.iter (
-    fun (group, _) ->
+    fun group ->
       pr "  { \"%s\", optgroup_%s_available },\n" group
group
-  ) optgroups;
+  ) optgroups_names_all;
   pr "  { NULL, NULL }\n";
   pr "};\n"
 
@@ -729,9 +745,9 @@ and generate_daemon_optgroups_h ()    pr "\n";
 
   List.iter (
-    fun (group, _) ->
+    fun group ->
       pr "extern int optgroup_%s_available (void);\n" group
-  ) optgroups;
+  ) optgroups_names_all;
 
   pr "\n";
 
diff --git a/generator/optgroups.ml b/generator/optgroups.ml
index 2a348e7..a08f9c3 100644
--- a/generator/optgroups.ml
+++ b/generator/optgroups.ml
@@ -21,6 +21,18 @@
 open Types
 open Actions
 
+(* The list of optional groups which need to be in the daemon as always
+ * available.
+ *
+ * NOTE: if a optional group listed here has functions using it, then its
+ * presence here will be ignored (thus being handled as usual). This way,
+ * there is no need to remove groups from this list, once they have been
+ * added (as it means at some point they had no more functions using them,
+ * but libguestfs ought to provide them to the user.
+ *)
+let internal_optgroups_available = [
+]
+
 (* Create list of optional groups. *)
 let optgroups    let h = Hashtbl.create 13 in
@@ -42,3 +54,17 @@ let optgroups          group, fns
     ) groups in
   List.sort (fun x y -> compare (fst x) (fst y)) groups
+
+let optgroups_names +  fst (List.split optgroups)
+
+let optgroups_available +  let groups +    List.filter (
+      fun group ->
+        List.mem group optgroups_names <> true
+    ) internal_optgroups_available in
+  List.sort compare groups
+
+let optgroups_names_all +  List.sort compare (optgroups_names @
optgroups_available)
-- 
1.9.0

It has been used unconditionally already for quite some time, so having
just the "realpath" command conditional on the presence of it does not
make much sense.

Drop the configure/build system handling of it, and make the
"realpath"
command no more optional (but keeping the "realpath" feature,
unconditionally available now).
---
 configure.ac           |  1 -
 daemon/realpath.c      | 14 --------------
 generator/actions.ml   |  1 -
 generator/optgroups.ml |  1 +
 4 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/configure.ac b/configure.ac
index 552ed77..f3fd10f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -308,7 +308,6 @@ AC_CHECK_FUNCS([\
     ntohs \
     posix_fallocate \
     posix_fadvise \
-    realpath \
     removexattr \
     setitimer \
     setxattr \
diff --git a/daemon/realpath.c b/daemon/realpath.c
index 99f5247..1e07291 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -33,14 +33,6 @@
 #include "optgroups.h"
 #include "actions.h"
 
-#ifdef HAVE_REALPATH
-
-int
-optgroup_realpath_available (void)
-{
-  return 1;
-}
-
 char *
 do_realpath (const char *path)
 {
@@ -57,12 +49,6 @@ do_realpath (const char *path)
   return ret;			/* caller frees */
 }
 
-#else /* !HAVE_REALPATH */
-
-OPTGROUP_REALPATH_NOT_AVAILABLE
-
-#endif /* !HAVE_REALPATH */
-
 static int find_path_element (int fd_cwd, int is_end, const char *name, char
**name_ret);
 
 char *
diff --git a/generator/actions.ml b/generator/actions.ml
index 0826137..85503ce 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -6665,7 +6665,6 @@ matching lines." };
     name = "realpath";
     style = RString "rpath", [Pathname "path"], [];
     proc_nr = Some 163;
-    optional = Some "realpath";
     tests = [
       InitISOFS, Always, TestResultString (
         [["realpath"; "/../directory"]],
"/directory"), []
diff --git a/generator/optgroups.ml b/generator/optgroups.ml
index a08f9c3..eb5ff74 100644
--- a/generator/optgroups.ml
+++ b/generator/optgroups.ml
@@ -31,6 +31,7 @@ open Actions
  * but libguestfs ought to provide them to the user.
  *)
 let internal_optgroups_available = [
+  "realpath";
 ]
 
 (* Create list of optional groups. *)
-- 
1.9.0

Similar to sysroot_path, but first resolves (using realpath) the given
path within sysroot.
---
 daemon/daemon.h   |  1 +
 daemon/guestfsd.c | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 6535658..94537b7 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -51,6 +51,7 @@ extern const char *sysroot;
 extern size_t sysroot_len;
 
 extern char *sysroot_path (const char *path);
+extern char *sysroot_realpath (const char *path);
 
 extern int is_root_device (const char *device);
 
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 6519ea6..622bda1 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -455,6 +455,27 @@ sysroot_path (const char *path)
   return r;
 }
 
+/* Resolve path within sysroot, calling sysroot_path on the resolved path.
+ *
+ * Caller must check for NULL and call reply_with_perror
("malloc/realpath")
+ * if it is.  Caller must also free the string.
+ *
+ * See also the custom %R printf formatter which does shell quoting too.
+ */
+char *
+sysroot_realpath (const char *path)
+{
+  CLEANUP_FREE char *rp = NULL;
+
+  CHROOT_IN;
+  rp = realpath (path, NULL);
+  CHROOT_OUT;
+  if (rp == NULL)
+    return NULL;
+
+  return sysroot_path (rp);
+}
+
 int
 xwrite (int sock, const void *v_buf, size_t len)
 {
-- 
1.9.0

Resolve the given path within the chroot, so scrub can be invoked
outside the chroot on an already-resolved path.

Add few tests in scrub-file for this and other similar issues.
---
 daemon/scrub.c       |  7 +++++--
 generator/actions.ml | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/daemon/scrub.c b/daemon/scrub.c
index cd880b9..c10b0a0 100644
--- a/daemon/scrub.c
+++ b/daemon/scrub.c
@@ -58,8 +58,11 @@ do_scrub_file (const char *file)
   CLEANUP_FREE char *err = NULL;
   int r;
 
-  /* Make the path relative to /sysroot. */
-  buf = sysroot_path (file);
+  /* Resolve the path to the file, and make the result relative to /sysroot.
+   * If it fails, then the file most probably does not exist or
"file" is
+   * a symlink pointing outside the chroot.
+   */
+  buf = sysroot_realpath (file);
   if (!buf) {
     reply_with_perror ("malloc");
     return -1;
diff --git a/generator/actions.ml b/generator/actions.ml
index 85503ce..ed65c6e 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -5719,7 +5719,18 @@ manual page for more details." };
     tests = [
       InitScratchFS, Always, TestRun (
         [["write"; "/scrub_file"; "content"];
-         ["scrub_file"; "/scrub_file"]]), []
+         ["scrub_file"; "/scrub_file"]]), [];
+      InitScratchFS, Always, TestRun (
+        [["write"; "/scrub_file_2"; "content"];
+         ["ln_s"; "/scrub_file_2";
"/scrub_file_2_link"];
+         ["scrub_file"; "/scrub_file_2_link"]]), [];
+      InitScratchFS, Always, TestLastFail (
+        [["ln_s"; "/scrub_file_3_notexisting";
"/scrub_file_3_link"];
+         ["scrub_file"; "/scrub_file_3_link"]]), [];
+      InitScratchFS, Always, TestLastFail (
+        [["write"; "/scrub_file_4"; "content"];
+         ["ln_s"; "../sysroot/scrub_file_4";
"/scrub_file_4_link"];
+         ["scrub_file"; "/scrub_file_4_link"]]), [];
     ];
     shortdesc = "scrub (securely wipe) a file";
     longdesc = "\
-- 
1.9.0

On Tue, May 20, 2014 at 06:16:37PM +0200, Pino Toscano
wrote:
> Good idea. Do you have also an idea which daemon functions, other than 
> scrub-files and realpath itself, might need such handling?
Not without examining every call.

The only other bug that has been filed in this area (by me as it
happens) is https://bugzilla.redhat.com/show_bug.cgi?id=604041 which
covers a variety of APIs (grep for guestfs_.* in src/fuse.c).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Tue, May 20, 2014 at 07:54:45PM +0200, Pino Toscano
wrote:
> Support the possibility to have optional groups always enabled (e.g.
> because they were present in the past, and they need to be kept for
> users).
> Add and use few helper optgroups-related functions to deal also with
> them.
What do you think about the attached addition to this patch?

(It is meant to be squashed into your patch.)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Tue, May 20, 2014 at 07:54:46PM +0200, Pino Toscano
wrote:
> It has been used unconditionally already for quite some time, so having
> just the "realpath" command conditional on the presence of it
does not
> make much sense.
> 
> Drop the configure/build system handling of it, and make the
"realpath"
> command no more optional (but keeping the "realpath" feature,
> unconditionally available now).
It would be good to mention commit a86eb0e0d2c67e2 in the
commit message.

Apart from that, ACK.

Rich.
>  configure.ac           |  1 -
>  daemon/realpath.c      | 14 --------------
>  generator/actions.ml   |  1 -
>  generator/optgroups.ml |  1 +
>  4 files changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 552ed77..f3fd10f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -308,7 +308,6 @@ AC_CHECK_FUNCS([\
>      ntohs \
>      posix_fallocate \
>      posix_fadvise \
> -    realpath \
>      removexattr \
>      setitimer \
>      setxattr \
> diff --git a/daemon/realpath.c b/daemon/realpath.c
> index 99f5247..1e07291 100644
> --- a/daemon/realpath.c
> +++ b/daemon/realpath.c
> @@ -33,14 +33,6 @@
>  #include "optgroups.h"
>  #include "actions.h"
>  
> -#ifdef HAVE_REALPATH
> -
> -int
> -optgroup_realpath_available (void)
> -{
> -  return 1;
> -}
> -
>  char *
>  do_realpath (const char *path)
>  {
> @@ -57,12 +49,6 @@ do_realpath (const char *path)
>    return ret;			/* caller frees */
>  }
>  
> -#else /* !HAVE_REALPATH */
> -
> -OPTGROUP_REALPATH_NOT_AVAILABLE
> -
> -#endif /* !HAVE_REALPATH */
> -
>  static int find_path_element (int fd_cwd, int is_end, const char *name,
char **name_ret);
>  
>  char *
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 0826137..85503ce 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -6665,7 +6665,6 @@ matching lines." };
>      name = "realpath";
>      style = RString "rpath", [Pathname "path"], [];
>      proc_nr = Some 163;
> -    optional = Some "realpath";
>      tests = [
>        InitISOFS, Always, TestResultString (
>          [["realpath"; "/../directory"]],
"/directory"), []
> diff --git a/generator/optgroups.ml b/generator/optgroups.ml
> index a08f9c3..eb5ff74 100644
> --- a/generator/optgroups.ml
> +++ b/generator/optgroups.ml
> @@ -31,6 +31,7 @@ open Actions
>   * but libguestfs ought to provide them to the user.
>   *)
>  let internal_optgroups_available = [
> +  "realpath";
>  ]
>  
>  (* Create list of optional groups. *)
> -- 
> 1.9.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Tue, May 20, 2014 at 07:54:47PM +0200, Pino Toscano
wrote:
> Similar to sysroot_path, but first resolves (using realpath) the given
> path within sysroot.
> ---
>  daemon/daemon.h   |  1 +
>  daemon/guestfsd.c | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 6535658..94537b7 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -51,6 +51,7 @@ extern const char *sysroot;
>  extern size_t sysroot_len;
>  
>  extern char *sysroot_path (const char *path);
> +extern char *sysroot_realpath (const char *path);
>  
>  extern int is_root_device (const char *device);
>  
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index 6519ea6..622bda1 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -455,6 +455,27 @@ sysroot_path (const char *path)
>    return r;
>  }
>  
> +/* Resolve path within sysroot, calling sysroot_path on the resolved path.
> + *
> + * Caller must check for NULL and call reply_with_perror
("malloc/realpath")
> + * if it is.  Caller must also free the string.
> + *
> + * See also the custom %R printf formatter which does shell quoting too.
> + */
> +char *
> +sysroot_realpath (const char *path)
> +{
> +  CLEANUP_FREE char *rp = NULL;
> +
> +  CHROOT_IN;
> +  rp = realpath (path, NULL);
> +  CHROOT_OUT;
> +  if (rp == NULL)
> +    return NULL;
> +
> +  return sysroot_path (rp);
> +}
> +
>  int
>  xwrite (int sock, const void *v_buf, size_t len)
>  {
> -- 
> 1.9.0
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, May 20, 2014 at 07:54:48PM +0200, Pino Toscano
wrote:
> Resolve the given path within the chroot, so scrub can be invoked
> outside the chroot on an already-resolved path.
> 
> Add few tests in scrub-file for this and other similar issues.
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Support the possibility to have optional groups always enabled (e.g.
because they were present in the past, and they need to be kept for
users).
Add and use few helper optgroups-related functions to deal also with
them.
---
 generator/daemon.ml    | 24 ++++++++++++++++++++----
 generator/optgroups.ml | 22 ++++++++++++++++++++++
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/generator/daemon.ml b/generator/daemon.ml
index 548982b..ca748d2 100644
--- a/generator/daemon.ml
+++ b/generator/daemon.ml
@@ -713,11 +713,27 @@ and generate_daemon_optgroups_c ()    pr "#include
\"optgroups.h\"\n";
   pr "\n";
 
+  if optgroups_retired <> [] then (
+    pr "static int\n";
+    pr "dummy_available (void)\n";
+    pr "{\n";
+    pr "  return 1;\n";
+    pr "}\n";
+    pr "\n";
+
+    List.iter (
+      fun group ->
+        pr "#define optgroup_%s_available dummy_available\n" group;
+    ) optgroups_retired;
+
+    pr "\n";
+  );
+
   pr "struct optgroup optgroups[] = {\n";
   List.iter (
-    fun (group, _) ->
+    fun group ->
       pr "  { \"%s\", optgroup_%s_available },\n" group
group
-  ) optgroups;
+  ) optgroups_names_all;
   pr "  { NULL, NULL }\n";
   pr "};\n"
 
@@ -729,9 +745,9 @@ and generate_daemon_optgroups_h ()    pr "\n";
 
   List.iter (
-    fun (group, _) ->
+    fun group ->
       pr "extern int optgroup_%s_available (void);\n" group
-  ) optgroups;
+  ) optgroups_names;
 
   pr "\n";
 
diff --git a/generator/optgroups.ml b/generator/optgroups.ml
index 2a348e7..1c7f45e 100644
--- a/generator/optgroups.ml
+++ b/generator/optgroups.ml
@@ -19,8 +19,16 @@
 (* Please read generator/README first. *)
 
 open Types
+open Utils
 open Actions
 
+(* The list of optional groups which need to be in the daemon as always
+ * available.  These are "retired" as they no longer appear in the
+ * list of functions.
+ *)
+let optgroups_retired = [
+]
+
 (* Create list of optional groups. *)
 let optgroups    let h = Hashtbl.create 13 in
@@ -42,3 +50,17 @@ let optgroups          group, fns
     ) groups in
   List.sort (fun x y -> compare (fst x) (fst y)) groups
+
+let optgroups_names +  fst (List.split optgroups)
+
+let optgroups_names_all +  List.sort compare (optgroups_names @
optgroups_retired)
+
+let () +  let file = "generator/optgroups.ml" in
+  List.iter (
+    fun x ->
+      if List.mem x optgroups_names then
+        failwithf "%s: optgroups_retired list contains optgroup with
functions (%s)" file x
+  ) optgroups_retired
-- 
1.9.0

commit a86eb0e0d2c67e2153139c681632edc72162b8ed made it an optional
feature, as on Windows it was not available; on the other hand, realpath
has been used unconditionally already for quite some time, so having
just the "realpath" command conditional on the presence of it does not
make much sense.

Drop the configure/build system handling of it, make the "realpath"
command no more optional, and keep the "realpath" feature as
unconditionally available now.
---
 configure.ac           |  1 -
 daemon/realpath.c      | 14 --------------
 generator/actions.ml   |  1 -
 generator/optgroups.ml |  1 +
 4 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/configure.ac b/configure.ac
index 552ed77..f3fd10f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -308,7 +308,6 @@ AC_CHECK_FUNCS([\
     ntohs \
     posix_fallocate \
     posix_fadvise \
-    realpath \
     removexattr \
     setitimer \
     setxattr \
diff --git a/daemon/realpath.c b/daemon/realpath.c
index 99f5247..1e07291 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -33,14 +33,6 @@
 #include "optgroups.h"
 #include "actions.h"
 
-#ifdef HAVE_REALPATH
-
-int
-optgroup_realpath_available (void)
-{
-  return 1;
-}
-
 char *
 do_realpath (const char *path)
 {
@@ -57,12 +49,6 @@ do_realpath (const char *path)
   return ret;			/* caller frees */
 }
 
-#else /* !HAVE_REALPATH */
-
-OPTGROUP_REALPATH_NOT_AVAILABLE
-
-#endif /* !HAVE_REALPATH */
-
 static int find_path_element (int fd_cwd, int is_end, const char *name, char
**name_ret);
 
 char *
diff --git a/generator/actions.ml b/generator/actions.ml
index 0826137..85503ce 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -6665,7 +6665,6 @@ matching lines." };
     name = "realpath";
     style = RString "rpath", [Pathname "path"], [];
     proc_nr = Some 163;
-    optional = Some "realpath";
     tests = [
       InitISOFS, Always, TestResultString (
         [["realpath"; "/../directory"]],
"/directory"), []
diff --git a/generator/optgroups.ml b/generator/optgroups.ml
index 1c7f45e..d13d794 100644
--- a/generator/optgroups.ml
+++ b/generator/optgroups.ml
@@ -27,6 +27,7 @@ open Actions
  * list of functions.
  *)
 let optgroups_retired = [
+  "realpath";
 ]
 
 (* Create list of optional groups. *)
-- 
1.9.0

Similar to sysroot_path, but first resolves (using realpath) the given
path within sysroot.
---
 daemon/daemon.h   |  1 +
 daemon/guestfsd.c | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 6535658..94537b7 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -51,6 +51,7 @@ extern const char *sysroot;
 extern size_t sysroot_len;
 
 extern char *sysroot_path (const char *path);
+extern char *sysroot_realpath (const char *path);
 
 extern int is_root_device (const char *device);
 
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 6519ea6..622bda1 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -455,6 +455,27 @@ sysroot_path (const char *path)
   return r;
 }
 
+/* Resolve path within sysroot, calling sysroot_path on the resolved path.
+ *
+ * Caller must check for NULL and call reply_with_perror
("malloc/realpath")
+ * if it is.  Caller must also free the string.
+ *
+ * See also the custom %R printf formatter which does shell quoting too.
+ */
+char *
+sysroot_realpath (const char *path)
+{
+  CLEANUP_FREE char *rp = NULL;
+
+  CHROOT_IN;
+  rp = realpath (path, NULL);
+  CHROOT_OUT;
+  if (rp == NULL)
+    return NULL;
+
+  return sysroot_path (rp);
+}
+
 int
 xwrite (int sock, const void *v_buf, size_t len)
 {
-- 
1.9.0

Resolve the given path within the chroot, so scrub can be invoked
outside the chroot on an already-resolved path.

Add few tests in scrub-file for this and other similar issues.
---
 daemon/scrub.c       |  7 +++++--
 generator/actions.ml | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/daemon/scrub.c b/daemon/scrub.c
index cd880b9..c10b0a0 100644
--- a/daemon/scrub.c
+++ b/daemon/scrub.c
@@ -58,8 +58,11 @@ do_scrub_file (const char *file)
   CLEANUP_FREE char *err = NULL;
   int r;
 
-  /* Make the path relative to /sysroot. */
-  buf = sysroot_path (file);
+  /* Resolve the path to the file, and make the result relative to /sysroot.
+   * If it fails, then the file most probably does not exist or
"file" is
+   * a symlink pointing outside the chroot.
+   */
+  buf = sysroot_realpath (file);
   if (!buf) {
     reply_with_perror ("malloc");
     return -1;
diff --git a/generator/actions.ml b/generator/actions.ml
index 85503ce..ed65c6e 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -5719,7 +5719,18 @@ manual page for more details." };
     tests = [
       InitScratchFS, Always, TestRun (
         [["write"; "/scrub_file"; "content"];
-         ["scrub_file"; "/scrub_file"]]), []
+         ["scrub_file"; "/scrub_file"]]), [];
+      InitScratchFS, Always, TestRun (
+        [["write"; "/scrub_file_2"; "content"];
+         ["ln_s"; "/scrub_file_2";
"/scrub_file_2_link"];
+         ["scrub_file"; "/scrub_file_2_link"]]), [];
+      InitScratchFS, Always, TestLastFail (
+        [["ln_s"; "/scrub_file_3_notexisting";
"/scrub_file_3_link"];
+         ["scrub_file"; "/scrub_file_3_link"]]), [];
+      InitScratchFS, Always, TestLastFail (
+        [["write"; "/scrub_file_4"; "content"];
+         ["ln_s"; "../sysroot/scrub_file_4";
"/scrub_file_4_link"];
+         ["scrub_file"; "/scrub_file_4_link"]]), [];
     ];
     shortdesc = "scrub (securely wipe) a file";
     longdesc = "\
-- 
1.9.0

On Wed, May 21, 2014 at 03:15:39PM +0200, Pino Toscano
wrote:
> Resolve the given path within the chroot, so scrub can be invoked
> outside the chroot on an already-resolved path.
> 
> Add few tests in scrub-file for this and other similar issues.
> ---
ACK whole series.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html