mismatch of type of ut_tv.tv_sec between glibc-2.41 and utmp(5)

mismatch of type of ut_tv.tv_sec between glibc-2.41 and utmp(5)

[Dr. Thomas Orgis]

Dear man-pages,

while investigating some old bad usage of time(&ut,ut_time) I noticed
that my glibc-2.41 headers define that part of the utmp struct like this:

#if __WORDSIZE_TIME64_COMPAT32
  int32_t ut_session;           /* Session ID, used for windowing.  */
  struct
  {
    __uint32_t tv_sec;          /* Seconds.  */
    int32_t tv_usec;            /* Microseconds.  */
  } ut_tv;                      /* Time entry was made.  */
#else
  long int ut_session;          /* Session ID, used for windowing.  */
  struct timeval ut_tv;         /* Time entry was made.  */
#endif

The man page claims this:

           #if __WORDSIZE == 64 && defined __WORDSIZE_COMPAT32
               int32_t ut_session;           /* Session ID (getsid(2)),
                                                used for windowing */
               struct {
                   int32_t tv_sec;           /* Seconds */
                   int32_t tv_usec;          /* Microseconds */
               } ut_tv;                      /* Time entry was made */
           #else
                long   ut_session;           /* Session ID */
                struct timeval ut_tv;        /* Time entry was made */
           #endif

I don't know the history … did it use to be a signed integer and
someone decided to buy some time by making it unsigned? This is a minor
detail for the bad time() usage, where 32 bit vs. 64 bit time_t might
be more serious. Also the macros being checked for this compatibility
mode differ, but I am not sure how closely the man page want to follow
glibc here.

At least the type of tv_sec should match, I guess.

Now I have to think how elaborately I want to handle possible overflow
from time_t assigning to uint32_t with the recommended way of using
gettimeofday() for utmp …


Regards,

Thomas

-- 
Dr. Thomas Orgis
HPC @ Universität Hamburg

Re: mismatch of type of ut_tv.tv_sec between glibc-2.41 and utmp(5)

[Florian Weimer]

* Thomas Orgis:

> I don't know the history … did it use to be a signed integer and
> someone decided to buy some time by making it unsigned?

Yes, this changed in glibc 2.40.  Some distributions may have
backported it.

commit 5361ad3910c257bc327567be76fde532ed238e42
Author: Florian Weimer <fweimer@redhat.com>
Date:   Fri Apr 19 14:38:17 2024 +0200

    login: Use unsigned 32-bit types for seconds-since-epoch
    
    These fields store timestamps when the system was running.  No Linux
    systems existed before 1970, so these values are unused.  Switching
    to unsigned types allows continued use of the existing struct layouts
    beyond the year 2038.
    
    The intent is to give distributions more time to switch to improved
    interfaces that also avoid locking/data corruption issues.
    
    Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> Now I have to think how elaborately I want to handle possible overflow
> from time_t assigning to uint32_t with the recommended way of using
> gettimeofday() for utmp …

On the other hand, utmp is pretty much obsolete because the design
assumes a fixed number of terminal lines.

Re: mismatch of type of ut_tv.tv_sec between glibc-2.41 and utmp(5)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 4068 bytes --]

Hi Thomas,

On Fri, Feb 28, 2025 at 10:36:10AM +0100, Dr. Thomas Orgis wrote:
> Dear man-pages,
> 
> while investigating some old bad usage of time(&ut,ut_time) I noticed
> that my glibc-2.41 headers define that part of the utmp struct like this:
> 
> #if __WORDSIZE_TIME64_COMPAT32
>   int32_t ut_session;           /* Session ID, used for windowing.  */
>   struct
>   {
>     __uint32_t tv_sec;          /* Seconds.  */
>     int32_t tv_usec;            /* Microseconds.  */
>   } ut_tv;                      /* Time entry was made.  */
> #else
>   long int ut_session;          /* Session ID, used for windowing.  */
>   struct timeval ut_tv;         /* Time entry was made.  */
> #endif
> 
> The man page claims this:
> 
>            #if __WORDSIZE == 64 && defined __WORDSIZE_COMPAT32
>                int32_t ut_session;           /* Session ID (getsid(2)),
>                                                 used for windowing */
>                struct {
>                    int32_t tv_sec;           /* Seconds */
>                    int32_t tv_usec;          /* Microseconds */
>                } ut_tv;                      /* Time entry was made */
>            #else
>                 long   ut_session;           /* Session ID */
>                 struct timeval ut_tv;        /* Time entry was made */
>            #endif

