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

[LibOS] time zone is incorrect in syscall gettimeofday() or glibc localtime() or bash command date #466

Open
llly opened this issue Mar 21, 2022 · 16 comments

Comments

@llly
Copy link
Contributor

llly commented Mar 21, 2022

Description of the problem

Current Gramine doesn't take care of time zone. The requests for time with time zone is incorrect and inconsistent.

Steps to reproduce

  1. Choose time zone using timedatectl set-timezone on Ubuntu.
  2. execute sample
time_t timep;
time (&timep);
printf("%s\n",asctime(localtime(&timep)));

struct timeval tv;
struct timezone tz= {60, 1};
gettimeofday (&tv , &tz);
printf("tz_minuteswest: %d\n", tz.tz_minuteswest);
  1. execute bash -c date in CI-Example.

Expected results

Mon Mar 21 14:39:45 2022
tz_minuteswest: -480
Mon Mar 21 14:40:21 CST 2022

Actual results

Mon Mar 21 06:40:45 2022
tz_minuteswest: 60
Mon Mar 21 06:41:41 UTC 2022

Additional information

We can see that all time become UTC time, while tz_minuteswest doesn't reflect UTC time.

Gramine commit hash

e37f773

@lejunzhu
Copy link
Contributor

If you add this to CI-Example bash manifest.template, the result is correct:

...
loader.env.TZ = ":/etc/localtime"
...
fs.mount.etc.type = "chroot"
fs.mount.etc.path = "/etc"
fs.mount.etc.uri = "file:/etc"
...
sgx.allowed_files = [
...
  "file:/etc/localtime",
]

Similar happens to the C example.

However, tz_minuteswest is always 60. Not sure why.

@lejunzhu
Copy link
Contributor

Looking at shim_do_gettimeofday(), tz is never touched, hence the initial value 60 is always there. Is it because this argument is obsolete or a bug?

@llly
Copy link
Contributor Author

llly commented Mar 22, 2022

loader.env.TZ = ":/etc/localtime"

Yes, The workaround works for localtime and date. Native app doesn't need a TZ env to get time zone, OS doesn't provide TZ env, neither.

@lejunzhu
Copy link
Contributor

loader.env.TZ = ":/etc/localtime"

Yes, The workaround works for localtime and date. Native app doesn't need a TZ env to get time zone, OS doesn't provide TZ env, neither.

TZ env is only needed for the bash -c case. C sample code runs fine without it.

If TZ is not set, when using "bash -c date", it will try to open /etc/localtime, e.g. /usr/local/etc/localtime. This doesn't seem correct...

@lejunzhu
Copy link
Contributor

The root cause is the glibc comes with Gramine has a different TZDEFAULT macro:

$ strings /usr/local/lib/x86_64-linux-gnu/gramine/runtime/glibc/libc.so  | grep "/usr/local/etc"
/usr/local/etc/localtime

This is introduced in glibc Makeconfig:

...
sysconfdir = $(prefix)/etc
...
localtime-file = $(sysconfdir)/localtime

So there are 3 options to get the correct timezone: 1. set TZ environment variable in the manifest, 2. copy /etc/localtime to /etc/localtime 3. change the setting of sysconfdir when building glibc.

@dimakuv
Copy link

dimakuv commented Apr 4, 2022

