Request: add offset to current system time

I would like to suggest an enhancement: add a user-specified offset, positive or negative, to the current system time and use the sum for Tor's normal processes in place of the system time.

This is to get around the situation where the system time is known to be inaccurate but cannot be changed. For example, in an OpenVZ VPS.

(OpenVZ VPSs, aka containers, inherit the system time of the server hosting the VPS. If the administrator of the OpenVZ host is unable or unwilling to correctly set the time, then the hosted clients just have to live with the inaccuracy.)

Trac:
Username: tmpname0901

changed milestone to %Tor: unspecified

added component::core tor/tor milestone::Tor: unspecified priority::medium reporter::tmpname0901 resolution::wontfix severity::normal status::closed tor-relay type::enhancement labels

I'd take a patch to do this. It's sad that it's needed, though: Every OS deserves an accurate view of UTC.

Trac:
Milestone: N/A to Tor: unspecified

This ticket might actually be a smart intermediate step toward having Tor autodetect its offset and correct for it. It's a nice way to break up the steps, at least.

Replying to tmpname0901:

I would like to suggest an enhancement: add a user-specified offset, positive or negative, to the current system time and use the sum for Tor's normal processes in place of the system time.

This is to get around the situation where the system time is known to be inaccurate but cannot be changed. For example, in an OpenVZ VPS.

The workaround for this (on defective Unixoid VPSes) is to install and use the ‘faketime’ package. It does seem to be something that Tor needs to support directly though (for maliciously misconfigured Windows boxes).

Trac:
Username: tmpname0901

tor-timeoffset.patch

Add offset to system time prior to using time in OR

Trac:
Username: tmpname0901

Hmmm... that "faketime" looks intriguing. Thanks for the heads-up. I'll take a look at it.

In the meantime, attached is the patch to implement my requested enhancement. This patch is against Tor v0.2.3.20.rc and has been running ( fingerprint: 24EB8A109D4C2555B2D61BB6C70041B5C4702066 ) for about 36 hours now. I'm running with new config option TimeOffset option set to -7356 to compensate for my 2:02:36 system time inaccuracy.

The heavy lifting is done in file or.h with this line:

#define CURRTIME (time(NULL) + get_options()->TimeOffset)

Then the many, many occurrences of 'time(NULL)' are replaced by 'CURRTIME'.

I started out replacing all occurrences of 'time(NULL)' then backed out a few of the replacements, the ones encountered early, before the config options are parsed.

Trac:
Username: tmpname0901

Replying to tmpname0901:

In the meantime, attached is the patch to implement my requested enhancement. This patch is against Tor v0.2.3.20.rc and has been running ( fingerprint: 24EB8A109D4C2555B2D61BB6C70041B5C4702066 ) for about 36 hours now. I'm running with new config option TimeOffset option set to -7356 to compensate for my 2:02:36 system time inaccuracy.

The heavy lifting is done in file or.h with this line:

#define CURRTIME (time(NULL) + get_options()->TimeOffset)

Eeek! That's a scary way to do it.

Better to implement a tor_time function in src/common/util.[hc] which adds an offset stored in a global static variable, then set that variable when the options are loaded. You might also need to change other time-related functions in there, too.

(This will also let you use your adjusted time in code in src/common/, not just in src/or/ .)

Trac:
Status: new to needs_revision

Also, you will be much happier if you use Git to prepare your patches (it'll leave out junk patch-bands like the changes to tor.1.in and tor.html.in automatically, and it can automatically provide authorship information too). Come to Tor's IRC channels if you need help with that.

Eeek! That's a scary way to do it.

Not being terribly familiar with the Tor code, going the #define route was the easiest as I knew that or.h was included everywhere. I'll take your advice re src/common/util.[hc] for the next revision of the patch.

For the initial revision I mostly wanted to see that the idea actually worked, an it seems that it does.

Trac:
Username: tmpname0901

Trac:
Username: tmpname0901

tor-timeoffset-rev2.patch

Same functionality, using function instead of #define

Trac:
Username: tmpname0901

Updated patch, relative to the release-0.2.3 branch. I hope it's less scary.

I tried the suggested src/common/util.[hc] but it didn't work due to linking error. (Unresolved get_options() in library.) So instead I put tor_time() in src/or/config.[hc], as the function is kinda config=option-related.

Trac:
Username: tmpname0901

Replying to tmpname0901:

Updated patch, relative to the release-0.2.3 branch. I hope it's less scary.

I tried the suggested src/common/util.[hc] but it didn't work due to linking error. (Unresolved get_options() in library.)

Which is one reason that I told you to use a static global variable which the configuration-loading code would set. (Another reason is that it would suck if your function/macro were ever called before Tor had loaded its configuration (i.e. with get_options() == NULL).)

So instead I put tor_time() in src/or/config.[hc], as the function is kinda config=option-related.

It belongs in src/common/, next to the other functions that return the current time (and which also need to learn to return an adjusted time). (I would expect those functions to live in src/common/util.c, but on further thought, they might be in a different file.)

Replying to rransom:

Which is one reason that I told you to use a static global variable which the configuration-loading code would set. (Another reason is that it would suck if your function/macro were ever called before Tor had loaded its configuration (i.e. with get_options() == NULL).)

In particular:

• Define the global variable somewhere (in the same file as tor_time, before it, preferably near it):

static int time_offset = 0;

• Add a function to set that variable:

void
tor_set_time_offset(int new_offset)
{
  time_offset = new_offset;
  /* XXXX Maybe a cached time needs to be updated here. */
}

• Call tor_set_time_offset when Tor's configuration is changed.
• Use tor_time everywhere, not just after Tor has loaded its configuration once. (Tor will use a time offset of 0 until it is set; after that, Tor will use the same time offset everywhere, not just in code that you didn't see run (and crash) before Tor loaded its configuration.)
• Make sure you have configured Tor with the --enable-gcc-warnings option.
• Run ‘make check-spaces’ (semi-optional).

Hi! I'll second rransom's recommendation to use git and do this as a patch series.

Other stuff:

• This should be based on master, not release-0.2.3; new features go into master.
• I don't think it's right to replace every intstance of time() with an offset time: for example, when we're looking at modification times of files, we need to use the current system time, or else we'll make the wrong decision about whether files are too new/too old.
• We can also get our time from tor_gettimeofday(); that might need an offset version too, if we ever use it for something besides measuring how long something takes.

After a week of testing I think that the use of libfaketime is a better solution than modifying Tor when a system has a fixed inaccuracy in time.

Besides having all the system components on the same page, time-wise, it also keeps Tor's components in sync. In particular, libevent checks the time in several locations. I didn't see any obvious problems when patching Tor and leaving libevent unpatched, but I'm sure I didn't exercise all the code paths needed to touch those unpatched instances.

Trac:
Username: tmpname0901

Trac:
Keywords: N/A deleted, tor-relay added

Trac:
Component: Tor Relay to Tor

The current recommendation really is "use libfaketime".

Trac:
Reviewer: N/A to N/A
Resolution: N/A to wontfix
Status: needs_revision to closed
Severity: N/A to Normal
Sponsor: N/A to N/A

closed

moved to tpo/core/tor#6683 (closed)