From 9b1f72d89b6db38a05b958baf2195f552fe0aa57 Mon Sep 17 00:00:00 2001 From: Jonathan M Davis Date: Tue, 8 Aug 2017 18:01:34 -0600 Subject: [PATCH] Fix issue 12507: SysTime.init.toString() segfaults. This adds a special TimeZone type internal to SysTime which _timezone is directly initialized to so that SysTime.init will work without segfaulting but will still be uniquely identifiable with the is operator. Or at least, _timezone is _supposed_ to be directly initialized to it, but issue# 17740 currently prevents that. So, _timezone has been temporarily renamed to _timezoneStorage and private getters and setters named _timezone have been added. The getter then does a null check and returns InitTimeZone() for SysTime.init to simulate the member variable having been initialized to InitTimeZone(). Once issue# 17740 has been fixed, these accessors will be unnecessary, and the code should be updated so that _timezone is properly a variable again and is directly initialized with InitTimeZone(). The new TimeZone type - InitTimeZone - is internal to SysTime and can only be obtained by the timezone getter on SysTime.init. It acts the same as UTC except that it is not special-cased by the to*String functions and thus will print out its timezone as +00:00 instead of z, which is perfectly legitimate per the spec. And as such, if _timezone were directly initialized with InitTimeZone(), there would be no extra checks due to this change, and everything would just work. However, until issue# 17740 is fixed, there will be an extra null check any time that a function is called on _timezone, because _timezone is currently a wrapper that does a null check rather than being a member variable directly like it's supposed to be. Unlike previous attempts along these lines, this does not make it so that SysTime.init has NaN behavior such that any operation (other than assignment) on an an uninitialized SysTime would result in SysTime.init, and the timezone setter property does not set the SysTime to SysTime.init if it's passed this TimeZone. So, unfortunately, it _is_ possible to have other SysTime values with the special TimeZone, but it was deemed unnecessarily complex for too little benefit to add the NaN behavior. And regardless, SysTime.init is still uniquely identifiable via the is operator. It's just that it can't technically be uniquely identified by the timezone getter, which was never a supported feature anyway. --- std/datetime/systime.d | 103 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 94 insertions(+), 9 deletions(-) diff --git a/std/datetime/systime.d b/std/datetime/systime.d index 81a57dc7a43..766e6587d66 100644 --- a/std/datetime/systime.d +++ b/std/datetime/systime.d @@ -2003,7 +2003,14 @@ public: @property bool dstInEffect() @safe const nothrow { return _timezone.dstInEffect(_stdTime); - // This function's unit testing is done in the time zone classes. + } + + // This function's full unit testing is done in the time zone classes, but + // this verifies that SysTime.init works correctly, since historically, it + // has segfaulted due to a null _timezone. + @safe unittest + { + assert(!SysTime.init.dstInEffect); } @@ -2016,6 +2023,14 @@ public: return _timezone.utcOffsetAt(_stdTime); } + // This function's full unit testing is done in the time zone classes, but + // this verifies that SysTime.init works correctly, since historically, it + // has segfaulted due to a null _timezone. + @safe unittest + { + assert(SysTime.init.utcOffset == Duration.zero); + } + /++ Returns a $(LREF SysTime) with the same std time as this one, but with @@ -2342,7 +2357,7 @@ public: { import std.utf : toUTFz; timeInfo.tm_gmtoff = cast(int) convert!("hnsecs", "seconds")(adjTime - _stdTime); - auto zone = (timeInfo.tm_isdst ? _timezone.dstName : _timezone.stdName); + auto zone = timeInfo.tm_isdst ? _timezone.dstName : _timezone.stdName; timeInfo.tm_zone = zone.toUTFz!(char*)(); } @@ -2408,6 +2423,29 @@ public: assert(to!string(timeInfo.tm_zone) == "PDT"); } } + + // This is more to verify that SysTime.init.toTM() doesn't segfault and + // does something sane rather than that the value is anything + // particularly useful. + { + auto timeInfo = SysTime.init.toTM(); + + assert(timeInfo.tm_sec == 0); + assert(timeInfo.tm_min == 0); + assert(timeInfo.tm_hour == 0); + assert(timeInfo.tm_mday == 1); + assert(timeInfo.tm_mon == 0); + assert(timeInfo.tm_year == -1899); + assert(timeInfo.tm_wday == 1); + assert(timeInfo.tm_yday == 0); + assert(timeInfo.tm_isdst == 0); + + version(Posix) + { + assert(timeInfo.tm_gmtoff == 0); + assert(to!string(timeInfo.tm_zone) == "SysTime.init's timezone"); + } + } } @@ -9035,18 +9073,65 @@ private: } - // Commented out due to bug http://d.puremagic.com/issues/show_bug.cgi?id=5058 - /+ - invariant() + final class InitTimeZone : TimeZone + { + public: + + static immutable(InitTimeZone) opCall() @safe pure nothrow @nogc { return _initTimeZone; } + + @property override bool hasDST() @safe const nothrow @nogc { return false; } + + override bool dstInEffect(long stdTime) @safe const nothrow @nogc { return false; } + + override long utcToTZ(long stdTime) @safe const nothrow @nogc { return 0; } + + override long tzToUTC(long adjTime) @safe const nothrow @nogc { return 0; } + + override Duration utcOffsetAt(long stdTime) @safe const nothrow @nogc { return Duration.zero; } + + private: + + this() @safe immutable pure + { + super("SysTime.init's timezone", "SysTime.init's timezone", "SysTime.init's timezone"); + } + + static immutable InitTimeZone _initTimeZone = new immutable(InitTimeZone); + } + + // https://issues.dlang.org/show_bug.cgi?id=17732 + @safe unittest + { + assert(SysTime.init.timezone is InitTimeZone()); + assert(SysTime.init.toISOString() == "00010101T000000+00:00"); + assert(SysTime.init.toISOExtString() == "0001-01-01T00:00:00+00:00"); + assert(SysTime.init.toSimpleString() == "0001-Jan-01 00:00:00+00:00"); + assert(SysTime.init.toString() == "0001-Jan-01 00:00:00+00:00"); + } + + // Assigning a value to _timezone in SysTime.init currently doesn't work due + // to https://issues.dlang.org/show_bug.cgi?id=17740. So, to hack around + // that problem, these accessors have been added so that we can insert a + // runtime check for null and then use InitTimeZone for SysTime.init (which + // which is the only case where _timezone would be null). This thus fixes + // the problem with segfaulting when using SysTime.init but at the cost of + // what should be an unnecessary null check. Once 17740 has finally been + // fixed, _timezoneStorage should be removed, these accessors should be + // removed, and the _timezone variable declaration should be restored. + pragma(inline, true) @property _timezone() @safe const pure nothrow @nogc + { + return _timezoneStorage is null ? InitTimeZone() : _timezoneStorage; + } + + pragma(inline, true) @property void _timezone(immutable TimeZone tz) @safe pure nothrow @nogc scope { - assert(_timezone !is null, "Invariant Failure: timezone is null. Were you foolish enough to use " ~ - "SysTime.init? (since timezone for SysTime.init can't be set at compile time)."); + _timezoneStorage = tz; } - +/ long _stdTime; - Rebindable!(immutable TimeZone) _timezone; + Rebindable!(immutable TimeZone) _timezoneStorage; + //Rebindable!(immutable TimeZone) _timezone = InitTimeZone(); } ///