@lejunzhu @llly Looks like we have two separate issues here:

  1. Time syscalls ignore the struct timezone argument completely, e.g.

    long shim_do_gettimeofday(struct __kernel_timeval* tv, struct __kernel_timezone* tz) {

  2. Gramine's patched Glibc needs special enablement in the manifest file to mimic the exact same behavior as system-wide Glibc, in terms of date-time-timezone (but not in terms of tz_minuteswest).

For the first issue, is this anything critical? From what I see, timezone::tz_minuteswest is brittle and not recommended for use. I doubt any real-world application actually uses it. So I would vote for Gramine to continue ignoring this argument.

For the second issue, I would prefer our patched Glibc to be built in the same way as the system-wide Glibc is built.

  • In other words, I don't like the idea of adding loader.env.TZ + sgx.allowed_files = ["file:/etc/localtime"]. This is magic (especially the TZ envvar), and the users shouldn't know/apply this. Gramine's Glibc should be a complete drop-in replacement for the system-wide Glibc.
  • So I vote for @lejunzhu's proposal number 3.

@lejunzhu Do you know why sysconfdir in the Glibc Makefile has a wrong prefix? And what would be a simple way of fixing this prefix (it looks like this prefix can be simply empty-string, this should work for all our supported distros -- Debian/Ubuntu and CentOS/RHEL/Fedora)?

@woju @pwmarcz @mkow Any comments on the second issue?

@lejunzhu
Copy link
Contributor

lejunzhu commented Apr 4, 2022

@lejunzhu Do you know why sysconfdir in the Glibc Makefile has a wrong prefix? And what would be a simple way of fixing this prefix (it looks like this prefix can be simply empty-string, this should work for all our supported distros -- Debian/Ubuntu and CentOS/RHEL/Fedora)?

This is because glibc's Makeconfig will prepend $prefix to sysconfdir by default:

sysconfdir = $(prefix)/etc

And we set $prefix in compile.sh at

But I don't know how to quickly fix it. Actually, if I set "/home/daniel/local" as Gramine prefix, I will find about 10 places in libc.so pointing to this directory:

$ strings libc.so | grep daniel
/home/daniel/local/lib/x86_64-linux-gnu/gramine/runtime/glibc/locale
/home/daniel/local/lib/x86_64-linux-gnu/gramine/runtime/glibc/locale/locale-archive
/home/daniel/local/share/locale
/home/daniel/local/share/zoneinfo
/home/daniel/local/lib/x86_64-linux-gnu/gramine/runtime/glibc/gconv/gconv-modules.cache
/home/daniel/local/share/locale
/home/daniel/local/share/locale/%L/%N:/home/daniel/local/share/locale/%L/LC_MESSAGES/%N:/home/daniel/local/share/locale/%l/%N:/home/daniel/local/share/locale/%l/LC_MESSAGES/%N:
/home/daniel/local/etc/localtime
/home/daniel/local/libexec/getconf
/home/daniel/local/lib/x86_64-linux-gnu/gramine/runtime/glibc/gcal/share/locale/%L/%N:/home/daniel/local/share/locale/%L/LC_MESSAGES/%N:/home/daniel/local/share/locale/%l/%N:/home/daniel/local/share/locale/%l/LC_MESSAGES/%N:
/home/daniel/local/lib/ld-linux-x86-64.so.2

All these paths look suspicious and probably can't be found when running inside Gramine. Maybe we should specify $install_root instead of $prefix when building and installing glibc?

@dimakuv
Copy link

dimakuv commented Apr 4, 2022

From what I read, prefix=/usr is the correct way to build Glibc that will use system-wide configuration files: https://sourceware.org/glibc/wiki/Testing/Builds#:~:text=Building%20glibc%20without%20installing,-To%20build%20glibc&text=By%20building%20with%20the%20prefix,prefix%20for%20a%20system%20glibc.

@woju @pwmarcz @mkow Summoning you.

@woju
Copy link
Member

woju commented Apr 4, 2022

configure --sysconfdir=/etc should work, if you need to override to to whatever, especially outside prefix. Makeconfig setting is in ifndef, and is a default. There are other overrides, and if we want to use system's paths for timezone database, we could override --datadir= or even --zonedir= (which defaults to $datadir/zoneinfo).

If we really care, other settings should be considered (now or sometime in the future), like --localedir, which is the other part of --datadir=.

@dimakuv In the wiki page you linked, I think they simply forgot about /etc.

But I don't understand why we need to mimic system's glibc 1:1, because we're really an alternative OS and alternative distribution. We should be compatible, yes, but we're not under obligation to copy everything.

Yes, TZ= is an ugly hack, it belongs to user, not distributors, and should be disposed of.

@dimakuv
Copy link

dimakuv commented Apr 4, 2022

configure --sysconfdir=/etc should work

Then I vote for this! Nice and contained.

But I don't understand why we need to mimic system's glibc 1:1, because we're really an alternative OS and alternative distribution. We should be compatible, yes, but we're not under obligation to copy everything.

Well, we debate on this for the last 4-5 years :) Glibc is not a stand-alone library, it depends on a bunch of scattered text files (like this /etc/localtime) and opens them from time to time. So we have two options:

  1. Pack and ship these files together with our patched Glibc, and build Glibc so that it points to these files.
  2. Allow our patched Glibc to use the system-wide files.

We currently use option 2, but simply because it is easier (just tinker with the manifest file). And with this GitHub issue, we see that at least sometimes, our patched Glibc doesn't do option 1 nor option 2 by default, but it defaults to something totally meaningless.

So since we're currently doing option 2, we should do it uniformly.

@pwmarcz
Copy link
Contributor

pwmarcz commented Apr 4, 2022

Actually, why do we configure Glibc --prefix="$PREFIX"? We're compiling glibc to be used inside Gramine, and a host path like /home/daniel/local/ (or whatever you end up installing Gramine on host) does not make any sense there.

As a user, I would expect the glibc in Gramine to use paths like /etc/localtime, without any extra prefixes. Remember, this is the main runtime in our "distribution", not some locally-installed alternative library.

@woju
Copy link
Member

woju commented Apr 4, 2022

Because we can map allowed files 1:1 and nothing should break. We'd have to document a canonical mount that should be expected in compatible manifests. We probably can change the approach if you think that would be better.

@pwmarcz
Copy link
Contributor

pwmarcz commented Apr 4, 2022

Because we can map allowed files 1:1 and nothing should break.

What do you mean? For this to make sense, we would have to mount Gramine install dir under the same path inside Gramine. AFAICT, we never do that, we only mount the Gramine install dir under /lib.

@woju
Copy link
Member

woju commented Apr 4, 2022

If we don't do that and it still works, then I suppose there isn't any reason.

@llly
Copy link
Contributor Author

llly commented Apr 11, 2022

Currently, I can use

fs.mounts = [
{ path = "/usr/local/etc", uri = "file:/etc" },
]

to workaround this issue until prefix of Glibc updated.

@hinkelski
Copy link

hinkelski commented Jul 5, 2023

Sorry, if this offtopic or something... I don't know this project here, but I came across this thread, when troubleshooting a similar issue in my toolchain.
I was able to fix it like this:
echo "localtime-file = /etc/localtime" > $glibc/configparms
Where $glibc is the glibc source folder. And this is run prior to configuring glibc.

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

No branches or pull requests

6 participants