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

From: Taylor Blau <me@ttaylorr.com>
Date: May 25, 2022, 00:07 UTC
Message-ID: <Yo1zW7ntTuNakpOD@nand.local>
In-Reply-To: <016e01d86fc5$64ecf180$2ec6d480$@nexbridge.com>

On Tue, May 24, 2022 at 07:24:14PM -0400, rsbecker@nexbridge.com wrote:

Show 16 quoted lines

> On May 24, 2022 6:25 PM ,Taylor Blau write:
> >On Tue, May 24, 2022 at 03:44:00PM -0400, rsbecker@nexbridge.com 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/YdiXecK6fAKl8++G@nand.local/).
>
> 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