[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Peter Seiderer]

Use ac_cv_func_working_mktime=yes to force the use of a provided
mktime implementation instead of compiling the failing own one.

Fixes:

  http://autobuild.buildroot.net/results/5bcd8f4235002da682cc900f866116d2fe87f1c8

  mktime.c: In function 'ydhms_diff':
  mktime.c:106:52: error: size of array 'a' is negative
   #define verify(name, assertion) struct name { char a[(assertion) ? 1 : -1]; }
                                                      ^
  mktime.c:170:3: note: in expansion of macro 'verify'
     verify (long_int_year_and_yday_are_wide_enough,
     ^~~~~~

with the failure/assert comming from the lines:

  verify (long_int_year_and_yday_are_wide_enough,
          INT_MAX <= LONG_MAX / 2 || TIME_T_MAX <= UINT_MAX);

which fails since the y2038 time_t conversion from 32bit to 64bit
(musl libc).

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
---
Changes v1 -> v2:
  - enhance commit log (as suggested by Thomas Petazzoni)
---
 package/cvs/cvs.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/package/cvs/cvs.mk b/package/cvs/cvs.mk
index 563802cc9d..6f83ca6408 100644
--- a/package/cvs/cvs.mk
+++ b/package/cvs/cvs.mk
@@ -12,7 +12,9 @@ CVS_LICENSE = GPL-1.0+, LGPL-2.0+, LGPL-2.1+ (glob)
 CVS_LICENSE_FILES = COPYING COPYING.LIB lib/glob-libc.h
 CVS_DEPENDENCIES = ncurses
 
-CVS_CONF_ENV = cvs_cv_func_printf_ptr=yes
+CVS_CONF_ENV = \
+	ac_cv_func_working_mktime=yes \
+	cvs_cv_func_printf_ptr=yes
 
 CVS_CONFIGURE_ARGS = --disable-old-info-format-support
 ifeq ($(BR2_PACKAGE_CVS_SERVER),y)
-- 
2.26.2

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Thomas Petazzoni]

On Thu, 28 May 2020 23:59:12 +0200
Peter Seiderer <ps.report@gmx.net> wrote:

> Use ac_cv_func_working_mktime=yes to force the use of a provided
> mktime implementation instead of compiling the failing own one.
> 
> Fixes:
> 
>   http://autobuild.buildroot.net/results/5bcd8f4235002da682cc900f866116d2fe87f1c8
> 
>   mktime.c: In function 'ydhms_diff':
>   mktime.c:106:52: error: size of array 'a' is negative
>    #define verify(name, assertion) struct name { char a[(assertion) ? 1 : -1]; }
>                                                       ^
>   mktime.c:170:3: note: in expansion of macro 'verify'
>      verify (long_int_year_and_yday_are_wide_enough,
>      ^~~~~~
> 
> with the failure/assert comming from the lines:
> 
>   verify (long_int_year_and_yday_are_wide_enough,
>           INT_MAX <= LONG_MAX / 2 || TIME_T_MAX <= UINT_MAX);

Wait wait, this is verifying that TIME_T_MAX is lower than UINT_MAX, so
I guess the rest of the code assumes that the size of time_t is less or
equal than the size of an unsigned int.

With the time_t conversion to 64bit, time_t *is* larger than an
unsigned int.

So aren't you papering over the problem, and in fact potentially
causing some issues in CVS, which seems to assume the time_t fits in
32-bit ?

Or maybe I'm wrong here, because I see AlpineLinux simply updates the
mktime configure test to fix this issue:

  https://git.alpinelinux.org/aports/tree/main/cvs/mktime-configure.patch

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Peter Seiderer]

Hello Thomas,

On Fri, 29 May 2020 08:09:48 +0200, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Thu, 28 May 2020 23:59:12 +0200
> Peter Seiderer <ps.report@gmx.net> wrote:
>
> > Use ac_cv_func_working_mktime=yes to force the use of a provided
> > mktime implementation instead of compiling the failing own one.
> >
> > Fixes:
> >
> >   http://autobuild.buildroot.net/results/5bcd8f4235002da682cc900f866116d2fe87f1c8
> >
> >   mktime.c: In function 'ydhms_diff':
> >   mktime.c:106:52: error: size of array 'a' is negative
> >    #define verify(name, assertion) struct name { char a[(assertion) ? 1 : -1]; }
> >                                                       ^
> >   mktime.c:170:3: note: in expansion of macro 'verify'
> >      verify (long_int_year_and_yday_are_wide_enough,
> >      ^~~~~~
> >
> > with the failure/assert comming from the lines:
> >
> >   verify (long_int_year_and_yday_are_wide_enough,
> >           INT_MAX <= LONG_MAX / 2 || TIME_T_MAX <= UINT_MAX);
>
> Wait wait, this is verifying that TIME_T_MAX is lower than UINT_MAX, so
> I guess the rest of the code assumes that the size of time_t is less or
> equal than the size of an unsigned int.
>
> With the time_t conversion to 64bit, time_t *is* larger than an
> unsigned int.
>
> So aren't you papering over the problem, and in fact potentially
> causing some issues in CVS, which seems to assume the time_t fits in
> 32-bit ?

....it is only the internal assumption of the local mktime implementation,
as far as I see all cvs callers of mktime use time_t to store the return
value....

Regards,
Peter

>
> Or maybe I'm wrong here, because I see AlpineLinux simply updates the
> mktime configure test to fix this issue:
>
>   https://git.alpinelinux.org/aports/tree/main/cvs/mktime-configure.patch
>
> Thomas

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Thomas Petazzoni]

