x32 gcc sign extension bug (was: Long/address printing in non-native personalities)

[Dmitry V. Levin]

Hi,

On Fri, Jun 10, 2016 at 06:58:57PM +0300, Eugene Syromyatnikov wrote:
> Hello.
> 
> It's been brought to my attention that one of strace tests (nsyscalls) is
> failing when x32 ABI is used.
> 
> $ cat tests-mx32/nsyscalls.log
> 1c1
> < syscall_546(0xbadc0ded, 0xbadc1ded, 0xbadc2ded, 0xbadc3ded, 0xbadc4ded, 
> 0xbadc5ded) = -1 (errno 38)
> ---
> > syscall_546(0xbadc0ded, 0xbadc1ded, 0xbadc2ded, 0xbadc3ded, 0xbadc4ded, 
> > 0xbadc5ded) = -1 (errno 38)
> nsyscalls.test: failed test: ../strace -e trace=none ./nsyscalls output 
> mismatch
> FAIL nsyscalls.test (exit status: 1)
> 
> Upon investigation, it has been revealed that the problem is in the way strace
> handles tracee's long integer variables, which, in turn, has been brought to
> light by the quirk in the way argument passing code being generated by GCC.
> More specifically, when a 6-parameter syscall is called (as nsyscalls tests
> does), 7 parameters should be passed to libc's syscall() wrapper: syscall
> number itself and its parameters. As a result, 7th argument should be passed
> via stack in accordance with x32 ABI (
> https://docs.google.com/viewer?a=v=sites=ZGVmYXVsdGRvbWFpbnx4MzJhYml8Z3g6MzVkYzQwOWIwOGU4YzViYw
> , page 22). Also, in conformance with x32 ABI, all arguments should be 
> extended
> to 8 bytes in size. Standard does not explicitly specify the way this should 
> be
> performed, and that's where GCC does something strange: it sign-extends
> parameters passed via stack while zero-extends parameters passed via registers
> (or, more precisely, doesn't care about higher bits at all in latter case, for
> obvious reasons):
> 
> Dump of assembler code for function main:
> 0x00400370 <+0>: push %rbx
> 0x00400371 <+1>: mov $0xbadc4ded,%r9d
> 0x00400377 <+7>: mov $0xbadc3ded,%r8d
> 0x0040037d <+13>: mov $0xbadc2ded,%ecx
> 0x00400382 <+18>: mov $0xbadc1ded,%edx
> 0x00400387 <+23>: mov $0xbadc0ded,%esi
> 0x0040038c <+28>: sub $0x8,%esp
> 0x0040038f <+31>: mov $0x4222,%edi
> 0x00400394 <+36>: xor %eax,%eax
> => 0x00400396 <+38>: pushq $0xbadc5ded
> 0x0040039b <+43>: callq 0x400350 

Eugene, thanks for the analysis, this explains the issue.

GCC actually produces the following instructions:

pushq   %rbx
movl$-1159967251, %r9d
movl$-1159971347, %r8d
movl$-1159975443, %ecx
movl$-1159979539, %edx
movl$-1159983635, %esi
subl$8, %esp
movl$1073742370, %edi
xorl%eax, %eax
pushq   $-1159963155
callsyscall

This is translated into the code you've cited.

> And this is what leads to test failure — strace does not care about tracee's
> size of long when it prints syscall arguments (printargs() in syscall.c simply
> calls tprintf("%s%#lx", i ? ", " : "", tcp->u_arg[i])). Moreover, it similarly
> does not care about tracee's pointer size in places where it prints them (my
> first attempt to fix this issue was by (incorrectly) utilising
> printnum_ulong(), but it failed hilariously, since umoven_or_printaddr(), 
> which
> is apparently called before actual printing, leads to precisely the same
> results since it simply calls tprintf("%#lx", addr)). As a result, I propose
> the following change where all cases when tracee's long (or pointer, since 
> they
> have the same size in Linux ABIs AFAIK) should be printed are handled via
> printaddr() and logic regarding dispatching current_wordsize is added to
> printaddr() itself (maybe it also should be renamed to printlong() or 
> something
> more suitable).

In strace, we are trying to display things from the kernel perspective rather
than from the userspace perspective.  When these points of view don't
match, we are trying to present the kernel's side of the story.

For example, when the kernel takes a pid argument, we print it as int.

With regards to address printing, the x86_64 kernel operates with
addresses as whole 64-bit words both in x86_64 and x32 modes, so explicit
cast to a 32-bit word won't be an appropriate solution.

Imagine an invocation of a 6-arg syscall with the last argument being an
address, e.g. pselect6.  Such syscall cannot be invoked properly on x32
using a syscall() wrapper because, as you've shown, the 32-bit address
will be sign-extended, producing a potentially wrong address that would
result to a failed syscall; I think strace would be less useful if it
stopped showing the 64-bit address actually passed to this syscall.

For this reason, I recommend x86_64 strace over native x32 strace for
tracing x32 processes because x32 strace doesn't show these high bits
(which are visible to the kernel) while x86_64 strace does show them.

Said that, I fully support your proposal of printing addresses consistently
using printaddr().  There are still about two dozens cases that should be
converted to use printaddr() instead of direct tprintf() calls.

printargs(), however, is used in different contexts, so changing it
to use printaddr() 

Long/address printing in non-native personalities

[Eugene Syromyatnikov]

