On Wed, Jul 1, 2026 at 10:04 AM Jonathan Wakely <[email protected]> wrote:
> > > On Wed, 1 Jul 2026, 08:51 Tomasz Kaminski, <[email protected]> wrote: > >> >> >> On Wed, Jul 1, 2026 at 9:30 AM Jonathan Wakely <[email protected]> >> wrote: >> >>> >>> >>> On Wed, 1 Jul 2026, 07:45 Tomasz Kaminski, <[email protected]> wrote: >>> >>>> >>>> >>>> On Tue, Jun 30, 2026 at 7:48 PM Jonathan Wakely <[email protected]> >>>> wrote: >>>> >>>>> Although the systemd docs say that /etc/localtime should be a symlink >>>>> to >>>>> one of the zoneinfo files, some systems make it a symlink to another >>>>> path, where that second path is a symlink to a zoneinfo file (e.g. if >>>>> /etc is mounted read-only then /etc/localtime can be a symlink to >>>>> another symlink on a writable disk, so that the system timezone can be >>>>> altered by re-pointing the symlink on the writable disk). >>>>> >>>>> In that case, using readlink would only tell us the location of the >>>>> second symlink, not which zoneinfo file it points to. Therefore, we >>>>> would not be able to extract a valid time zone name from the path, and >>>>> chrono::current_zone() would fail. >>>>> >>>>> To support multiple symlinks we could recursively keep resolving >>>>> symlinks with readlink until we reach a path from which we can extract >>>>> a >>>>> zone name. Alternatively, we can just use realpath to resolve all >>>>> symlinks to a physical file (which is what HowardHinnant/date does). >>>>> This means we only need one system call and don't need the extra >>>>> complexity of calling readlink in a loop. >>>>> >>>>> The realpath system call also removes redunant slashes, so we can >>>>> remove >>>>> the code that did that manually. >>>>> >>>>> The possible downsides of this approach that I'm aware of are: >>>>> >>>>> - When /etc/localtime is a symlink to /invalid/Europe/London but that >>>>> file doesn't exist. With the previous implementation we would have >>>>> resolved that symlink to the zone "Europe/London" as long as that >>>>> name >>>>> is known to the current chrono::tzdb object. With this change, we >>>>> won't get a valid zone name and current_zone() will fail. I'm not >>>>> sure >>>>> how realistic this case is. It might be plausible if libstdc++ is >>>>> using the embedded static copy of tzdata.zi and there are no zoneinfo >>>>> files on disk at all. In that case the system might still use >>>>> /etc/localtime to name a zone, even though the symlink is dangling. >>>>> We could fall back to filesystem::weakly_canonical for this case, but >>>>> this patch leaves that for a future change, if it turns out to be >>>>> needed by any users. >>>>> >>>>> - When /etc/localtime is a symlink to /usr/share/zoneinfo/Foo/Bar where >>>>> "Foo/Bar" is a valid zone in the chrono::tzdb object, but the Bar >>>>> file >>>>> is another symlink to ./Baz where "Foo/Bar" is also a valid zone. >>>>> With the previous implementation current_zone() would have returned >>>>> the "Foo/Bar" zone. With this change it would return "Foo/Baz". I >>>>> don't think it's realistic to have two zones which are distinct zones >>>>> (not a Zone and a Link to it) but where one of them is defined >>>>> on-disk >>>>> using a symlink to the other. >>>>> >>>>> libstdc++-v3/ChangeLog: >>>>> >>>>> PR libstdc++/125467 >>>>> * src/c++20/tzdb.cc (tzdb::current_zone): Use realpath to >>>>> resolve the /etc/localtime symlink instead of readlink. >>>>> --- >>>>> >>>>> v2: Make the type of 'str' always std::string_view. Check str != >>>>> "/etc/localtime" so that we don't bother trying to extract a zone name >>>>> from the symlink target if it isn't even a symlink. >>>>> >>>>> Tested x86_64-linux. >>>>> >>>> LGTM with very small suggestion. >>>> >>>>> >>>>> libstdc++-v3/src/c++20/tzdb.cc | 76 +++++++++++++--------------------- >>>>> 1 file changed, 28 insertions(+), 48 deletions(-) >>>>> >>>>> diff --git a/libstdc++-v3/src/c++20/tzdb.cc >>>>> b/libstdc++-v3/src/c++20/tzdb.cc >>>>> index 9e601fc176f3..c658e0c9cd37 100644 >>>>> --- a/libstdc++-v3/src/c++20/tzdb.cc >>>>> +++ b/libstdc++-v3/src/c++20/tzdb.cc >>>>> @@ -41,8 +41,13 @@ >>>>> # include <ext/concurrence.h> // __gnu_cxx::__mutex >>>>> #endif >>>>> >>>>> -#if defined(_GLIBCXX_HAVE_READLINK) && defined(_GLIBCXX_HAVE_UNISTD_H) >>>>> -# include <unistd.h> // readlink >>>>> +#ifdef _GLIBCXX_HAVE_UNISTD_H >>>>> +# include <unistd.h> // _XOPEN_VERSION >>>>> +#endif >>>>> +#if defined _GLIBCXX_USE_REALPATH && _XOPEN_VERSION >= 700 >>>>> +# include <stdlib.h> // malloc, free, realpath >>>>> +#else >>>>> +# include <filesystem> // filesystem::canonicalize >>>>> #endif >>>>> >>>>> #ifdef _AIX >>>>> @@ -2098,58 +2103,33 @@ constinit tzdb_list::_Node::NumLeapSeconds >>>>> tzdb_list::_Node::num_leap_seconds; >>>>> // to have a way to force a re-read. >>>>> >>>>> #if !defined(_AIX) && !defined(_GLIBCXX_HAVE_WINDOWS_H) >>>>> -#if defined(_GLIBCXX_HAVE_READLINK) && defined(_GLIBCXX_HAVE_UNISTD_H) >>>>> - string_view str; >>>>> - char buf[128]; // strlen("../usr/share/zoneinfo/...") is usually >>>>> < 55 >>>>> - string dynbuf; >>>>> // /etc/localtime should be a symlink that ends with a zone name, >>>>> // e.g. /etc/localtime -> /usr/share/zoneinfo/Europe/London >>>>> // >>>>> https://www.freedesktop.org/software/systemd/man/latest/localtime.html >>>>> // This should work on GNU/Linux, macOS, NetBSD, and OpenBSD. >>>>> - // Some FreeBSD systems also use a symlink for /etc/localtime. >>>>> - // Use readlink directly to avoid std::filesystem overhead. >>>>> - if (auto n = ::readlink("/etc/localtime", buf, sizeof(buf)); n > >>>>> 0) >>>>> + // Some FreeBSD systems also use a symlink for /etc/localtime >>>>> (since 15.0). >>>>> + >>>>> + // N.B. we do not support dangling symlinks here. If that becomes >>>>> necessary >>>>> + // then after realpath fails we could fallback to using >>>>> + // >>>>> filesystem::weakly_canonical(filesystem::read_symlink("etc/localtime")). >>>>> + >>>>> +#if defined _GLIBCXX_USE_REALPATH && _XOPEN_VERSION >= 700 >>>>> + unique_ptr<char[], void(*)(void*)> cbuf{ nullptr, &::free }; >>>>> + string_view str; >>>>> >>>> This is very subjective and not necessary, but I would prefer this >>>> string_view to be >>>> declared before #if here. I find it easier to follow that way. >>>> >>> >>> The order I used means that the string view lifetime is shorter than the >>> buffer lifetime so it can't dangle. >>> >> OK. >> You would need to observe that from the destructor of the object created >> before this #ifdef, >> but it is some argument. >> > > Yeah, in this case it makes no difference because no dangling uses are > possible. It's just a general habit that I prefer to create the view so > that its lifetime is shorter than the underlying storage. > > I can change it here. > No need to, it's good habbit that maybe I should develop. > > > >>> >> >>> >>>> >>>>> + // Use realpath directly to avoid std::filesystem overhead. >>>>> + // We use realpath not readlink to resolve multiple levels of >>>>> symlinks. >>>>> + if (char* p = ::realpath("/etc/localtime", nullptr)) >>>>> { >>>>> - if (static_cast<size_t>(n) < sizeof(buf)) >>>>> - str = string_view(buf, n); >>>>> - else [[unlikely]] >>>>> - { >>>>> - // We read the symlink but it didn't fit in buf[], use >>>>> dynbuf. >>>>> - do >>>>> - { >>>>> - n *= 2; >>>>> - dynbuf.__resize_and_overwrite(n, [](char* p, size_t >>>>> len) { >>>>> - auto n2 = ::readlink("/etc/localtime", p, len); >>>>> - if (n2 == -1) // symlink removed or replaced by >>>>> file?! >>>>> - __throw_runtime_error("tzdb: error reading >>>>> /etc/localtime"); >>>>> - const size_t r = n2; >>>>> - return r < len ? r : 0; >>>>> - }); >>>>> - } >>>>> - while (dynbuf.empty()); >>>>> - str = dynbuf; >>>>> - } >>>>> + cbuf.reset(p); >>>>> + str = p; >>>>> } >>>>> +#else >>>>> + string sbuf = >>>>> std::filesystem::canonical("/etc/localtime").string(); >>>>> + string_view str = sbuf; >>>>> >>>> And this will become an assignment. >>>> >>>>> +#endif >>>>> >>>>> - if (!str.empty()) >>>>> + if (!str.empty() && str != "/etc/localtime") >>>>> { >>>>> - // Remove any redundant slashes so we can match zone names. >>>>> - // e.g. /usr/share/zoneinfo/Europe//London is a valid symlink, >>>>> - // but won't match against "Europe/London". >>>>> - if (auto pos = str.rfind("//"); pos != str.npos) [[unlikely]] >>>>> - { >>>>> - if (str.data() != dynbuf.data()) >>>>> - dynbuf = str; >>>>> - string::size_type spos = pos; >>>>> - do >>>>> - { >>>>> - dynbuf.erase(spos, 1); >>>>> - spos = dynbuf.rfind("//", spos); >>>>> - } >>>>> - while (spos != dynbuf.npos); >>>>> - str = dynbuf; >>>>> - } >>>>> - >>>>> // Check the trailing components of the path against known >>>>> zone names. >>>>> // Valid IANA times zones can have one, two, or three parts, >>>>> e.g. >>>>> // "UTC", "Europe/London", and "America/Indiana/Indianapolis". >>>>> @@ -2175,10 +2155,10 @@ constinit tzdb_list::_Node::NumLeapSeconds >>>>> tzdb_list::_Node::num_leap_seconds; >>>>> str.substr(pos + 1))) >>>>> return tz; >>>>> } >>>>> -#endif >>>>> + >>>>> // Otherwise, look for a file naming the time zone. >>>>> string_view files[] { >>>>> - "/etc/timezone", // Debian derivates >>>>> + "/etc/timezone", // Debian derivates, non-systemd Gentoo >>>>> "/var/db/zoneinfo", // FreeBSD >>>>> }; >>>>> for (auto f : files) >>>>> -- >>>>> 2.54.0 >>>>> >>>>>
