From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33755 invoked by alias); 9 Sep 2017 14:37:37 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 32701 invoked by uid 89); 9 Sep 2017 14:37:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=spotting X-HELO: smtp6-g21.free.fr Date: Sat, 09 Sep 2017 14:37:00 -0000 From: Albert ARIBAUD To: Paul Eggert Cc: libc-alpha@sourceware.org Subject: Re: [RFC PATCH 51/52] Y2038: add RPC functions Message-ID: <20170909163728.05fa95ee.albert.aribaud@3adev.fr> In-Reply-To: References: <20170907224219.12483-50-albert.aribaud@3adev.fr> <20170908174909.28192-1-albert.aribaud@3adev.fr> <20170908174909.28192-2-albert.aribaud@3adev.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2017-09/txt/msg00411.txt.bz2 Hi Paul, On Fri, 8 Sep 2017 12:59:27 -0700, Paul Eggert 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); > > +}=20=20 >=20 > 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=20 > check. Please look systematically for similar errors. Will do. > While you're doing that systematic review, I suggest putting something=20 > like the following code into a suitable private include file, and using=20 > it to tell whether a __time64_t value is in time_t range. This will=20 > generate zero instructions when time_t is 64-bit, so generic callers can= =20 > use this function without needing any ifdefs and without losing any=20 > performance on 64-bit time_t platforms. You should write similar static=20 > functions for checking whether struct __timeval64 is in struct timeval=20 > range, and similarly for struct __timespec64. These can check for the=20 > subseconds parts being in range, as needed (watch for x32 here). The=20 > idea is to be systematic about this stuff and to do it in one place, to=20 > avoid ticky-tack range bugs such as are in the above-quoted code. >=20 > =C2=A0 /* time_t is always 'long int' in the GNU C Library.=C2=A0 */ > =C2=A0 #define TIME_T_MIN LONG_MIN > =C2=A0 #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? > =C2=A0 static inline bool > =C2=A0 fits_in_time_t (__time64_t t) > =C2=A0 { > =C2=A0 #if 7 <=3D __GNUC__ > =C2=A0=C2=A0=C2=A0 return !__builtin_add_overflow_p (t, 0, TIME_T_MAX); > =C2=A0 #endif > =C2=A0=C2=A0=C2=A0 return TIME_T_MIN <=3D t && t <=3D TIME_T_MAX; > =C2=A0 } 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