linux-user: fix emulation of getdents
PATCH 1/1 linux-user: fix emulation of getdents
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 native dirent structure, thus breaking everything that relies on valid d_type value, e.g. glob(3).
--- linux-user/syscall.c | 7 +++---- linux-user/syscall_defs.h | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-)
linux-user/syscall.c +3 -4
@@ -7030,10 +7030,9 @@ 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);
linux-user/syscall_defs.h +2 -1
@@ -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 ];/* We must not include limits.h! */ +/* 257 = NAME_MAX + '\0' + d_type */ };struct target_dirent64 {
Signature · 1 line
ldv
CC'ed to linux-user maintainer, Riku.
Quoted 46 lines · Dmitry V. Levin
In case when
TARGET_ABI_BITS== 32 &&HOST_LONG_BITS== 64, the last byte of the target dirent structure (akad_typebyte) was never copied from the native dirent structure, thus breaking everything that relies on validd_typevalue, e.g. glob(3).Signed-off-by Dmitry V. Levin <[email protected]>--- linux-user/syscall.c | 7 +++---- linux-user/syscall_defs.h | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-)linux-user/syscall.c +3
-4@@ -7030,10 +7030,9 @@ 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);linux-user/syscall_defs.h +2
-1@@ -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 ];/* We must not include limits.h! */ +/* 257 = NAME_MAX + '\0' + d_type */ };struct target_dirent64 {Signature · 2 lines
-- ldv
Signature · 5 lines
Wei-Ren Chen (陳韋任) Computer Systems Lab, Institute of Information Science, Academia Sinica, Taiwan (R.O.C.) Tel:886-2-2788-3799 #1667 Homepage: http://people.cs.nctu.edu.tw/~chenwj
Quoted 16 lines · Dmitry V. Levin
In case when
TARGET_ABI_BITS== 32 &&HOST_LONG_BITS== 64, the last byte of the target dirent structure (akad_typebyte) was never copied from the native dirent structure, thus breaking everything that relies on validd_typevalue, e.g. glob(3).Signed-off-by Dmitry V. Levin <[email protected]>--- linux-user/syscall.c | 7 +++---- linux-user/syscall_defs.h | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-)linux-user/syscall.c +0
-0
linux-user/syscall.c abi_long do_syscall(void *cpu_env, int num, abi_long arg1, · line 7034
Quoted 9 lines
linux-user/syscall.c +2
-4@@ -7030,10 +7030,9 @@ 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);
Missing braces, please read CODING_STYLE.
Otherwise patch looks good.
Quoted 22 lines
linux-user/syscall.c +1
-0+memcpy (tde->d_name, de->d_name, tnamelen); de = (struct linux_dirent *)((char *)de + reclen); len -= reclen; tde = (struct target_dirent *)((char *)tde + treclen);linux-user/syscall_defs.h +2
-2@@ -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 ];/* We must not include limits.h! */ +/* 257 = NAME_MAX + '\0' + d_type */ };struct target_dirent64 { --ldv
PATCH v2 1/1 linux-user: fix emulation of getdents
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).
--- linux-user/syscall.c | 8 ++++---- linux-user/syscall_defs.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-)
linux-user/syscall.c +4 -4
@@ -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);
linux-user/syscall_defs.h +2 -1
@@ -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 {
Signature · 1 line
ldv
Quoted 6 lines · Dmitry V. Levin
In case when
TARGET_ABI_BITS== 32 &&HOST_LONG_BITS== 64, the last byte of the target dirent structure (akad_typebyte) was never copied from the host dirent structure, thus breaking everything that relies on validd_typevalue, e.g. glob(3).Signed-off-by Dmitry V. Levin <[email protected]>
Quoted 41 lines
--- linux-user/syscall.c | 8 ++++---- linux-user/syscall_defs.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-)linux-user/syscall.c +4
-4@@ -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);linux-user/syscall_defs.h +2
-2@@ -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
Quoted 16 lines · Dmitry V. Levin
In case when
TARGET_ABI_BITS== 32 &&HOST_LONG_BITS== 64, the last byte of the target dirent structure (akad_typebyte) was never copied from the host dirent structure, thus breaking everything that relies on validd_typevalue, e.g. glob(3).Signed-off-by Dmitry V. Levin <[email protected]>--- linux-user/syscall.c | 8 ++++---- linux-user/syscall_defs.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-)linux-user/syscall.c +0
-0
linux-user/syscall.c abi_long do_syscall(void *cpu_env, int num, abi_long arg1, · line 7032
Quoted 3 lines
linux-user/syscall.c +0
-0@@ -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);
linux-user/syscall.c abi_long do_syscall(void *cpu_env, int num, abi_long arg1, · line 7036
Quoted 8 lines
linux-user/syscall.c +4
-4-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.
Quoted 8 lines
linux-user/syscall.c +0
-0de = (struct linux_dirent *)((char *)de + reclen); len -= reclen; tde = (struct target_dirent *)((char *)tde + treclen);linux-user/syscall_defs.h +0
-0
linux-user/syscall_defs.h struct target_dirent { · line 263
Quoted 7 lines
linux-user/syscall_defs.h +2
-1@@ -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 v3 1/1 linux-user: fix emulation of getdents
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).
--- linux-user/syscall.c | 11 +++++------ linux-user/syscall_defs.h | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-)
linux-user/syscall.c +5 -6
@@ -7025,15 +7025,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, tde = target_dirp;while (len >0 ) { reclen = de->d_reclen; - treclen = reclen - (2 * (sizeof (long ) -sizeof (abi_long))); + tnamelen = reclen - offsetof(struct linux_dirent , d_name); +assert (tnamelen >=0 ); + treclen = tnamelen + offsetof(struct target_dirent , d_name); +assert (count1 + treclen <= count); tde->d_reclen =tswap16 (treclen); 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); +memcpy (tde->d_name, de->d_name, tnamelen); de = (struct linux_dirent *)((char *)de + reclen); len -= reclen; tde = (struct target_dirent *)((char *)tde + treclen);
linux-user/syscall_defs.h +1 -1
@@ -258,7 +258,7 @@ 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[]; };struct target_dirent64 {
Signature · 1 line
ldv
Quoted 4 lines · Dmitry V. Levin
In case when
TARGET_ABI_BITS== 32 &&HOST_LONG_BITS== 64, the last byte of the target dirent structure (akad_typebyte) was never copied from the host dirent structure, thus breaking everything that relies on validd_typevalue, e.g. glob(3).
Looks pretty good, just one nit...
linux-user/syscall_defs.h struct target_dirent { · line 262
Quoted 11 lines
linux-user/syscall_defs.h +1
-1@@ -258,7 +258,7 @@ 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[]; };
Can you convert this struct to match coding style while you're touching it, please? (ie no hardcoded tabs). scripts/checkpatch.pl will let you know about this kind of thing.
thanks
-- PMM
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).
--- linux-user/syscall.c | 11 +++++------ linux-user/syscall_defs.h | 8 ++++---- 2 files changed, 9 insertions(+), 10 deletions(-)
linux-user/syscall.c +5 -6
@@ -7025,15 +7025,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, tde = target_dirp;while (len >0 ) { reclen = de->d_reclen; - treclen = reclen - (2 * (sizeof (long ) -sizeof (abi_long))); + tnamelen = reclen - offsetof(struct linux_dirent , d_name); +assert (tnamelen >=0 ); + treclen = tnamelen + offsetof(struct target_dirent , d_name); +assert (count1 + treclen <= count); tde->d_reclen =tswap16 (treclen); 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); +memcpy (tde->d_name, de->d_name, tnamelen); de = (struct linux_dirent *)((char *)de + reclen); len -= reclen; tde = (struct target_dirent *)((char *)tde + treclen);
linux-user/syscall_defs.h +4 -4
@@ -255,10 +255,10 @@ struct kernel_statfs { };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! */ +abi_long d_ino; +abi_long d_off; +unsigned short d_reclen; +char d_name[]; };struct target_dirent64 {
Signature · 1 line
ldv
Peter Maydell
Reviewed-byQuoted 6 lines · Dmitry V. Levin
In case when
TARGET_ABI_BITS== 32 &&HOST_LONG_BITS== 64, the last byte of the target dirent structure (akad_typebyte) was never copied from the host dirent structure, thus breaking everything that relies on validd_typevalue, e.g. glob(3).Signed-off-by Dmitry V. Levin <[email protected]>
(though not tested, since the 32 bit target I care about (ARM) uses getdents64 rather than getdents.)
-- PMM