[Bug nptl/18138] New: sem_timedwait should not convert absolute timeout to relative

[Bug nptl/18138] New: sem_timedwait should not convert absolute timeout to relative

[jsm28 at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18138

            Bug ID: 18138
           Summary: sem_timedwait should not convert absolute timeout to
                    relative
           Product: glibc
           Version: 2.21
            Status: NEW
          Severity: normal
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: jsm28 at gcc dot gnu.org
                CC: drepper.fsp at gmail dot com

sem_timedwait converts absolute timeouts to relative to pass them to the futex
syscall.  (Before the recent reimplementation, on x86_64 it used
FUTEX_CLOCK_REALTIME, but not on other architectures.)

Correctly implementing POSIX requirements, however, requires use of
FUTEX_CLOCK_REALTIME; passing a relative timeout to the kernel does not conform
to POSIX.  The POSIX specification for sem_timedwait says "The timeout shall be
based on the CLOCK_REALTIME clock.".  The POSIX specification for clock_settime
says "If the value of the CLOCK_REALTIME clock is set via clock_settime(), the
new value of the clock shall be used to determine the time of expiration for
absolute time services based upon the CLOCK_REALTIME clock. This applies to the
time at which armed absolute timers expire. If the absolute time requested at
the invocation of such a time service is before the new value of the clock, the
time service shall expire immediately as if the clock had reached the requested
time normally.".  If a relative timeout is passed to the kernel, it is
interpreted according to the CLOCK_MONOTONIC clock, and so fails to meet that
POSIX requirement in the event of clock changes.

Testcase below (requires root, changes the system clock and leaves the clock
changed).  Testing a patch.

#include <errno.h>
#include <inttypes.h>
#include <semaphore.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <unistd.h>

int
main (void)
{
  struct timespec old_time, new_time, timeout, final_time;
  if (clock_gettime (CLOCK_REALTIME, &old_time) != 0)
    {
      perror ("clock_gettime");
      exit (EXIT_FAILURE);
    }
  new_time = old_time;
  timeout = old_time;
  new_time.tv_sec += 30;
  timeout.tv_sec += 20;
  pid_t pid = fork ();
  if (pid == -1)
    {
      perror ("fork");
      exit (EXIT_FAILURE);
    }
  if (pid == 0)
    {
      /* Child.  */
      if (sleep (1) != 0)
        {
          perror ("sleep");
          exit (EXIT_FAILURE);
        }
      if (clock_settime (CLOCK_REALTIME, &new_time) != 0)
        {
          perror ("clock_settime");
          exit (EXIT_FAILURE);
        }
      exit (EXIT_SUCCESS);
    }
  /* Parent.  */
  sem_t sem;
  if (sem_init (&sem, 0, 0) != 0)
    {
      perror ("sem_init");
      exit (EXIT_FAILURE);
    }
  int ret = sem_timedwait (&sem, &timeout);
  if (ret != -1 || errno != ETIMEDOUT)
    {
      fprintf (stderr, "sem_timedout returned %d (errno = %d: %m)\n",
               ret, errno);
      exit (EXIT_FAILURE);
    }
  if (clock_gettime (CLOCK_REALTIME, &final_time) != 0)
    {
      perror ("clock_gettime");
      exit (EXIT_FAILURE);
    }
  int64_t time_diff
    = (INT64_C(1000000000) * (final_time.tv_sec - new_time.tv_sec)
       + (final_time.tv_nsec - new_time.tv_nsec));
  fprintf (stderr, "time difference %"PRId64"ns\n", time_diff);
  if (time_diff < 0 || time_diff > 1000000000)
    {
      fprintf (stderr, "bad time difference\n");
      exit (EXIT_FAILURE);
    }
  fprintf (stderr, "time difference OK\n");
  exit (EXIT_SUCCESS);
}

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/18138] sem_timedwait should not convert absolute timeout to relative

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18138

--- Comment #1 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  c2f5813ae0a68f6c6d69e66dac2da6e46b9df034 (commit)
      from  3382c079da26c3e5b02fb68764ffd8e2834e4806 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c2f5813ae0a68f6c6d69e66dac2da6e46b9df034

