From: Keno Fischer <keno@alumni.harvard.edu>

This code relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR having the same
numerical value, but on Darwin, they do not.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 6e0b2e8..c55ea25 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1376,7 +1376,17 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
         return -1;
     }
 
-    ret = local_unlinkat_common(ctx, dirfd, name, flags);
+    if ((flags & ~P9_DOTL_AT_REMOVEDIR) != 0) {
+        errno = EINVAL;
+        return -1;
+    }
+
+    size_t rflags = 0;
+    if (flags & P9_DOTL_AT_REMOVEDIR) {
+        rflags |= AT_REMOVEDIR;
+    }
+
+    ret = local_unlinkat_common(ctx, dirfd, name, rflags);
     close_preserve_errno(dirfd);
     return ret;
 }
-- 
2.8.1

On Sat, 26 May 2018 01:23:09 -0400
keno@juliacomputing.com wrote:

> From: Keno Fischer <keno@alumni.harvard.edu>
> 
> This code relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR having the same
> numerical value, but on Darwin, they do not.
> 
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  hw/9pfs/9p-local.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 6e0b2e8..c55ea25 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1376,7 +1376,17 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
>          return -1;
>      }
>  
> -    ret = local_unlinkat_common(ctx, dirfd, name, flags);
> +    if ((flags & ~P9_DOTL_AT_REMOVEDIR) != 0) {

The != 0 isn't needed but...

> +        errno = EINVAL;
> +        return -1;

... I'm more concerned about this new error path. How can this happen ?

> +    }
> +
> +    size_t rflags = 0;

Please declare this at the beginning of the function.

> +    if (flags & P9_DOTL_AT_REMOVEDIR) {
> +        rflags |= AT_REMOVEDIR;
> +    }
> +
> +    ret = local_unlinkat_common(ctx, dirfd, name, rflags);
>      close_preserve_errno(dirfd);
>      return ret;
>  }

>> +        errno = EINVAL;
>> +        return -1;
>
> ... I'm more concerned about this new error path. How can this happen ?
>

As far as I can tell, the flags come from the client without any
intermediate error
checking. Since the Darwin constants do not match the Linux constants (which
have the same numerical values as the 9p constants), we need to perform this
checking/translation somewhere to ensure correct behavior.
Is there a more appropriate place to put this check?

On Thu, 31 May 2018 12:25:49 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> >> +        errno = EINVAL;
> >> +        return -1;  
> >
> > ... I'm more concerned about this new error path. How can this happen ?
> >  
> 
> As far as I can tell, the flags come from the client without any
> intermediate error
> checking. 

Indeed :-\

> Since the Darwin constants do not match the Linux constants (which
> have the same numerical values as the 9p constants), we need to perform this
> checking/translation somewhere to ensure correct behavior.
> Is there a more appropriate place to put this check?

The right thing to do would be to check and translate the flag from
P9_DOTL_AT_REMOVEDIR to AT_REMOVEDIR in the core 9p server code.