[RFC,51/52] Y2038: add RPC functions

Message ID 20170908174909.28192-2-albert.aribaud@3adev.fr
State New
Headers show
Series
  • Untitled series #2264
Related show

Commit Message

Albert ARIBAUD Sept. 8, 2017, 5:49 p.m.
Three functions in RPC have a struct timeval in their arguments:
pmap_rmtcall, clntudp_create, and clntudp_bufcreate.

Since these struct timeval arguments contain relative timeouts, and
since RPC timeouts can reasonably be expected to be small enough to
still fit in 32-bit representations, the implementations of these
functions just verify that the 64-bit timeout they received can fit
in 32 bits, convert it to 32 bit, and pass it to their 32-bit-time
counterparts.

Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
---
 sunrpc/clnt_udp.c      | 27 +++++++++++++++++++++++++++
 sunrpc/pmap_rmt.c      | 23 +++++++++++++++++++++++
 sunrpc/rpc/clnt.h      | 24 ++++++++++++++++++++++++
 sunrpc/rpc/pmap_clnt.h | 15 +++++++++++++++
 4 files changed, 89 insertions(+)

Comments

Joseph Myers Sept. 8, 2017, 7:46 p.m. | #1
Since the RPC functions are already obsoleted (compat symbols unless you 
use --enable-obsolete-rpc), I think there is a good case for not providing 
_TIME_BITS=64 variants of them (#error in the relevant headers if used 
with _TIME_BITS=64), leaving it to TIRPC to provide such versions.
Paul Eggert Sept. 8, 2017, 7:59 p.m. | #2
On 09/08/2017 10:49 AM, Albert ARIBAUD (3ADEV) wrote:
> +CLIENT *
> +__clntudp_create64 (struct sockaddr_in *raddr, u_long program, u_long version,
> +		    struct __timeval64 wait, int *sockp)
> +{
> +  struct timeval wait32;
> +  if (wait.tv_sec > INT_MAX)
> +  {
> +    return NULL;
> +  }
> +  return clntudp_create (raddr, program, version, wait32, sockp);
> +}

I'm not seeing how this could work. The code does not copy the value of 
'wait' to 'wait32'. And the code doesn't have a proper check for fitting 
in 'int', e.g., it will do the wrong thing for INT_MIN - 1L. And there's 
no error status set when the time is out of range.

I haven't reviewed the patches carefully, just caught this in a spot 
check. Please look systematically for similar errors.

While you're doing that systematic review, I suggest putting something 
like the following code into a suitable private include file, and using 
it to tell whether a __time64_t value is in time_t range. This will 
generate zero instructions when time_t is 64-bit, so generic callers can 
use this function without needing any ifdefs and without losing any 
performance on 64-bit time_t platforms. You should write similar static 
functions for checking whether struct __timeval64 is in struct timeval 
range, and similarly for struct __timespec64. These can check for the 
subseconds parts being in range, as needed (watch for x32 here). The 
idea is to be systematic about this stuff and to do it in one place, to 
avoid ticky-tack range bugs such as are in the above-quoted code.

   /* time_t is always 'long int' in the GNU C Library.  */
   #define TIME_T_MIN LONG_MIN
   #define TIME_T_MAX LONG_MAX

   static inline bool
   fits_in_time_t (__time64_t t)
   {
   #if 7 <= __GNUC__
     return !__builtin_add_overflow_p (t, 0, TIME_T_MAX);
   #endif
     return TIME_T_MIN <= t && t <= TIME_T_MAX;
   }
Albert ARIBAUD Sept. 9, 2017, 2:37 p.m. | #3
Hi Paul,

On Fri, 8 Sep 2017 12:59:27 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 09/08/2017 10:49 AM, Albert ARIBAUD (3ADEV) wrote:
> > +CLIENT *
> > +__clntudp_create64 (struct sockaddr_in *raddr, u_long program,
> > u_long version,
> > +		    struct __timeval64 wait, int *sockp)
> > +{
> > +  struct timeval wait32;
> > +  if (wait.tv_sec > INT_MAX)
> > +  {
> > +    return NULL;
> > +  }
> > +  return clntudp_create (raddr, program, version, wait32, sockp);
> > +}  
> 
> I'm not seeing how this could work. The code does not copy the value
> of 'wait' to 'wait32'. And the code doesn't have a proper check for
> fitting in 'int', e.g., it will do the wrong thing for INT_MIN - 1L.
> And there's no error status set when the time is out of range.

That's a cut/paste typo during a recent cleanup round :( --
__clntudp_bufcreate64 is missing the same two lines which are present
in __pmap_rmtcall_t64 and copy the timeout into its 32-bit counterpart.
Thanks for spotting this, will add the missing lines (or possibly a
call to a conversion function, similar to your suggestion below).

> I haven't reviewed the patches carefully, just caught this in a spot 
> check. Please look systematically for similar errors.

Will do.

