Patchwork [v2] linux-user: fix emulation of getdents

login
register
mail settings
Submitter Dmitry V. Levin
Date Aug. 18, 2012, 9:56 p.m.
Message ID <20120818215629.GA27951@altlinux.org>
Download mbox | patch
Permalink /patch/178494/
State New
Headers show

Comments

Dmitry V. Levin - Aug. 18, 2012, 9:56 p.m.
In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
byte of the target dirent structure (aka d_type byte) was never copied
from the host dirent structure, thus breaking everything that relies
on valid d_type value, e.g. glob(3).

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 linux-user/syscall.c      | 8 ++++----
 linux-user/syscall_defs.h | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)
Blue Swirl - Aug. 19, 2012, 12:59 p.m.
On Sat, Aug 18, 2012 at 9:56 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
> byte of the target dirent structure (aka d_type byte) was never copied
> from the host dirent structure, thus breaking everything that relies
> on valid d_type value, e.g. glob(3).
>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

Acked-by: Blue Swirl <blauwirbel@gmail.com>

> ---
>  linux-user/syscall.c      | 8 ++++----
>  linux-user/syscall_defs.h | 3 ++-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 41c869b..adc3270 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7030,10 +7030,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                      tde->d_ino = tswapal(de->d_ino);
>                      tde->d_off = tswapal(de->d_off);
>                     tnamelen = treclen - (2 * sizeof(abi_long) + 2);
> -                   if (tnamelen > 256)
> -                        tnamelen = 256;
> -                    /* XXX: may not be correct */
> -                    pstrcpy(tde->d_name, tnamelen, de->d_name);
> +                    if (tnamelen > sizeof(tde->d_name)) {
> +                        tnamelen = sizeof(tde->d_name);
> +                    }
> +                    memcpy(tde->d_name, de->d_name, tnamelen);
>                      de = (struct linux_dirent *)((char *)de + reclen);
>                      len -= reclen;
>                      tde = (struct target_dirent *)((char *)tde + treclen);
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 2cfda5a..19fe414 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -258,7 +258,8 @@ struct target_dirent {
>         abi_long        d_ino;
>         abi_long        d_off;
>         unsigned short  d_reclen;
> -       char            d_name[256]; /* We must not include limits.h! */
> +       char            d_name[257];    /* 257 = NAME_MAX + '\0' + d_type, */
> +                                        /* we must not include limits.h! */
>  };
>
>  struct target_dirent64 {
>
> --
> ldv
Peter Maydell - Aug. 20, 2012, 4:17 p.m.
On 18 August 2012 22:56, Dmitry V. Levin <ldv@altlinux.org> wrote:
> In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
> byte of the target dirent structure (aka d_type byte) was never copied
> from the host dirent structure, thus breaking everything that relies
> on valid d_type value, e.g. glob(3).
>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>  linux-user/syscall.c      | 8 ++++----
>  linux-user/syscall_defs.h | 3 ++-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 41c869b..adc3270 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7030,10 +7030,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                      tde->d_ino = tswapal(de->d_ino);
>                      tde->d_off = tswapal(de->d_off);
>                     tnamelen = treclen - (2 * sizeof(abi_long) + 2);

Incidentally I think this would be clearer as
  tnamelen = treclen - offsetof(target_dirent, d_name);

> -                   if (tnamelen > 256)
> -                        tnamelen = 256;
> -                    /* XXX: may not be correct */
> -                    pstrcpy(tde->d_name, tnamelen, de->d_name);
> +                    if (tnamelen > sizeof(tde->d_name)) {
> +                        tnamelen = sizeof(tde->d_name);
> +                    }
> +                    memcpy(tde->d_name, de->d_name, tnamelen);

Why are we limiting the amount copied to sizeof(tde->d_name) in the
first place? The only actual limit we care about is that we mustn't
overrun the guest's overall buffer size (and I assert that we can't
do that because sizeof(target_dirent) <= sizeof(linux_dirent). We
could even assert in the code that we don't write more than 'count'
bytes to the guest buffer...

As far as I can tell the kernel doesn't impose any particular limit
on the filesize; we should just copy all of the record as defined
by d_reclen.

>                      de = (struct linux_dirent *)((char *)de + reclen);
>                      len -= reclen;
>                      tde = (struct target_dirent *)((char *)tde + treclen);
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 2cfda5a..19fe414 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -258,7 +258,8 @@ struct target_dirent {
>         abi_long        d_ino;
>         abi_long        d_off;
>         unsigned short  d_reclen;
> -       char            d_name[256]; /* We must not include limits.h! */
> +       char            d_name[257];    /* 257 = NAME_MAX + '\0' + d_type, */
> +                                        /* we must not include limits.h! */
>  };

...and if we did that we could make d_name[] a flexible
array member, ie "char d_name[];".

-- PMM

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41c869b..adc3270 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7030,10 +7030,10 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                     tde->d_ino = tswapal(de->d_ino);
                     tde->d_off = tswapal(de->d_off);
 		    tnamelen = treclen - (2 * sizeof(abi_long) + 2);
-		    if (tnamelen > 256)
-                        tnamelen = 256;
-                    /* XXX: may not be correct */
-                    pstrcpy(tde->d_name, tnamelen, de->d_name);
+                    if (tnamelen > sizeof(tde->d_name)) {
+                        tnamelen = sizeof(tde->d_name);
+                    }
+                    memcpy(tde->d_name, de->d_name, tnamelen);
                     de = (struct linux_dirent *)((char *)de + reclen);
                     len -= reclen;
                     tde = (struct target_dirent *)((char *)tde + treclen);
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 2cfda5a..19fe414 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -258,7 +258,8 @@  struct target_dirent {
 	abi_long	d_ino;
 	abi_long	d_off;
 	unsigned short	d_reclen;
-	char		d_name[256]; /* We must not include limits.h! */
+	char		d_name[257];    /* 257 = NAME_MAX + '\0' + d_type, */
+                                        /* we must not include limits.h! */
 };
 
 struct target_dirent64 {