On Fri, 29 May 2020 20:43:43 +0200
Peter Seiderer <ps.report@gmx.net> wrote:

> > With the time_t conversion to 64bit, time_t *is* larger than an
> > unsigned int.
> >
> > So aren't you papering over the problem, and in fact potentially
> > causing some issues in CVS, which seems to assume the time_t fits in
> > 32-bit ?  
> 
> ....it is only the internal assumption of the local mktime implementation,
> as far as I see all cvs callers of mktime use time_t to store the return
> value....

Hm, ok. But does it make sense to use the AlpineLinux patch instead ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Peter Seiderer]

Hello Thomas,

On Fri, 29 May 2020 21:07:05 +0200, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Fri, 29 May 2020 20:43:43 +0200
> Peter Seiderer <ps.report@gmx.net> wrote:
>
> > > With the time_t conversion to 64bit, time_t *is* larger than an
> > > unsigned int.
> > >
> > > So aren't you papering over the problem, and in fact potentially
> > > causing some issues in CVS, which seems to assume the time_t fits in
> > > 32-bit ?
> >
> > ....it is only the internal assumption of the local mktime implementation,
> > as far as I see all cvs callers of mktime use time_t to store the return
> > value....
>
> Hm, ok. But does it make sense to use the AlpineLinux patch instead ?

Did a short test (as the alpine linux patch patches configure directly):

	$ wget https://git.alpinelinux.org/aports/plain/main/cvs/mktime-configure.patch
	$ make cvs-patch
	$ cd build/cvs-1.12.13/
	$ patch -p 1 < mktime-configure.patch
	$ cd ../..
	$ make cvs
...still fails (as expected as the mktime check is a runtime check (?) which
is unfixable for cross-compile)...

Regards,
Peter

>
> Thomas

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Thomas Petazzoni]

On Thu, 28 May 2020 23:59:12 +0200
Peter Seiderer <ps.report@gmx.net> wrote:

> Use ac_cv_func_working_mktime=yes to force the use of a provided
> mktime implementation instead of compiling the failing own one.
> 
> Fixes:
> 
>   http://autobuild.buildroot.net/results/5bcd8f4235002da682cc900f866116d2fe87f1c8
> 
>   mktime.c: In function 'ydhms_diff':
>   mktime.c:106:52: error: size of array 'a' is negative
>    #define verify(name, assertion) struct name { char a[(assertion) ? 1 : -1]; }
>                                                       ^
>   mktime.c:170:3: note: in expansion of macro 'verify'
>      verify (long_int_year_and_yday_are_wide_enough,
>      ^~~~~~
> 
> with the failure/assert comming from the lines:
> 
>   verify (long_int_year_and_yday_are_wide_enough,
>           INT_MAX <= LONG_MAX / 2 || TIME_T_MAX <= UINT_MAX);
> 
> which fails since the y2038 time_t conversion from 32bit to 64bit
> (musl libc).
> 
> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> ---
> Changes v1 -> v2:
>   - enhance commit log (as suggested by Thomas Petazzoni)
> ---
>  package/cvs/cvs.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2] package/cvs: fix mktime related compile failure

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > On Thu, 28 May 2020 23:59:12 +0200
 > Peter Seiderer <ps.report@gmx.net> wrote:

 >> Use ac_cv_func_working_mktime=yes to force the use of a provided
 >> mktime implementation instead of compiling the failing own one.
 >> 
 >> Fixes:
 >> 
 >> http://autobuild.buildroot.net/results/5bcd8f4235002da682cc900f866116d2fe87f1c8
 >> 
 >> mktime.c: In function 'ydhms_diff':
 >> mktime.c:106:52: error: size of array 'a' is negative
 >> #define verify(name, assertion) struct name { char a[(assertion) ? 1 : -1]; }
 >> ^
 >> mktime.c:170:3: note: in expansion of macro 'verify'
 >> verify (long_int_year_and_yday_are_wide_enough,
 >> ^~~~~~
 >> 
 >> with the failure/assert comming from the lines:
 >> 
 >> verify (long_int_year_and_yday_are_wide_enough,
 >> INT_MAX <= LONG_MAX / 2 || TIME_T_MAX <= UINT_MAX);
 >> 
 >> which fails since the y2038 time_t conversion from 32bit to 64bit
 >> (musl libc).
 >> 
 >> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
 >> ---
 >> Changes v1 -> v2:
 >> - enhance commit log (as suggested by Thomas Petazzoni)
 >> ---
 >> package/cvs/cvs.mk | 4 +++-
 >> 1 file changed, 3 insertions(+), 1 deletion(-)

Committed to 2020.02.x and 2020.05.x, thanks.

-- 
Bye, Peter Korsgaard