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

From: Taylor Blau <[email-masked]>
Date: Tue, 24 May 2022 20:07:55 -0400

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.

Thanks,
Taylor