> While you're doing that systematic review, I suggest putting something 
> like the following code into a suitable private include file, and using 
> it to tell whether a __time64_t value is in time_t range. This will 
> generate zero instructions when time_t is 64-bit, so generic callers can 
> use this function without needing any ifdefs and without losing any 
> performance on 64-bit time_t platforms. You should write similar static 
> functions for checking whether struct __timeval64 is in struct timeval 
> range, and similarly for struct __timespec64. These can check for the 
> subseconds parts being in range, as needed (watch for x32 here). The 
> idea is to be systematic about this stuff and to do it in one place, to 
> avoid ticky-tack range bugs such as are in the above-quoted code.
> 
>    /* time_t is always 'long int' in the GNU C Library.  */
>    #define TIME_T_MIN LONG_MIN
>    #define TIME_T_MAX LONG_MAX

BTW, time_t is a signed int, but its negative range is not completely
usable since (time_t)-1 is used as an error status. So should the
TIME_T_MIN limit be LONG_MIN or should it be 0?

>    static inline bool
>    fits_in_time_t (__time64_t t)
>    {
>    #if 7 <= __GNUC__
>      return !__builtin_add_overflow_p (t, 0, TIME_T_MAX);
>    #endif
>      return TIME_T_MIN <= t && t <= TIME_T_MAX;
>    }

Will do -- in fact, as I hinted above, I'll probably augment this with
timeval and timespec test and conversion functions to automate things
such as setting errno to EOVERFLOW if the conversion cannot be done.

Thanks for your comment and suggestion!

Cordialement,
Albert ARIBAUD
3ADEV
Paul Eggert Sept. 11, 2017, 6:33 a.m. | #4
Albert ARIBAUD wrote:
> time_t is a signed int, but its negative range is not completely
> usable since (time_t)-1 is used as an error status. So should the
> TIME_T_MIN limit be LONG_MIN or should it be 0?

Definitely LONG_MIN. Functions like localtime accept -1 as valid input meaning 
1969-12-31 23:59:59 UTC.

Even for some functions like mktime that return -1 as an error indicator, -1 can 
also be a valid (non-error) return value. I help maintain application code that 
can tell the difference between the two seemingly-identical -1s that mktime 
returns, so that the time_t range is completely usable there too. Admittedly 
it's not an ideal API.
Paul Eggert Sept. 11, 2017, 7:05 a.m. | #5
Paul Eggert wrote:
> 
>    /* time_t is always 'long int' in the GNU C Library.  */
>    #define TIME_T_MIN LONG_MIN
>    #define TIME_T_MAX LONG_MAX
> 
>    static inline bool
>    fits_in_time_t (__time64_t t)
>    {
>    #if 7 <= __GNUC__
>      return !__builtin_add_overflow_p (t, 0, TIME_T_MAX);
>    #endif
>      return TIME_T_MIN <= t && t <= TIME_T_MAX;
>    }

Marc Glisse suggested a simpler solution:

   static inline bool
   fits_in_time_t (__time64_t t)
   {
     return t == (time64_t) t;
   }

This is documented to work for GCC, something I had forgotten, and it generates 
better code for pre-7 GCC. See:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82170#c2
Paul Eggert Sept. 11, 2017, 2:07 p.m. | #6
Paul Eggert wrote:
>    static inline bool
>    fits_in_time_t (__time64_t t)
>    {
>      return t == (time64_t) t;
>    }

Ooops, that last statement should be "return t == (time_t) t;" of course. Sorry.
Albert ARIBAUD Sept. 11, 2017, 3:59 p.m. | #7
Hi Paul,

On Mon, 11 Sep 2017 07:07:29 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> Paul Eggert wrote:
> >    static inline bool
> >    fits_in_time_t (__time64_t t)
> >    {
> >      return t == (time64_t) t;
> >    }  
> 
> Ooops, that last statement should be "return t == (time_t) t;" of course. Sorry.

Noted -- and thanks to you and Marc for the simpler formulation.

Cordialement,
Albert ARIBAUD
3ADEV

Patch

diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c
index df21e28f64..b02b80e2c4 100644
--- a/sunrpc/clnt_udp.c
+++ b/sunrpc/clnt_udp.c
@@ -644,3 +644,30 @@  clntudp_destroy (CLIENT *cl)
   mem_free ((caddr_t) cu, (sizeof (*cu) + cu->cu_sendsz + cu->cu_recvsz));
   mem_free ((caddr_t) cl, sizeof (CLIENT));
 }
