[PATCH] Fix {get, set}rlimit decoding with unreliable SIZEOF_RLIM_T

[James Hogan]

Hi Dmitry,

On 16/05/14 17:02, Dmitry V. Levin wrote:
> On Fri, May 16, 2014 at 12:53:29PM +0100, James Hogan wrote:
>> On 14/05/14 15:17, Dmitry V. Levin wrote:
> [...]
>>> I have two questions wrt this patch:
>>>
>>> 1. Removing the conditional would result with print_rlimit32 defined for
>>> all architectures, including pure 64bit ones.  I suppose we need a check
>>> like this anyway:
>>>
>>> #if PERSONALITY0_WORDSIZE == 4 || \
>>>     (SUPPORTED_PERSONALITIES > 1 && PERSONALITY1_WORDSIZE == 4) || \
>>>     (SUPPORTED_PERSONALITIES > 2 && PERSONALITY2_WORDSIZE == 4)
>>
>> There don't appear to be any arches supported (according to defs.h) with
>>> 2 personalities with all wordsizes the same.
>>
>> Where there are 2 personalities with the same wordsize the definition of
>> current_wordsize takes this into account and defines it as
>> PERSONALITY0_WORDSIZE.
> 
> Yes, so the check could be simplified to
> #if !defined(current_wordsize) || current_wordsize == 4

Unfortunately this confuses preprocessor when using this definition:
# define PERSONALITY0_WORDSIZE (int)(sizeof(long))

Maybe with this extra hunk?

diff --git a/defs.h b/defs.h
index 074c8f0..4e06a92 100644
--- a/defs.h
+++ b/defs.h
@@ -370,7 +370,7 @@ struct arm_pt_regs {
 # define DEFAULT_PERSONALITY 0
 #endif
 #ifndef PERSONALITY0_WORDSIZE
-# define PERSONALITY0_WORDSIZE (int)(sizeof(long))
+# define PERSONALITY0_WORDSIZE SIZEOF_LONG
 #endif

 #if defined(I386) || defined(X86_64)

>> So this would seem to allow the compiler to optimise out whichever
>> print_rlimit{32,64} is unused, and a quick look at the x86_64 assembly
>> output (gcc 4.7.3 -O2) with a hacked current_wordsize in resource.c
>> confirms that.
> 
> Yes, and some day the compiler will also optimize out decode_rlimit
> because it ends up the same as decode_rlimit64.
> I'd rather not wait when it happens and add the check.

Ah yes, I see the reasoning behind it now :)

> 
>> There is of course the "!addr" and "!verbose(tcp || (exiting(tcp) &&
>> syserror(tcp))" conditions which seem to be skipped for the pure 64-bit
>> case at the moment which I'm not sure was intentional anyway?
> 
> Hopefully, they are not skipped.  I admit these function names
> (sprint_rlim64, print_rlimit64, and decode_rlimit64) are somewhat
> confusing.  The last of them does all these top level checks.

Yep, my mistake. I was confusing the functions and reading print_rlimit64.

> 
>>> 2. I'm not sure about architectures where kernel wordsize is 8 but user
>>
>>> wordsize is 4 (like x32 and mips n32); what's the correct rlim_t for these
>>> architectures?
>>
>> Hmm, that's a good point.
>>
>> MIPS n32 appears to use the default __kernel_ulong_t define of unsigned
>> long (32bit) so it would work fine I think (although sadly it appears
>> that strace doesn't support personalities on MIPS yet).
>>
>> x32 appears to define __kernel_ulong_t as unsigned long long (64bit), so
>> strace handling of {get,set}rlimit would already appear to be broken for
>> that (this patch doesn't make that any worse).
> 
> Thanks.  I suppose this patch fixes all these rlim_t issues:
> 
> --- a/configure.ac
> +++ b/configure.ac
> @@ -309,7 +309,6 @@ AC_CACHE_CHECK([for BLKGETSIZE64], [ac_cv_have_blkgetsize64],
>  AC_CHECK_SIZEOF([long])
>  AC_CHECK_SIZEOF([long long])
>  AC_CHECK_SIZEOF([off_t],,[#include <sys/types.h>])
> -AC_CHECK_SIZEOF([rlim_t],,[#include <sys/resource.h>])
>  
>  AC_CACHE_CHECK([for SA_RESTORER], [st_cv_sa_restorer],
>  	       [st_cv_sa_restorer="$(echo SA_RESTORER |
> --- a/resource.c
> +++ b/resource.c
> @@ -88,10 +88,6 @@ static const struct xlat resources[] = {
>  	XLAT_END
>  };
>  
> -#if !(SIZEOF_RLIM_T == 4 || SIZEOF_RLIM_T == 8)
> -# error "Unsupported SIZEOF_RLIM_T value"
> -#endif
> -
>  static const char *
>  sprint_rlim64(uint64_t lim)
>  {
> @@ -135,7 +131,7 @@ decode_rlimit64(struct tcb *tcp, unsigned long addr)
>  		print_rlimit64(tcp, addr);
>  }
>  
> -#if SIZEOF_RLIM_T == 4 || SUPPORTED_PERSONALITIES > 1
> +#if !defined(current_wordsize) || current_wordsize == 4
>  
>  static const char *
>  sprint_rlim32(uint32_t lim)
> @@ -176,22 +172,22 @@ decode_rlimit(struct tcb *tcp, unsigned long addr)
>  	else if (!verbose(tcp) || (exiting(tcp) && syserror(tcp)))
>  		tprintf("%#lx", addr);
>  	else {
> -# if SIZEOF_RLIM_T == 4
> -		print_rlimit32(tcp, addr);
> +# ifdef X32

Does this need to take X86_64 into account too, since I presume
personality 1 is X32 too?

Thanks
James

> +		if (current_personality == 1)
>  # else
>  		if (current_wordsize == 4)
> +# endif
>  			print_rlimit32(tcp, addr);
>  		else
>  			print_rlimit64(tcp, addr);
> -# endif
>  	}
>  }
>  
> -#else /* SIZEOF_RLIM_T == 8 && SUPPORTED_PERSONALITIES == 1 */
> +#else /* defined(current_wordsize) && current_wordsize != 4 */
>  
>  # define decode_rlimit decode_rlimit64
>  
> -#endif /* SIZEOF_RLIM_T == 4 || SUPPORTED_PERSONALITIES > 1 */
> +#endif
>  
>  int
>  sys_getrlimit(struct tcb *tcp)
> 
>