[BZ,#16996] get_nprocs: Only return explictly set cache values

  The implementation of __get_nprocs uses a stactic variable to cache
the value of the current number of processors.  The caching breaks when
'time (NULL) == 0':

  $ cat nproc.c
  #include <stdio.h>
  #include <time.h>
  #include <sys/time.h>

  int main(int argc, char *argv[])
  {
    time_t t;
    struct timeval tv = {0, 0};
    printf("settimeofday({0, 0}, NULL) = %d\n", settimeofday(&tv, NULL));
    t = time(NULL);
    printf("Time: %d, CPUs: %d\n", (unsigned int)t, get_nprocs());
    return 0;
  }
  $ gcc -O3 nproc.c
  $ ./a.out
  settimeofday({0, 0}, NULL) = -1
  Time: 1401311578, CPUs: 4
  $ sudo ./a.out
  settimeofday({0, 0}, NULL) = 0
  Time: 0, CPUs: 0

The problem is with the condition used to check whether a cached
value should be returned or not:

  static int cached_result;
  static time_t timestamp;

  time_t now = time (NULL);
  time_t prev = timestamp;
  atomic_read_barrier ();
  if (now == prev)
    return cached_result;

When 'timestamp == 0', as in the above test case, an unitialized
cache value is returned.

This patch fixes the problem by ensuring that 'cached_result' has
been set at least once before returning it.

I checked for regressions with 'make check' and verified that the
described test case works, but couldn't find a good way to automate
the test in the GLIBC testsuite.

OK?

2014-06-12  Meador Inge  <meadori@codesourcery.com>

	[BZ #16996]
	sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Ensure that
	the cached result has been set before returning it.

---
 sysdeps/unix/sysv/linux/getsysstats.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On 06/12/2014 03:54 PM, Siddhesh Poyarekar wrote:

> Oh yes, so:
> 
>     (now == prev && now != 0)

That is indeed simpler and fixes the correctness issue, but now caching is
disabled for the 'time (NULL) == 0' case.  Therefore two close call to
'get_nprocs' at that time point will cause the value to be computed both times.
I prefer the first patch where correctness and the caching behavior is
maintained, but I understand if others value the simpler approach that
sacrifices a caching corner-case while still fixing correctness.
  

On Thu, Jun 12, 2014 at 11:37:16AM -0500, Meador Inge wrote:
> The implementation of __get_nprocs uses a stactic variable to cache
> the value of the current number of processors.  The caching breaks when
> 'time (NULL) == 0':
> 
>   $ cat nproc.c
>   #include <stdio.h>
>   #include <time.h>
>   #include <sys/time.h>
> 
>   int main(int argc, char *argv[])
>   {
>     time_t t;
>     struct timeval tv = {0, 0};
>     printf("settimeofday({0, 0}, NULL) = %d\n", settimeofday(&tv, NULL));
>     t = time(NULL);
>     printf("Time: %d, CPUs: %d\n", (unsigned int)t, get_nprocs());
>     return 0;
>   }
>   $ gcc -O3 nproc.c
>   $ ./a.out
>   settimeofday({0, 0}, NULL) = -1
>   Time: 1401311578, CPUs: 4
>   $ sudo ./a.out
>   settimeofday({0, 0}, NULL) = 0
>   Time: 0, CPUs: 0
> 
> The problem is with the condition used to check whether a cached
> value should be returned or not:
> 
>   static int cached_result;
>   static time_t timestamp;
> 
>   time_t now = time (NULL);
>   time_t prev = timestamp;
>   atomic_read_barrier ();
>   if (now == prev)
>     return cached_result;
> 
> When 'timestamp == 0', as in the above test case, an unitialized
> cache value is returned.
> 
> This patch fixes the problem by ensuring that 'cached_result' has
> been set at least once before returning it.
> 
> I checked for regressions with 'make check' and verified that the
> described test case works, but couldn't find a good way to automate
> the test in the GLIBC testsuite.
> 
> OK?
> 
> 2014-06-12  Meador Inge  <meadori@codesourcery.com>
> 
> 	[BZ #16996]
> 	sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Ensure that
> 	the cached result has been set before returning it.
> 
> ---
>  sysdeps/unix/sysv/linux/getsysstats.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/getsysstats.c b/sysdeps/unix/sysv/linux/getsysstats.c
> index b6a6fe3..755c259 100644
> --- a/sysdeps/unix/sysv/linux/getsysstats.c
> +++ b/sysdeps/unix/sysv/linux/getsysstats.c
> @@ -126,13 +126,13 @@ next_line (int fd, char *const buffer, char **cp, char **re,
>  int
>  __get_nprocs (void)
>  {
> -  static int cached_result;
> +  static int cached_result = -1;
>    static time_t timestamp;
>  
>    time_t now = time (NULL);
>    time_t prev = timestamp;
>    atomic_read_barrier ();
> -  if (now == prev)
> +  if (now == prev && cached_result > -1)

Wouldn't (now == prev && now > 0) be nicer?

Siddhesh

>      return cached_result;
>  
>    /* XXX Here will come a test for the new system call.  */
> -- 
> 1.7.9.5
>
  

On 06/12/2014 12:15 PM, Siddhesh Poyarekar wrote:

> Wouldn't (now == prev && now > 0) be nicer?

Hmmm, good point, but is it OK to make assumptions about the underlying
representation of 'time_t'* (e.g. that we get the expected behavior from the > 0
compare)?  The 'now == prev' compare is at least 'time_t'-to-'time_t'.  I
thought about just doing 'cached_result > 0' too, but I suppose 'cached_result'
can actually be 0 in the case of an error.

* The C Standard generally says not to, but maybe we can make that assumption
within the implementation of the C Standard Library.
  

time_t is always an integer type counting seconds since 1970 in glibc.
However, it can be signed and negative values can be valid timestamps (if
you set your computer's clock to 1969, for example).
  

On Thu, Jun 12, 2014 at 01:37:37PM -0700, Roland McGrath wrote:
> time_t is always an integer type counting seconds since 1970 in glibc.
> However, it can be signed and negative values can be valid timestamps (if
> you set your computer's clock to 1969, for example).

Oh yes, so:

    (now == prev && now != 0)

Siddhesh
  

On Wed, Dec 31, 1969 at 06:06:21PM -0600, Meador Inge wrote:
> Therefore two close call to 'get_nprocs' at that time point will
> cause the value to be computed both times.  I prefer the first patch
> where correctness and the caching behavior is maintained, but I
> understand if others value the simpler approach that sacrifices a
> caching corner-case while still fixing correctness.

I'd say correctness trumps here, even if it is an obscure corner case.
I'll push it tomorrow if nobody objects.

Siddhesh