Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make tests pass in 2038 #927

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

bmwiedemann
Copy link
Contributor

see individual commit messages for details

Background:
As part of my work on reproducible builds for openSUSE, I check that software still gives identical build results in the future.
The usual offset is +16 years, because that is how long I expect some software will be used in some places.
This showed up failing tests in our package build.

btw: there might be more work needed, but at least tests passed without this

+++ b/proto_bin.c
         time_t exptime = ntohl(t->message.body.expiration);
+        if (exptime < 0) {
+            exptime += 0x100000000;
+        }

@dormando
Copy link
Member

hah brilliant! I was probably putting this off until at least 2030.

I was suspecting this to break in more places, so I'll have to double check the reltime stuff. The expiration time in actual items is 32bit (and I can't expand that to 64bit), but since it's relative it means you can set exptimes like 60 years in the future which should be fine...

looking at it quickly, in memcached.c:realtime() it looks like you can cause overflow/underflow now by setting exptime to be a date 100+ years into the future. return (rel_time_t)(exptime - process_started) - if you're not using a date based format it looks fine since current_time + exptime (< REALTIME_MAXDELTA) won't ever be that high.

not clear if that's a bug yet though....

@bmwiedemann
Copy link
Contributor Author

looking at it quickly, in memcached.c:realtime() it looks like you can cause overflow/underflow now by setting exptime to be a date 100+ years into the future.

should we add clamping code there to ensure that setting an expiry 100 years in the future just sets its to the max value 0x7fffffff (68y)?

@dormando
Copy link
Member

I think so yeah. need to be careful since the realtime() function's in a couple hot paths.

@bmwiedemann
Copy link
Contributor Author

bmwiedemann commented Aug 30, 2022

https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html could help. Since 30+ years, x86 CPUs have features to report if overflow happened with little performance impact.

@dormando
Copy link
Member

I've been careful of these types of extensions because of the various risc ports. I won't have time to do some in-depth research for a couple weeks but if you or someone wants to propose something portable that'd be super

@bmwiedemann
Copy link
Contributor Author

I added a 3rd commit with an overflow check. The compiler (gcc+clang) should ensure __builtin_sub_overflow to be portable across architectures - it might just use some extra CPU cycles.

However, I did not really understand the meaning of REALTIME_MAXDELTA and

return (rel_time_t)(exptime + current_time) makes no sense to me, unless one of these variables is negative.

@bmwiedemann
Copy link
Contributor Author

If you could add some tests for expiry in +100y, -100y and such, I could run them with these patches.

@dormando
Copy link
Member

I'll try to explain some of the idioms here:

  • The expiration time is stored as a 32bit value on each item in cache. This isn't expandable to 64bits because we're extremely sensitive to memory overhead.
  • current_time is set to be 0-ish with when the server starts. Once per second it's set to "now - process_started"
  • The 32bit expiration times on items are compared against current_time; so if you start a server, then set an item with a TTL of 30 seconds, the expiration time will be set to "30 + current_time(0ish)"
  • REALTIME_MAXDELTA is a likely infrequently used trick where expiration times > 30 days are treated as dates, and the TTL is re-interpreted to be current_time + however much time to hit that date.

This essentially gives us a sliding window for expirations instead of a strict 2038 bug; though I never specifically tested 2038. It also means + or - 100 years will never work as a test, as the internal TTL stays 32bit.

It also means the time limit is essentially "the total uptime of the system + how far into the future you're setting TTL's".

For that we parse exptime as 64-bit int
to avoid errors in t/expirations.t and t/flush-all.t
that had potential for problems with new 64-bit timestamps
@bmwiedemann
Copy link
Contributor Author

I rebased the patch to make it easier to read now that #934 is merged.

@dormando
Copy link
Member

dormando commented Apr 6, 2023

Thanks for that! sorry this is taking so long to validate and merge. My users are all highly conservative so I need to make sure I have dedicated enough time to validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants