times(2) misbehaviour

[Denys Vlasenko]

On Sun, Jan 31, 2010 at 8:33 PM, Julien Boibessot
<julien.boibessot at free.fr> wrote:
> Hello,
>
> after a big bug hunt we also found that the implementation of the
> "times" system call in uClibc 0.9.29/0.9.30 is problematic on some 32
> bits architectures (arm926 in our case); like it was reported 2 years
> ago by Will.
>
> The fact that uClibc considers some Linux sys_times() return values as
> errors makes some user space applications become crazy (Qt in our case):
> when Linux "times" syscall's return value is inside uClibc errors range
> (cf INTERNAL_SYSCALL_ERROR_P macro), uClibc forward a (-1) error code to
> caller. An application may freeze during many seconds (40 if system tick
> is 10ms), if it's event handling relies on times() accuracy.
>
> Could maintainers of uClibc get inspired from that patch and do a
> correction for this problem please ?

Your patch seems to go to great lengths to catch
times(bad_pointer), right? -

+  clock_t ret = INTERNAL_SYSCALL (times, err, 1, buf);
+  if (INTERNAL_SYSCALL_ERROR_P (ret, err)
+      && __builtin_expect (INTERNAL_SYSCALL_ERRNO (ret, err) == EFAULT, 0))
+    {
+      /* This might be an error or not.  For architectures which have
+         no separate return value and error indicators we cannot
+         distinguish a return value of -1 from an error.  Do it the
+         hard way.  We crash applications which pass in an invalid BUF
+         pointer.  */
+#define touch(v) \
+      do {                                                                    \
+        clock_t temp = v;                                                     \
+        asm volatile ("" : "+r" (temp));                                      \
+        v = temp;                                                             \
+      } while (0)
+      touch (buf->tms_utime);
+      touch (buf->tms_stime);
+      touch (buf->tms_cutime);
+      touch (buf->tms_cstime);
...

Why bother? The program which passes bogus pointer to times()
is likely to crash anyway, say, when it will try to access
the data using that pointer.

Alternative (attached) patch simply removes error return check.
In assembly, the diff is:

 Disassembly of section .text.__GI_times:

@@ -43,13 +42,7 @@
 53                     push   %ebx
 89 fb                  mov    %edi,%ebx
 b8 2b 00 00 00         mov    $0x2b,%eax
 cd 80                  int    $0x80
 5b                     pop    %ebx
-3d 00 f0 ff ff         cmp    $0xfffff000,%eax
-76 0a                  jbe    21 <__GI_times+0x21>
-f7 d8                  neg    %eax
-a3 00 00 00 00         mov    %eax,0x0
-                       R_386_32        errno
-83 c8 ff               or     $0xffffffff,%eax
 5f                     pop    %edi
 c3                     ret


--
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 9.patch
Type: application/octet-stream
Size: 2167 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/uclibc/attachments/20100202/64498943/attachment.obj>