[2/2] linux: clock_settime: Return proper value when passing NULL pointer

Message ID 20191108153344.10949-2-lukma@denx.de
State New
Headers show
Series
  • [1/2] linux: clock_settime: Remove check for nanoseconds validity
Related show

Commit Message

Lukasz Majewski Nov. 8, 2019, 3:33 p.m.
When in __clock_settime function (__TIMESIZE != 64) the const struct
timespec's *tp pointer is NULL, the Linux kernel syscall returns
-EFAULT.
Without this patch the glibc crashes (when dereferencing NULL pointer)
as the Linux kernel syscall is not reached at all.

There is no need for such check in the __clock_settime64, as this
pointer either goes directly to Linux kernel or the pointer to local
copy is used (ts64).
---
 sysdeps/unix/sysv/linux/clock_settime.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.20.1

Comments

Alistair Francis Nov. 8, 2019, 4:56 p.m. | #1
On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote:
>

> When in __clock_settime function (__TIMESIZE != 64) the const struct

> timespec's *tp pointer is NULL, the Linux kernel syscall returns

> -EFAULT.

> Without this patch the glibc crashes (when dereferencing NULL pointer)

> as the Linux kernel syscall is not reached at all.

>

> There is no need for such check in the __clock_settime64, as this

> pointer either goes directly to Linux kernel or the pointer to local

> copy is used (ts64).


Reviewed-by: Alistair Francis <alistair.francis@wdc.com>


Alistair

> ---

>  sysdeps/unix/sysv/linux/clock_settime.c | 9 ++++++++-

>  1 file changed, 8 insertions(+), 1 deletion(-)

>

> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c

> index 6706dbb31f..e358a18998 100644

> --- a/sysdeps/unix/sysv/linux/clock_settime.c

> +++ b/sysdeps/unix/sysv/linux/clock_settime.c

> @@ -51,7 +51,14 @@ __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)

>  int

>  __clock_settime (clockid_t clock_id, const struct timespec *tp)

>  {

> -  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*tp);

> +  struct __timespec64 ts64;

> +

> +  if (tp == NULL)

> +    {

> +      __set_errno (EFAULT);

> +      return -1;

> +    }

> +  ts64 = valid_timespec_to_timespec64 (*tp);

>

>    return __clock_settime64 (clock_id, &ts64);

>  }

> --

> 2.20.1

>
Joseph Myers Nov. 8, 2019, 5 p.m. | #2
On Fri, 8 Nov 2019, Alistair Francis wrote:

> On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote:

> >

> > When in __clock_settime function (__TIMESIZE != 64) the const struct

> > timespec's *tp pointer is NULL, the Linux kernel syscall returns

> > -EFAULT.

> > Without this patch the glibc crashes (when dereferencing NULL pointer)

> > as the Linux kernel syscall is not reached at all.

> >

> > There is no need for such check in the __clock_settime64, as this

> > pointer either goes directly to Linux kernel or the pointer to local

> > copy is used (ts64).

> 

> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>


This patch is contrary to glibc conventions.  There is explicitly no 
guarantee of whether a segfault or EFAULT occurs when a function is called 
with invalid arguments.  There should be no explicit checks for NULL 
pointers in cases where a segfault will reliably occur otherwise and any 
existing such checks should be removed from glibc.

https://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers

(And note the POSIX specification of EFAULT, "The reliable detection of 
this error cannot be guaranteed, and when not detected may result in the 
generation of a signal, indicating an address violation, which is sent to 
the process.".)

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
index 6706dbb31f..e358a18998 100644
--- a/sysdeps/unix/sysv/linux/clock_settime.c
+++ b/sysdeps/unix/sysv/linux/clock_settime.c
@@ -51,7 +51,14 @@  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
 int
 __clock_settime (clockid_t clock_id, const struct timespec *tp)
 {
-  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*tp);
+  struct __timespec64 ts64;
+
+  if (tp == NULL)
+    {
+      __set_errno (EFAULT);
+      return -1;
+    }
+  ts64 = valid_timespec_to_timespec64 (*tp);
 
   return __clock_settime64 (clock_id, &ts64);
 }