| Message ID | 1254486336.1738.30.camel@localhost.localdomain |
|---|---|
| State | Superseded |
| Headers | show |
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..
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
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
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);
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(-)