+
+/* 64-bit time versions */
+
+CLIENT *
+__clntudp_create64 (struct sockaddr_in *raddr, u_long program, u_long version,
+		    struct __timeval64 wait, int *sockp)
+{
+  struct timeval wait32;
+  if (wait.tv_sec > INT_MAX)
+  {
+    return NULL;
+  }  
+  return clntudp_create (raddr, program, version, wait32, sockp);
+}
+
+CLIENT *
+__clntudp_bufcreate64 (struct sockaddr_in *raddr, u_long program, u_long version,
+		       struct __timeval64 wait, int *sockp, u_int sendsz,
+		       u_int recvsz)
+{
+  struct timeval wait32;
+  if (wait.tv_sec > INT_MAX)
+  {
+    return NULL;
+  }  
+  return clntudp_bufcreate (raddr, program, version, wait32, sockp, sendsz, recvsz);
+}
diff --git a/sunrpc/pmap_rmt.c b/sunrpc/pmap_rmt.c
index 6d4ed7206c..6dd360edcd 100644
--- a/sunrpc/pmap_rmt.c
+++ b/sunrpc/pmap_rmt.c
@@ -391,3 +391,26 @@  done_broad:
   return stat;
 }
 libc_hidden_nolink_sunrpc (clnt_broadcast, GLIBC_2_0)
+
+/* 64-bit-time version */
+
+/* The 64-bit-time version of pmap_rmtcall.
+ * Only handles 64-bit-time timeouts smaller than 2^^31 seconds.
+ */
+enum clnt_stat
+__pmap_rmtcall_t64 (struct sockaddr_in *addr, u_long prog, u_long vers,
+		    u_long proc, xdrproc_t xdrargs, caddr_t argsp,
+		    xdrproc_t xdrres, caddr_t resp,
+		    struct __timeval64 tout, u_long *port_ptr)
+{
+  struct timeval tout32;
+  if (tout.tv_sec > INT_MAX)
+  {
+    return RPC_SYSTEMERROR;
+  }
+  tout32.tv_sec = tout.tv_sec;
+  tout32.tv_usec = tout.tv_usec;
+  
+  return pmap_rmtcall (addr, prog, vers, proc, xdrargs, argsp, xdrres,
+                       resp, tout32, port_ptr);
+}
diff --git a/sunrpc/rpc/clnt.h b/sunrpc/rpc/clnt.h
index f4d4a941c7..e559242eeb 100644
--- a/sunrpc/rpc/clnt.h
+++ b/sunrpc/rpc/clnt.h
@@ -329,9 +329,33 @@  extern CLIENT *clnttcp_create (struct sockaddr_in *__raddr, u_long __prog,
  *	u_int sendsz;
  *	u_int recvsz;
  */
+#ifdef __USE_TIME_BITS64
+# if defined(__REDIRECT)
+extern CLIENT * __REDIRECT (clntudp_create,(struct sockaddr_in *__raddr,
+                                            u_long __program,
+					    u_long __version,
+					    struct timeval __wait_resend,
+					    int *__sockp),
+                            __clntudp_create_t64) __THROW;
+# else
+# define clntudp_create __clntudp_create_t64
+# endif
+#endif
 extern CLIENT *clntudp_create (struct sockaddr_in *__raddr, u_long __program,
 			       u_long __version, struct timeval __wait_resend,
 			       int *__sockp) __THROW;
+#ifdef __USE_TIME_BITS64
+# if defined(__REDIRECT)
+extern CLIENT * __REDIRECT (clntudp_bufcreate,(struct sockaddr_in *__raddr,
+				               u_long __program, u_long __version,
+				               struct __timeval64 __wait_resend,
+					       int *__sockp, u_int __sendsz,
+					       u_int __recvsz),
+                            __clntudp_bufcreate_t64) __THROW;
+# else
+# define clntudp_bufcreate __clntudp_bufcreate_t64
+# endif
+#endif
 extern CLIENT *clntudp_bufcreate (struct sockaddr_in *__raddr,
 				  u_long __program, u_long __version,
 				  struct timeval __wait_resend, int *__sockp,
diff --git a/sunrpc/rpc/pmap_clnt.h b/sunrpc/rpc/pmap_clnt.h
index 1cc94b8fee..70ec89b723 100644
--- a/sunrpc/rpc/pmap_clnt.h
+++ b/sunrpc/rpc/pmap_clnt.h
@@ -71,6 +71,21 @@  extern bool_t pmap_set (const u_long __program, const u_long __vers,
 extern bool_t pmap_unset (const u_long __program, const u_long __vers)
      __THROW;
 extern struct pmaplist *pmap_getmaps (struct sockaddr_in *__address) __THROW;
+#ifdef __USE_TIME_BITS64
+# if defined(__REDIRECT)
+extern enum clnt_stat __REDIRECT (pmap_rmtcall, (struct sockaddr_in *__addr,
+                                                 const u_long __prog,
+                                                 const u_long __vers,
+                                                 const u_long __proc,
+                                                 xdrproc_t __xdrargs,
+                                                 caddr_t __argsp, xdrproc_t __xdrres,
+                                                 caddr_t __resp, struct timeval __tout,
+                                                 u_long *__port_ptr),
+                                  __pmap_rmtcall_t64) __THROW;
+# else
+# define pmap_rmtcall __pmap_rmtcall_t64
+# endif
+#endif
 extern enum clnt_stat pmap_rmtcall (struct sockaddr_in *__addr,
 				    const u_long __prog,
 				    const u_long __vers,