commit c2f5813ae0a68f6c6d69e66dac2da6e46b9df034
Author: Joseph Myers <joseph@codesourcery.com>
Date:   Wed Mar 18 17:05:38 2015 +0000

    Make sem_timedwait use FUTEX_CLOCK_REALTIME (bug 18138).

    sem_timedwait converts absolute timeouts to relative to pass them to
    the futex syscall.  (Before the recent reimplementation, on x86_64 it
    used FUTEX_CLOCK_REALTIME, but not on other architectures.)

    Correctly implementing POSIX requirements, however, requires use of
    FUTEX_CLOCK_REALTIME; passing a relative timeout to the kernel does
    not conform to POSIX.  The POSIX specification for sem_timedwait says
    "The timeout shall be based on the CLOCK_REALTIME clock.".  The POSIX
    specification for clock_settime says "If the value of the
    CLOCK_REALTIME clock is set via clock_settime(), the new value of the
    clock shall be used to determine the time of expiration for absolute
    time services based upon the CLOCK_REALTIME clock. This applies to the
    time at which armed absolute timers expire. If the absolute time
    requested at the invocation of such a time service is before the new
    value of the clock, the time service shall expire immediately as if
    the clock had reached the requested time normally.".  If a relative
    timeout is passed to the kernel, it is interpreted according to the
    CLOCK_MONOTONIC clock, and so fails to meet that POSIX requirement in
    the event of clock changes.

    This patch makes sem_timedwait use lll_futex_timed_wait_bitset with
    FUTEX_CLOCK_REALTIME when possible, as done in some other places in
    NPTL.  FUTEX_CLOCK_REALTIME is always available for supported Linux
    kernel versions; unavailability of lll_futex_timed_wait_bitset is only
    an issue for hppa (an issue noted in
    <https://sourceware.org/glibc/wiki/PortStatus>, and fixed by the
    unreviewed
    <https://sourceware.org/ml/libc-alpha/2014-12/msg00655.html> that
    removes the hppa lowlevellock.h completely).

    In the FUTEX_CLOCK_REALTIME case, the glibc code still needs to check
    for negative tv_sec and handle that as timeout, because the Linux
    kernel returns EINVAL not ETIMEDOUT for that case, so resulting in
    failures of nptl/tst-abstime and nptl/tst-sem13 in the absence of that
    check.  If we're trying to distinguish between Linux-specific and
    generic-futex NPTL code, I suppose having this in an nptl/ file isn't
    ideal, but there doesn't seem to be any better place at present.

    It's not possible to add a testcase for this issue to the testsuite
    because of the requirement to change the system clock as part of a
    test (this is a case where testing would require some form of
    container, with root in that container, and one whose CLOCK_REALTIME
    is isolated from that of the host; I'm not sure what forms of
    containers, short of a full virtual machine, provide that clock
    isolation).

    Tested for x86_64.  Also tested for powerpc with the testcase included
    in the bug.

        [BZ #18138]
        * nptl/sem_waitcommon.c: Include <kernel-features.h>.
        (futex_abstimed_wait)
        [__ASSUME_FUTEX_CLOCK_REALTIME && lll_futex_timed_wait_bitset]:
        Use lll_futex_timed_wait_bitset with FUTEX_CLOCK_REALTIME instead
        of lll_futex_timed_wait.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog             |    9 +++++++++
 NEWS                  |    2 +-
 nptl/sem_waitcommon.c |   15 +++++++++++++++
 3 files changed, 25 insertions(+), 1 deletions(-)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/18138] sem_timedwait should not convert absolute timeout to relative

[jsm28 at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18138

Joseph Myers <jsm28 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #2 from Joseph Myers <jsm28 at gcc dot gnu.org> ---
Fixed for 2.22.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/18138] sem_timedwait should not convert absolute timeout to relative

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18138

--- Comment #3 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  a9fe4c5aa8e53ee30f7d0a1c878391d5d6324e6e (commit)
      from  afcd9480feca651eef436d8438b783dde5c3bbb2 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=a9fe4c5aa8e53ee30f7d0a1c878391d5d6324e6e

