This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: bug in times.c
- From: Holger Brunck <holger dot brunck at keymile dot com>
- To: Petr Baudis <pasky at ucw dot cz>, libc-alpha at sourceware dot org
- Cc: "Bigler, Stefan" <Stefan dot Bigler at keymile dot com>
- Date: Mon, 11 Mar 2013 12:51:13 +0100
- Subject: Re: bug in times.c
- References: <513DB23B.6030802@keymile.com> <20130311112350.GB31274@machine.or.cz>
On 03/11/2013 12:23 PM, Petr Baudis wrote:
> On Mon, Mar 11, 2013 at 11:30:19AM +0100, Holger Brunck wrote:
>> Here is the diff to the current glibc git tree for the proposed solution:
>>
>> diff --git a/sysdeps/unix/sysv/linux/times.c b/sysdeps/unix/sysv/linux/times.c
>> index f3b5f01..ba20e5b 100644
>> --- a/sysdeps/unix/sysv/linux/times.c
>> +++ b/sysdeps/unix/sysv/linux/times.c
>> @@ -39,11 +39,12 @@ __times (struct tms *buf)
>> asm volatile ("" : "+r" (temp)); \
>> v = temp; \
>> } while (0)
>> - touch (buf->tms_utime);
>> - touch (buf->tms_stime);
>> - touch (buf->tms_cutime);
>> - touch (buf->tms_cstime);
>> -
>> + if (buf != NULL) {
>> + touch (buf->tms_utime);
>> + touch (buf->tms_stime);
>> + touch (buf->tms_cutime);
>> + touch (buf->tms_cstime);
>> + }
>> /* If we come here the memory is valid and the kernel did not
>> return an EFAULT error. Return the value given by the kernel. */
>> }
>>
>> Any comments on this? should I send a regular git patch?
>
> I think a slightly more elegant version would be
>
> 2013-03-11 Petr Baudis <pasky@ucw.cz>
>
> * sysdeps/unix/sysv/linux/times.c (__times): On EFAULT, test for
> non-NULL pointer before the memory validity test. Pointed out
> by Holger Brunck <holger.brunck@keymile.com>.
>
> diff --git a/sysdeps/unix/sysv/linux/times.c b/sysdeps/unix/sysv/linux/times.c
> index f3b5f01..200fc2d 100644
> --- a/sysdeps/unix/sysv/linux/times.c
> +++ b/sysdeps/unix/sysv/linux/times.c
> @@ -26,7 +26,8 @@ __times (struct tms *buf)
> INTERNAL_SYSCALL_DECL (err);
> 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))
> + && __builtin_expect (INTERNAL_SYSCALL_ERRNO (ret, err) == EFAULT, 0)
> + && buf != NULL)
> {
> /* This might be an error or not. For architectures which have
> no separate return value and error indicators we cannot
> @@ -44,7 +45,8 @@ __times (struct tms *buf)
> touch (buf->tms_cutime);
> touch (buf->tms_cstime);
>
> - /* If we come here the memory is valid and the kernel did not
> + /* If we come here the memory is valid (or BUF is NULL, which is
> + * a valid condition for the kernel syscall) and the kernel did not
> return an EFAULT error. Return the value given by the kernel. */
> }
>
>
yep agreed this is more elegant.
Regards
Holger