Re: [SCM] strace branch, master, updated. v4.6-124-geb0e3e8

[Denys V. <dvl...@re...>]

On 02/15/2013 02:28 AM, Dmitry V. Levin wrote:
> Denys,
> 
> On Tue, Aug 30, 2011 at 05:04:35PM +0000, Denys Vlasenko wrote:
> [...]
>> @@ -1553,14 +1536,24 @@ syscall_enter(struct tcb *tcp)
>>  		if (upeek(tcp, REG_GENERAL(syscall_regs[i]), &tcp->u_arg[i]) < 0)
>>  			return -1;
>>  # elif defined(X86_64)
>> -	static const int argreg[SUPPORTED_PERSONALITIES][MAX_ARGS] = {
>> -		{ 8 * RDI, 8 * RSI, 8 * RDX, 8 * R10, 8 * R8 , 8 * R9  }, /* x86-64 ABI */
>> -		{ 8 * RBX, 8 * RCX, 8 * RDX, 8 * RSI, 8 * RDI, 8 * RBP }  /* i386 ABI */
>> -	};
>> -
>> -	for (i = 0; i < nargs; ++i)
>> -		if (upeek(tcp, argreg[current_personality][i], &tcp->u_arg[i]) < 0)
>> -			return -1;
>> +	(void)i;
>> +	(void)nargs;
>> +	if (current_personality == 0) { /* x86-64 ABI */
>> +		tcp->u_arg[0] = x86_64_regs.rdi;
>> +		tcp->u_arg[1] = x86_64_regs.rsi;
>> +		tcp->u_arg[2] = x86_64_regs.rdx;
>> +		tcp->u_arg[3] = x86_64_regs.r10;
>> +		tcp->u_arg[4] = x86_64_regs.r8;
>> +		tcp->u_arg[5] = x86_64_regs.r9;
>> +	} else { /* i386 ABI */
>> +		/* Sign-extend lower 32 bits */
>> +		tcp->u_arg[0] = (long)(int)x86_64_regs.rbx;
>> +		tcp->u_arg[1] = (long)(int)x86_64_regs.rcx;
>> +		tcp->u_arg[2] = (long)(int)x86_64_regs.rdx;
>> +		tcp->u_arg[3] = (long)(int)x86_64_regs.rsi;
>> +		tcp->u_arg[4] = (long)(int)x86_64_regs.rdi;
>> +		tcp->u_arg[5] = (long)(int)x86_64_regs.rbp;
>> +	}
>>  # elif defined(MICROBLAZE)
>>  	for (i = 0; i < nargs; ++i)
>>  		if (upeek(tcp, (5 + i) * 4, &tcp->u_arg[i]) < 0)
> 
> This sign-extending on x86-64 appeared to be not so good after all.

The widening here is meant to avoid adding zillions of widening
operations all over syscall handlers. I think it is not controversial
that widening here is useful?

Unfortunately, neither signed nor unsigned widening is
without drawbacks.

Unsigned widening makes it necessary to fix printing integer
parameters, e.g. pids:

sys_kill(struct tcb *tcp)
{
        if (entering(tcp)) {
                long pid = tcp->u_arg[0];
#if SUPPORTED_PERSONALITIES > 1
                /* Sign-extend a 32-bit value when that's what it is. */
                if (current_wordsize < sizeof pid)
                        pid = (long) (int) pid;
#endif
                tprintf("%ld, %s", pid, signame(tcp->u_arg[1]));
        }
        return 0;
}

> I don't remember many syscalls taking signed long arguments

They don't have to be longs. Any signed argument needs signed widening.
See example above.

> but there are
> a lot that take pointers, and these are displayed wrongly now, e.g.
> 
> $ strace -etrace=uname,mprotect,mmap2,munmap,set_thread_area,fstat64 -everbose=none /path/to/chroot32/bin/cat </dev/null 
> [ Process PID=12345 runs in 32 bit mode. ]
> uname(0xffffffffbfbf4a5a)               = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb7fda000
> fstat64(3, 0xffffffffbfbf46a0)          = 0

And as you just observed, signed widening messes up unsigned quantities,
such as pointers.

Do you have a feeling unsigned cup of poison is less painful?

-- 
vda