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

[Dmitry V. Levin]

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

> 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.

> 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.

> > 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
+		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)


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20140516/bbcbb9d2/attachment.bin>