Hello.

It's been brought to my attention that one of strace tests (nsyscalls) is
failing when x32 ABI is used.

$ cat tests-mx32/nsyscalls.log
1c1
< syscall_546(0xbadc0ded, 0xbadc1ded, 0xbadc2ded, 0xbadc3ded, 0xbadc4ded, 
0xbadc5ded) = -1 (errno 38)
---
> syscall_546(0xbadc0ded, 0xbadc1ded, 0xbadc2ded, 0xbadc3ded, 0xbadc4ded, 
> 0xbadc5ded) = -1 (errno 38)
nsyscalls.test: failed test: ../strace -e trace=none ./nsyscalls output mismatch
FAIL nsyscalls.test (exit status: 1)

Upon investigation, it has been revealed that the problem is in the way strace
handles tracee's long integer variables, which, in turn, has been brought to
light by the quirk in the way argument passing code being generated by GCC.
More specifically, when a 6-parameter syscall is called (as nsyscalls tests
does), 7 parameters should be passed to libc's syscall() wrapper: syscall
number itself and its parameters. As a result, 7th argument should be passed
via stack in accordance with x32 ABI (
https://docs.google.com/viewer?a=v=sites=ZGVmYXVsdGRvbWFpbnx4MzJhYml8Z3g6MzVkYzQwOWIwOGU4YzViYw
, page 22). Also, in conformance with x32 ABI, all arguments should be extended
to 8 bytes in size. Standard does not explicitly specify the way this should be
performed, and that's where GCC does something strange: it sign-extends
parameters passed via stack while zero-extends parameters passed via registers
(or, more precisely, doesn't care about higher bits at all in latter case, for
obvious reasons):

Dump of assembler code for function main:
0x00400370 <+0>: push %rbx
0x00400371 <+1>: mov $0xbadc4ded,%r9d
0x00400377 <+7>: mov $0xbadc3ded,%r8d
0x0040037d <+13>: mov $0xbadc2ded,%ecx
0x00400382 <+18>: mov $0xbadc1ded,%edx
0x00400387 <+23>: mov $0xbadc0ded,%esi
0x0040038c <+28>: sub $0x8,%esp
0x0040038f <+31>: mov $0x4222,%edi
0x00400394 <+36>: xor %eax,%eax
=> 0x00400396 <+38>: pushq $0xbadc5ded
0x0040039b <+43>: callq 0x400350 

And this is what leads to test failure — strace does not care about tracee's
size of long when it prints syscall arguments (printargs() in syscall.c simply
calls tprintf("%s%#lx", i ? ", " : "", tcp->u_arg[i])). Moreover, it similarly
does not care about tracee's pointer size in places where it prints them (my
first attempt to fix this issue was by (incorrectly) utilising
printnum_ulong(), but it failed hilariously, since umoven_or_printaddr(), which
is apparently called before actual printing, leads to precisely the same
results since it simply calls tprintf("%#lx", addr)). As a result, I propose
the following change where all cases when tracee's long (or pointer, since they
have the same size in Linux ABIs AFAIK) should be printed are handled via
printaddr() and logic regarding dispatching current_wordsize is added to
printaddr() itself (maybe it also should be renamed to printlong() or something
more suitable).

I'm not really sure whether GCC's behaviour is correct, but it (at least)
doesn't contradict common sense (and x32 ABI spec) as far as I can see, since
higher 32 bits are not used anyway.


---
 syscall.c |7 +--
 util.c|   25 +
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/syscall.c b/syscall.c
index d71ead3..4d9d152 100644
--- a/syscall.c
+++ b/syscall.c
@@ -636,8 +636,11 @@ printargs(struct tcb *tcp)
if (entering(tcp)) {
int i;
int n = tcp->s_ent->nargs;
-   for (i = 0; i < n; i++)
-   tprintf("%s%#lx", i ? ", " : "", tcp->u_arg[i]);
+   for (i = 0; i < n; i++) {
+   if (i)
+   tprints(", ");
+   printaddr(tcp->u_arg[i]);
+   }
}
return 0;
 }
diff --git a/util.c b/util.c
index 9c8c978..ba0d2a6 100644
--- a/util.c
+++ b/util.c
@@ -398,10 +398,15 @@ printflags64(const struct xlat *xlat, uint64_t flags, 
const char *dflt)
 void
 printaddr(const long addr)
 {
-   if (!addr)
+   if (!addr) {
tprints("NULL");
-   else
-   tprintf("%#lx", addr);
+   } else {
+   if (current_wordsize sizeof(int)) {
+   tprintf("%#lx", addr);
+   } else {
+   tprintf("%#x", (unsigned int)addr);
+   }
+   }
 }
 
 #define DEF_PRINTNUM(name, type) \
@@ -761,7 +766,7 @@ printpathn(struct tcb *tcp, long addr, unsigned int n)
/* Fetch one byte more to find out whether path length n. */
nul_seen = umovestr(tcp, addr, n + 1, path);
if (nul_seen < 0)
-   tprintf("%#lx", addr);
+   printaddr(addr);
else {
path[n++] = '\0';
print_quoted_string(path, n, QUOTE_0_TERMINATED);
@@ -812,7 +817,7 @@ printstr(struct tcb *tcp, long addr, long len)
 * because string_quote may look one byte ahead.
 */
if (umovestr(tcp, addr, size + 1, str) <