On Tue, May 24, 2022 at 07:24:14PM -0400, rsbecker@xxxxxxxxxxxxx wrote: > On May 24, 2022 6:25 PM ,Taylor Blau write: > >On Tue, May 24, 2022 at 03:44:00PM -0400, rsbecker@xxxxxxxxxxxxx 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
