[Qemu-devel] [PATCH] linux-user: use realpath for emulation dir paths

[Qemu-devel] [PATCH] linux-user: use realpath for emulation dir paths

[Paul Bolle]

The paths to files inside the emulation dir as returned by path() are
not neat canonicalized absolute pathnames but can (and will) contain
"/./", "/../" and "//" parts. As far as I know these ugly paths will
only be seen when one is (printf) debugging. I assume, however, that
these paths have to be canonicalized every time they are used to open
files so that might as well be done when they are created.

Some minor cleanups etc. added too.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
I only noticed this because I "#if 1"'d a printf() in
linux-user/elfload.c (see a trivial patch I just send to the list).

Note that I have some reservations about the current init_paths() and
path() code:
- their names seem to confusing. Maybe those should be init_base() and
base() or something similar;
- why does init_paths() copy all filenames in the emulation dir (at
least, that what it seems to do)? Try something silly like
"-L /home/../" to see what I mean ...
- and why does path() return the original filename if that file isn't
found in the emulation dir? That looks like a nice source for confusing
behavior or crashes, as that means an identical named file (but using
the regular root) will then be used.

Maybe I'll elaborate in a separate mail.

 path.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/path.c b/path.c
index cc9e007..875cb03 100644
--- a/path.c
+++ b/path.c
@@ -122,25 +122,30 @@ follow_path(const struct pathelem *cursor, const char *name)
 void init_paths(const char *prefix)
 {
     char pref_buf[PATH_MAX];
+    char real_buf[PATH_MAX];
 
-    if (prefix[0] == '\0' ||
-        !strcmp(prefix, "/"))
+    if (prefix[0] == '\0')
         return;
 
     if (prefix[0] != '/') {
         char *cwd = getcwd(NULL, 0);
-        size_t pref_buf_len = sizeof(pref_buf);
 
         if (!cwd)
             abort();
-        pstrcpy(pref_buf, sizeof(pref_buf), cwd);
-        pstrcat(pref_buf, pref_buf_len, "/");
-        pstrcat(pref_buf, pref_buf_len, prefix);
+        pstrcpy(pref_buf, PATH_MAX, cwd);
+        pstrcat(pref_buf, PATH_MAX, "/");
+        pstrcat(pref_buf, PATH_MAX, prefix);
         free(cwd);
     } else
-        pstrcpy(pref_buf, sizeof(pref_buf), prefix + 1);
+        pstrcpy(pref_buf, PATH_MAX, prefix);
 
-    base = new_entry("", NULL, pref_buf);
+    if (realpath(pref_buf, real_buf))
+         pstrcpy(pref_buf, PATH_MAX, real_buf);
+
+    if (!strcmp(pref_buf, "/"))
+        return;
+
+    base = new_entry("", NULL, pref_buf + 1);
     base = add_dir_maybe(base);
     if (base->num_entries == 0) {
         free (base);
-- 
1.6.5.rc2

Re: [Qemu-devel] [PATCH] linux-user: use realpath for emulation dir paths

[Riku Voipio]

On Fri, Oct 02, 2009 at 02:25:36PM +0200, Paul Bolle wrote:
> Note that I have some reservations about the current init_paths() and
> path() code:
> - their names seem to confusing. Maybe those should be init_base() and
> base() or something similar;
> - why does init_paths() copy all filenames in the emulation dir (at
> least, that what it seems to do)? Try something silly like
> "-L /home/../" to see what I mean ...
> - and why does path() return the original filename if that file isn't
> found in the emulation dir? That looks like a nice source for confusing
> behavior or crashes, as that means an identical named file (but using
> the regular root) will then be used.

Yeah, all that is a big mess and should be cleaned up. At the moment it
is all too easy to get init_paths to recurse forever..

Re: [Qemu-devel] [PATCH] linux-user: use realpath for emulation dir paths

[Arnaud Patard]

Riku Voipio <riku.voipio@iki.fi> writes:

Hi,

> On Fri, Oct 02, 2009 at 02:25:36PM +0200, Paul Bolle wrote:
>> Note that I have some reservations about the current init_paths() and
>> path() code:
>> - their names seem to confusing. Maybe those should be init_base() and
>> base() or something similar;
>> - why does init_paths() copy all filenames in the emulation dir (at
>> least, that what it seems to do)? Try something silly like
>> "-L /home/../" to see what I mean ...
>> - and why does path() return the original filename if that file isn't
>> found in the emulation dir? That looks like a nice source for confusing
>> behavior or crashes, as that means an identical named file (but using
>> the regular root) will then be used.
>
> Yeah, all that is a big mess and should be cleaned up. At the moment it
> is all too easy to get init_paths to recurse forever..

fwiw, it should not be hard to prevent this dead loop. I have somewhere
a patch avoiding going into /dev,/proc and it cured the problem. Of
course, there may be some other places leading to deadloop but at least
avoiding /dev and /proc would be a good start if one really wants to fix
that.

Arnaud

Re: [Qemu-devel] [PATCH] linux-user: use realpath for emulation dir paths

[Paul Bolle]

On Fri, 2009-12-04 at 11:00 +0100, Arnaud Patard wrote:
> Riku Voipio <riku.voipio@iki.fi> writes:
> > On Fri, Oct 02, 2009 at 02:25:36PM +0200, Paul Bolle wrote:
> >> Note that I have some reservations about the current init_paths() and
> >> path() code:
> >> - their names seem to confusing. Maybe those should be init_base()and
> >> base() or something similar;
> >> - why does init_paths() copy all filenames in the emulation dir (at
> >> least, that what it seems to do)? Try something silly like
> >> "-L /home/../" to see what I mean ...
> >> - and why does path() return the original filename if that file isn't
> >> found in the emulation dir? That looks like a nice source for confusing
> >> behavior or crashes, as that means an identical named file (but using 
> >> the regular root) will then be used.
> >
> > Yeah, all that is a big mess and should be cleaned up. At the moment it
> > is all too easy to get init_paths to recurse forever..
> 
> fwiw, it should not be hard to prevent this dead loop. I have
> somewhere a patch avoiding going into /dev,/proc and it cured the
> problem. Of course, there may be some other places leading to deadloop
> but at least avoiding /dev and /proc would be a good start if one
> really wants to fix that.

It's been two months, so I have forgotten most details here, but why is
the init_path() step actually needed? Can't path() simply prepend the
name of emulation dir, if any, and return that (possibly unaltered)
path?

I guess the original path must be altered too (ie, it should point to
the newly created path and the original string should be freed). Can't
that be done reliably?


Paul