On May 24, 2022 8:08 PM, Taylor Blau wrote: >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. 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
