| Message ID | 20220824165136.11510-1-tomi.belan@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [pseudo] pseudo_util: Fix resolving relative paths from "/" | expand |
On Wed, 24 Aug 2022 16:51:36 +0000 "Tomi Belan" <tomi.belan@gmail.com> wrote: > pseudo_fix_path() incorrectly assumed that 'base' never ends with a slash > because it's a canonical path. However, base can be "/" - a path which is > canonical and yet starts with a slash. This happens when pseudo_cwd is "/" or ^^^^^^ starts -> ends? > when we're starting from a dirfd pointing to "/". The wrong result from > pseudo_fix_path() caused the database lookup to fail and made pseudo abort. > > Signed-off-by: Tomi Belan <tomi.belan@gmail.com> Luca
On Wed, Aug 24, 2022 at 10:40 PM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > > canonical and yet starts with a slash. This happens when pseudo_cwd is "/" or > ^^^^^^ > starts -> ends? Oops, yes, of course. Good catch. Should I send a v2 patch? :) The corrected commit message line should be: --- canonical and yet ends with a slash. This happens when pseudo_cwd is "/" or --- Tomi
Hi Tomi, On Wed, 24 Aug 2022 23:09:51 +0200 "Tomi Belan" <tomi.belan@gmail.com> wrote: > On Wed, Aug 24, 2022 at 10:40 PM Luca Ceresoli > <luca.ceresoli@bootlin.com> wrote: > > > canonical and yet starts with a slash. This happens when pseudo_cwd is "/" or > > ^^^^^^ > > starts -> ends? > > Oops, yes, of course. Good catch. > > Should I send a v2 patch? :) No need. Richard already took your patch and fixed the message. It's currently in the oe-core-next branch: https://git.yoctoproject.org/pseudo/log/?h=oe-core-next
diff --git a/pseudo_util.c b/pseudo_util.c index e8e9803..64636b7 100644 --- a/pseudo_util.c +++ b/pseudo_util.c @@ -792,9 +792,9 @@ static char *pathbufs[PATHBUFS] = { 0 }; static int pathbuf = 0; /* Canonicalize path. "base", if present, is an already-canonicalized - * path of baselen characters, presumed not to end in a /. path is - * the new path to be canonicalized. The tricky part is that path may - * contain symlinks, which must be resolved. + * path of baselen characters, presumed not to end in a / unless it is + * just "/". path is the new path to be canonicalized. The tricky part + * is that path may contain symlinks, which must be resolved. */ char * pseudo_fix_path(const char *base, const char *path, size_t rootlen, size_t baselen, size_t *lenp, int leave_last) { @@ -808,6 +808,12 @@ pseudo_fix_path(const char *base, const char *path, size_t rootlen, size_t basel pseudo_diag("can't fix empty path.\n"); return 0; } + if (baselen == 1) { + baselen = 0; + } + if (rootlen == 1) { + rootlen = 0; + } newpathlen = pseudo_path_max(); pathlen = strlen(path); /* Crazy shell code (e.g. libtool) can pass in a command pipeline as a path which exceeds the max path diff --git a/test/test-relative-from-root.sh b/test/test-relative-from-root.sh new file mode 100755 index 0000000..e2c230e --- /dev/null +++ b/test/test-relative-from-root.sh @@ -0,0 +1,14 @@ +#!/bin/bash +# pseudo had a bug that made it abort() when looking up a relative path from +# base "/", such as by openat(dirfd_of_root, "foo/bar") or when cwd is /. It +# tried to look up base+"/"+path = "//foo/bar", which is wrong. + +set -e + +touch f1 +relative_pwd=${PWD#/} + +cd / +cat "$relative_pwd/f1" + +rm "$relative_pwd/f1"
pseudo_fix_path() incorrectly assumed that 'base' never ends with a slash because it's a canonical path. However, base can be "/" - a path which is canonical and yet starts with a slash. This happens when pseudo_cwd is "/" or when we're starting from a dirfd pointing to "/". The wrong result from pseudo_fix_path() caused the database lookup to fail and made pseudo abort. Signed-off-by: Tomi Belan <tomi.belan@gmail.com> --- Hi. Here is my third patch for 'pseudo'. I included a test that demonstrates the fix. Try "make test" with old and new pseudo_util.c to see the difference. Run it with PSEUDO_DEBUG to see what happens and how it leads to an abort. JFYI: The real fix is to check "baselen == 1". The "rootlen == 1" condition is just for symmetry. Unlike pseudo_cwd and fd_path(), which can be "/", I think pseudo_chroot will never be "/" because pseudo_client_chroot sets it to NULL instead. So rootlen should never be 1. It's just to be safe. pseudo_util.c | 12 +++++++++--- test/test-relative-from-root.sh | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) create mode 100755 test/test-relative-from-root.sh