Subject: RE: [PATCH v5 02/17] pack-mtimes: support reading .mtimes files

From: <[email-masked]>
Date: Tue, 24 May 2022 20:20:42 -0400

On May 24, 2022 8:08 PM, Taylor Blau wrote:
>On Tue, May 24, 2022 at 07:24:14PM -0400, [email-masked] wrote:
>> On May 24, 2022 6:25 PM ,Taylor Blau write:
>> >On Tue, May 24, 2022 at 03:44:00PM -0400, [email-masked] wrote:
>> >> I am again concerned about 32-bit time_t assumptions. time_t is
>> >> 32-bit on some platforms, signed/unsigned, and sometimes 64-bit. We
>> >> are talking about potentially long-persistent files, as I
>> >> understand this series, so we should not be limiting times to end
>> >> at 2038. That's only
>> >> 16 years off and I would wager that many clones that exist today will exist
>then.
>> >
>> >Note that we're using unsigned fields here, so we have until 2106
>> >(see my earlier response on this in
>https://lore.kernel.org/git/[email-masked]/).
>>
>> I appreciate that, but 32-bit time_t is still signed on many
>> platforms, so when cast, it still might, at some point in another
>> series, cause issues. Please be cautious. I expect that this is the
>> particular hill on which I will die. 😉
>> --Randall
>
>Yes, definitely. There is only one spot that we turn the result of
>nth_packed_mtime() into a time_t, and that's in
>add_object_in_unpacked_pack(). The code there is something like:
>
>    time_t mtime;
>    if (pack->is_cruft)
>      mtime = nth_packed_mtime(pack, object_pos);
>    else
>      mtime = pack->mtime;
>
>    ...
>
>    add_cruft_object_entry(oid, ..., mtime);
>
>...and the reason mtime is a time_t is because that's the type of
>pack->mtime.
>
>And we quickly convert that back to a uint32_t in add_cruft_object_entry(). If
>time_t is signed, then we'll truncate any values beyond 2106, and pre-epoch
>values will become large positive values. That means our error is one-sided in the
>favorable direction, i.e., that we'll keep objects around for longer instead of
>pruning something that we shouldn't have.

I can only hope. I am working with the platform compiler team on time_t issues. Hoping we can get to 64-bit builds within two years, but out of my control. That would make time_t an int64_t, which puts failure well outside my own lifespan and anyone else in my company. Provisioning for signed 64-bit time values would be prudent even if unsupported in a specific build. We are almost 1/4 through 20xx.
--Randall