It seems your suspicion was right.  Someone decided to borrow some time,
according to the commit message that changed that code in glibc:

	alx@debian:~/src/gnu/glibc/master$ grep -rn '__uint32_t tv_sec;' -l \
		| xargs -L1 git blame -- \
		| grep '__uint32_t tv_sec;';
	5361ad3910c bits/utmp.h                (Florian Weimer 2024-04-19 14:38:17 +0200  79)     __uint32_t tv_sec;		/* Seconds.  */
	5361ad3910c (Florian Weimer 2024-04-19 14:38:17 +0200  77)     __uint32_t tv_sec;		/* Seconds.  */
	alx@debian:~/src/gnu/glibc/master$ git log -1 5361ad3910c --stat;
	commit 5361ad3910c257bc327567be76fde532ed238e42
	Author: Florian Weimer <fweimer@redhat.com>
	Date:   Fri Apr 19 14:38:17 2024 +0200

	    login: Use unsigned 32-bit types for seconds-since-epoch
	    
	    These fields store timestamps when the system was running.  No Linux
	    systems existed before 1970, so these values are unused.  Switching
	    to unsigned types allows continued use of the existing struct layouts
	    beyond the year 2038.
	    
	    The intent is to give distributions more time to switch to improved
	    interfaces that also avoid locking/data corruption issues.
	    
	    Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

	 NEWS                         |  9 ++++++++-
	 bits/utmp.h                  |  4 ++--
	 login/Makefile               |  4 +++-
	 login/tst-utmp-unsigned-64.c |  1 +
	 login/tst-utmp-unsigned.c    | 40 ++++++++++++++++++++++++++++++++++++++++
	 sysdeps/gnu/bits/utmpx.h     |  2 +-
	 6 files changed, 55 insertions(+), 5 deletions(-)


> 
> I don't know the history … did it use to be a signed integer and
> someone decided to buy some time by making it unsigned? This is a minor
> detail for the bad time() usage, where 32 bit vs. 64 bit time_t might
> be more serious. Also the macros being checked for this compatibility
> mode differ, but I am not sure how closely the man page want to follow
> glibc here.

We should document the change.  If anyone wants to send a patch, I'll
review it.  I won't write it myself, because I'm not an expert in
compatibility code between 32 and 64 bits, so I prefer if someone more
expert makes sure the documentation is correct.

> At least the type of tv_sec should match, I guess.

Yes.  And we should probably document the old type in the HISTORY
section.

> Now I have to think how elaborately I want to handle possible overflow
> from time_t assigning to uint32_t with the recommended way of using
> gettimeofday() for utmp …

If you show some code, we can have a look at it.  :)


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: mismatch of type of ut_tv.tv_sec between glibc-2.41 and utmp(5)

[Dr. Thomas Orgis]

Am Fri, 28 Feb 2025 15:06:33 +0100
schrieb Alejandro Colomar <alx@kernel.org>:

> It seems your suspicion was right.  Someone decided to borrow some time,
> according to the commit message that changed that code in glibc:

> We should document the change.  If anyone wants to send a patch, I'll
> review it.  I won't write it myself, because I'm not an expert in
> compatibility code between 32 and 64 bits, so I prefer if someone more
> expert makes sure the documentation is correct.

Florian? Seems to be the best qualified. As a user, a question for me
is if the specific macros deciding for the 32 bit field should be in
the man page or not, as they seem to be an implementation detail that
one cannot rely on. I guess I need to do a check like sizeof(time_t) >
sizeof(tv_sec)? And then just assume unsigned type? It is a hack, I
understand. So any use will be hacky.

> If you show some code, we can have a look at it.  :)

Not much to show. It is semi-abandoned code in simpleinit-msb that
called time() directly on tv_sec. I switched that to gettimeofday() and
kept the same level of error handling … which is none. My function
detects if the assignment to tv_sec changed the time_t value and
returns an error, but the caller code is inside void functions and
doesn't have an error handling path.

The effect will be broken lastlog entries in a few decades. The
unsigned int extension moves that a bit further back.


Alrighty then,

Thomas

-- 
Dr. Thomas Orgis
HPC @ Universität Hamburg

Re: mismatch of type of ut_tv.tv_sec between glibc-2.41 and utmp(5)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

Hi Thomas,

On Mon, Mar 03, 2025 at 10:31:01AM +0100, Dr. Thomas Orgis wrote:
> Am Fri, 28 Feb 2025 15:06:33 +0100
> schrieb Alejandro Colomar <alx@kernel.org>:
> 
> > It seems your suspicion was right.  Someone decided to borrow some time,
> > according to the commit message that changed that code in glibc:
> 
> > We should document the change.  If anyone wants to send a patch, I'll
> > review it.  I won't write it myself, because I'm not an expert in
> > compatibility code between 32 and 64 bits, so I prefer if someone more
> > expert makes sure the documentation is correct.
> 
> Florian? Seems to be the best qualified. As a user, a question for me
> is if the specific macros deciding for the 32 bit field should be in
> the man page or not, as they seem to be an implementation detail that
> one cannot rely on. I guess I need to do a check like sizeof(time_t) >
> sizeof(tv_sec)? And then just assume unsigned type? It is a hack, I
> understand. So any use will be hacky.

Hmmm, this reminds me of timespec(3type).  Maybe we can document it
similarly here.  Feel free to inspire on that page for a patch (you or
Florian).

> > If you show some code, we can have a look at it.  :)
> 
> Not much to show. It is semi-abandoned code in simpleinit-msb that
> called time() directly on tv_sec. I switched that to gettimeofday() and
> kept the same level of error handling … which is none. My function
> detects if the assignment to tv_sec changed the time_t value and
> returns an error, but the caller code is inside void functions and
> doesn't have an error handling path.
> 
> The effect will be broken lastlog entries in a few decades. The
> unsigned int extension moves that a bit further back.

Thanks for the details!


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]