[v4,3/3] 9pfs: handle walk of ".." in the root directory

Commit Message

Greg Kurz Aug. 30, 2016, 6:40 p.m. UTC

The 9P spec at http://man.cat-v.org/plan_9/5/intro says:

All directories must support walks to the directory .. (dot-dot) meaning
parent directory, although by convention directories contain no explicit
entry for .. or . (dot).  The parent of the root directory of a server's
tree is itself.

This means that a client cannot walk further than the root directory
exported by the server. In other words, if the client wants to walk
"/.." or "/foo/../..", the server should answer like the request was
to walk "/".

This patch just does that:
- we cache the QID of the root directory at attach time
- during the walk we compare the QID of each path component with the root
  QID to detect if we're in a "/.." situation
- if so, we skip the current component and go to the next one

Signed-off-by: Greg Kurz <[e-mail]>
Reviewed-by: Eric Blake <[e-mail]>

V3: - fixed typo in changelog
    - added Eric's R-b tag
---
 hw/9pfs/9p.c |   40 +++++++++++++++++++++++++++++++---------
 hw/9pfs/9p.h |    1 +
 2 files changed, 32 insertions(+), 9 deletions(-)

Comments

Paolo Bonzini Sept. 15, 2016, 10:22 p.m. UTC | #1

On 30/08/2016 20:40, Greg Kurz wrote:
> +
> +    err = fid_to_qid(pdu, fidp, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
> +
>      v9fs_path_init(&dpath);
>      v9fs_path_init(&path);

The "out" label can now be reached without having initialized dpath and
path.  This upsets Coverity (and might also cause a segfault, indeed).

The simplest fix is to move the v9fs_path_init before the fid_to_qid call.

Paolo

>      /*
> @@ -1318,16 +1334,22 @@ static void v9fs_walk(void *opaque)
>      v9fs_path_copy(&dpath, &fidp->path);
>      v9fs_path_copy(&path, &fidp->path);
>      for (name_idx = 0; name_idx < nwnames; name_idx++) {
> -        err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data, &path);
> -        if (err < 0) {
> -            goto out;
> -        }
> -        err = v9fs_co_lstat(pdu, &path, &stbuf);
> -        if (err < 0) {
> -            goto out;
> +        if (not_same_qid(&pdu->s->root_qid, &qid) ||
> +            strcmp("..", wnames[name_idx].data)) {
> +            err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data,
> +                                       &path);
> +            if (err < 0) {
> +                goto out;
> +            }
> +
> +            err = v9fs_co_lstat(pdu, &path, &stbuf);
> +            if (err < 0) {
> +                goto out;
> +            }
> +            stat_to_qid(&stbuf, &qid);
> +            v9fs_path_copy(&dpath, &path);
>          }
> -        stat_to_qid(&stbuf, &qids[name_idx]);
> -        v9fs_path_copy(&dpath, &path);
> +        memcpy(&qids[name_idx], &qid, sizeof(qid));
>      }
>      if (fid == newfid) {
>          BUG_ON(fidp->fid_type != P9_FID_NONE);
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index b4f757ab5449..a38603398ef5 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -236,6 +236,7 @@ typedef struct V9fsState
>      int32_t root_fid;
>      Error *migration_blocker;
>      V9fsConf fsconf;
> +    V9fsQID root_qid;
>  } V9fsState;
>  
>  /* 9p2000.L open flags */
> 
> 
>

Patch

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 51c6f9883bf8..dfe293d11d1c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1010,6 +1010,7 @@  static void v9fs_attach(void *opaque)
         goto out;
     }
     err += offset;
+    memcpy(&s->root_qid, &qid, sizeof(qid));
     trace_v9fs_attach_return(pdu->tag, pdu->id,
                              qid.type, qid.version, qid.path);
     /*
@@ -1261,6 +1262,14 @@  static bool name_is_illegal(const char *name)
     return !*name || strchr(name, '/') != NULL;
 }
 
+static bool not_same_qid(const V9fsQID *qid1, const V9fsQID *qid2)
+{
+    return
+        qid1->type != qid2->type ||
+        qid1->version != qid2->version ||
+        qid1->path != qid2->path;
+}
+
 static void v9fs_walk(void *opaque)
 {
     int name_idx;
@@ -1276,6 +1285,7 @@  static void v9fs_walk(void *opaque)
     V9fsFidState *newfidp = NULL;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
+    V9fsQID qid;
 
     err = pdu_unmarshal(pdu, offset, "ddw", &fid, &newfid, &nwnames);
     if (err < 0) {
@@ -1309,6 +1319,12 @@  static void v9fs_walk(void *opaque)
         err = -ENOENT;
         goto out_nofid;
     }
+
+    err = fid_to_qid(pdu, fidp, &qid);
+    if (err < 0) {
+        goto out;
+    }
+
     v9fs_path_init(&dpath);
     v9fs_path_init(&path);
     /*
@@ -1318,16 +1334,22 @@  static void v9fs_walk(void *opaque)
     v9fs_path_copy(&dpath, &fidp->path);
     v9fs_path_copy(&path, &fidp->path);
     for (name_idx = 0; name_idx < nwnames; name_idx++) {
-        err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data, &path);
-        if (err < 0) {
-            goto out;
-        }
-        err = v9fs_co_lstat(pdu, &path, &stbuf);
-        if (err < 0) {
-            goto out;
+        if (not_same_qid(&pdu->s->root_qid, &qid) ||
+            strcmp("..", wnames[name_idx].data)) {
+            err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data,
+                                       &path);
+            if (err < 0) {
+                goto out;
+            }
+
+            err = v9fs_co_lstat(pdu, &path, &stbuf);
+            if (err < 0) {
+                goto out;
+            }
+            stat_to_qid(&stbuf, &qid);
+            v9fs_path_copy(&dpath, &path);
         }
-        stat_to_qid(&stbuf, &qids[name_idx]);
-        v9fs_path_copy(&dpath, &path);
+        memcpy(&qids[name_idx], &qid, sizeof(qid));
     }
     if (fid == newfid) {
         BUG_ON(fidp->fid_type != P9_FID_NONE);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index b4f757ab5449..a38603398ef5 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -236,6 +236,7 @@  typedef struct V9fsState
     int32_t root_fid;
     Error *migration_blocker;
     V9fsConf fsconf;
+    V9fsQID root_qid;
 } V9fsState;
 
 /* 9p2000.L open flags */