commit a9fe4c5aa8e53ee30f7d0a1c878391d5d6324e6e
Author: Joseph Myers <joseph@codesourcery.com>
Date:   Wed Mar 25 15:17:54 2015 +0000

    Support six-argument syscalls from C for 32-bit x86, use generic
lowlevellock-futex.h (bug 18138).

    This patch follows the approach outlined in
    <https://sourceware.org/ml/libc-alpha/2015-03/msg00656.html> to
    support six-argument syscalls from INTERNAL_SYSCALL for 32-bit x86,
    making them call a function __libc_do_syscall that takes the syscall
    number and three syscall arguments in the registers in which the
    kernel expects them, along with a pointer to a structure containing
    the other three arguments.

    In turn, this allows the generic lowlevellock-futex.h to be used on
    32-bit x86, so supporting lll_futex_timed_wait_bitset (and so allowing
    FUTEX_CLOCK_REALTIME to be used in various cases, so fixing bug 18138
    for 32-bit x86 and leaving hppa as the only architecture missing
    lll_futex_timed_wait_bitset).  The change to lowlevellock.h's
    definition of SYS_futex is because the generic lowlevelloc-futex.h
    ends up bringing in bits/syscall.h which defines SYS_futex to
    __NR_futex, so resulting in redefinition errors.  The revised
    definition in lowlevellock.h is in line with what the x86_64 version
    does.

    __libc_do_syscall is only needed in libpthread at present (meaning
    nothing special needs to be done to make it shared-only in most
    libraries containing it, static in libc only, as on ARM).

    Tested for 32-bit x86, with the glibc testsuite and with the test in
    bug 18138.  The failures seen

    FAIL: nptl/tst-cleanupx4
    FAIL: rt/tst-cpuclock2

    are pre-existing.

        [BZ #18138]
        * sysdeps/unix/sysv/linux/i386/sysdep.h (struct
        libc_do_syscall_args): New structure.
        (INTERNAL_SYSCALL_MAIN_0): New macro.
        (INTERNAL_SYSCALL_MAIN_1): Likewise.
        (INTERNAL_SYSCALL_MAIN_2): Likewise.
        (INTERNAL_SYSCALL_MAIN_3): Likewise.
        (INTERNAL_SYSCALL_MAIN_4): Likewise.
        (INTERNAL_SYSCALL_MAIN_5): Likewise.
        (INTERNAL_SYSCALL_MAIN_6): Likewise.  Call __libc_do_syscall.
        (INTERNAL_SYSCALL): Define to use INTERNAL_SYSCALL_MAIN_##nr.
        Replace conditional definitions by conditional definitions of ....
        (INTERNAL_SYSCALL_MAIN_INLINE): ... this.  New macro.
        * sysdeps/unix/sysv/linux/i386/libc-do-syscall.S: New file.
        * sysdeps/unix/sysv/linux/i386/Makefile [$(subdir) = nptl]
        (libpthread-sysdep_routines): Add libc-do-syscall.
        * sysdeps/unix/sysv/linux/i386/lowlevellock-futex.h: Remove file.
        * sysdeps/unix/sysv/linux/i386/lowlevellock.h (SYS_futex): Define
        to __NR_futex not 240.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                          |   22 +++
 sysdeps/unix/sysv/linux/i386/Makefile              |    5 +
 .../i386/{call_pselect6.S => libc-do-syscall.S}    |   44 ++----
 sysdeps/unix/sysv/linux/i386/lowlevellock-futex.h  |  137 --------------------
 sysdeps/unix/sysv/linux/i386/lowlevellock.h        |    2 +-
 sysdeps/unix/sysv/linux/i386/sysdep.h              |   65 +++++++--
 6 files changed, 93 insertions(+), 182 deletions(-)
 copy sysdeps/unix/sysv/linux/i386/{call_pselect6.S => libc-do-syscall.S} (63%)
 delete mode 100644 sysdeps/unix/sysv/linux/i386/lowlevellock-futex.h

-- 
You are receiving this mail because:
You are on the CC list for the bug.