View Issue Details

IDProjectCategoryView StatusLast Update
0005042The Dark ModCodingpublic16.08.2019 07:04
ReporterstgatilovAssigned Tostgatilov 
PriorityurgentSeveritymajorReproducibilitysometimes
Status assignedResolutionopen 
Product VersionTDM 2.07 
Target VersionTDM 2.08Fixed in Version 
Summary0005042: Timestamps fiasco: Unix vs DOS, local vs UTC
DescriptionThe engine code heavily relies on file timestamps. They influence incremental reloading of assets, choosing how to resolve conflict when same asset is present in many places, etc. Moreover, if file timestamp cannot be determined properly, then the engine may decide that the file does not exist (but still read it at the same time). All of this causes terrible issues sometimes.

One problem about it was noticed in 2016 at TDM 2.05:
  http://forums.thedarkmod.com/index.php?/topic/18582-bogus-timestamps-in-tdm-update-files-cause-tdm-crashes-under-linux/&tab=comments#comment-399317
The next problem happened in 2019 at TDM 2.07 (confirmed):
  http://forums.thedarkmod.com/index.php?/topic/20050-proccpuinfo-cpu-frequency-200007-mhz-signal-caught-segmentation-fault-si_code-1/

The code stores timestamps as ID_TIME_T, which is a typedef for time_t from C. This type stores a Unix Timestamp, i.e. the number of seconds passed since Epoch, where Epoch is specified in UTC. Most importantly, Unix timestamp for an event is logically the same on every machine in every timezone. It is NOT a local time.

A major convention used in engine (e.g. by image loading code) is:
  If timestamp is -1, then the file was not loaded.
The same convention is used by mktime function, which computes Unit Timestamp from local time:
  If local time cannot be converted, then timestamp -1 is returned.
Adding these two conventions together: if some error happens when handling file timestamp, then the file is considered as "not loaded" (sort of... partially)

Now back to why we need local times at all.
The engine uses zip file format for all assets on the player side. Zip format is very old, and it stores last modification time of each file in DOS format. Which is just "year, month, day, hour, minute, second", without any timezone/DST info. By common convention, everyone treats DOS time as local time.
Since zip modification times are considered local, the engine converts them to Unix Timestamps on reading. This is done in function Sys_DosToUnixTime, and it calls aforementioned mktime standard function to convert from local time to Unit Timestamp.

Here are some problems:
1) mktime function works differently depending on locale/timezone/DST/whatever. It may fail due to some DST weirdness or due to random value stored in zip datetimes.
2) Sys_DosToUnixTime function does not initialize the field "dostm.tm_isdst" before calling mktime. Depending on the sign of this value, mktime considers input to be at non-DST/DST/uncertain moment, which may result in different output or even fail (depending on locale).
3) The pk4 files have somewhat imprecise timestamps. Because TDM server packages everything in one timezone, while players read these zips in another timezone.
TagsNo tags attached.

Relationships

related to 0004864 assignedstgatilov Remove pk4 timestamps fixes from updater 
related to 0003732 closedgrayman Engine should take daylight savings into account when determining which DLL to load 
related to 0003224 resolvedgrayman TheDarkMod.exe crashes after updating a 1.07 installation to 1.08 

Activities

stgatilov

stgatilov

12.08.2019 17:54

developer   ~0011816

I suggest hardening the handling of file/timestamps reading errors in several places, introducing a zillion of cvars in process. The cvars should have proper default values so that no one has to change them, but they should allow some freedom to fix issues if they happen.

Possible mitigations:
1) Somehow set tm.tm_isdst in Sys_DosToUnixTime. Perhaps to -1, perhaps to a value of some cvar. Also, maybe move this function to common Sys file, since now it is the same for Linux and Windows.
2) Treat file modification times from pk4 files as in UTC. Not a good thing, but why not.
3) Add more error checking in image loading code to avoid hard-to-debug crashes.
4) Print warning to game console when mktime fails.
5) Add handmade implementation of converting "year,month,day,hour,minute,second" into Unit Timestamp. Use this implementation a) always, b) when mktime fails, c) never (depending on cvar).
6) In case mktime fails, do something, Maybe just set some predefined valid timestamp.
7) Allow overriding timezone/dst/locale somehow. Maybe via locale function, maybe in conjunction with self-written date conversion.
stgatilov

stgatilov

13.08.2019 03:43

developer   ~0011817

This is the commit which added usage of zip timestamps (yeah, that's TDM-specific):

Revision: 5723
Author: taaaki
Date: 26 марта 2013 г. 10:58:38
Message:
Added timestamp check when determining whether to use the existing game dll/so in the executable path or the dll/so in the game pak file. Refer to issue 0003224.
----
Modified : /trunk/TypeInfo/main.cpp
Modified : /trunk/framework/File.cpp
Modified : /trunk/framework/File.h
Modified : /trunk/framework/FileSystem.cpp
Modified : /trunk/sys/posix/posix_main.cpp
Modified : /trunk/sys/sys_public.h
Modified : /trunk/sys/win32/win_main.cpp
stgatilov

stgatilov

16.08.2019 07:03

developer   ~0011818

Last edited: 16.08.2019 07:04

View 3 revisions

In the latest case:
  http://forums.thedarkmod.com/index.php?/topic/20050-proccpuinfo-cpu-frequency-200007-mhz-signal-caught-segmentation-fault-si_code-1
It happens as follows:
1) Uninitialized member dostm.tm_isdst turns out to be positive, which means "DST is in effect".
2) Function mktime consults records for the current timezone and sees that DST was never used for the current timezone. so it returns error (-1).
3) R_LoadImage considers that LoadTGA failed because timestamp is -1. It does not care that image buffer it not NULL.
4) LoadJPG is called. It clears the image buffer, but obviously fails to load jpg since there is no jpg.
5) The case "makeIntensity" in R_ParseImageProgram_r applies postprocessing to the image buffer without checking it for being NULL.
Note that mktime started returning -1 (step 2) only in recent versions of glibc (i.e. version 2.29). Previously it returned some positive result. Here is report about this change in glibc:
  https://bugzilla.redhat.com/show_bug.cgi?id=1653340

The workaround for TDM 2.07 (or earlier) is to set ANY timezone which has Daylight Saving Time (DST). For instance, Asia/Tehran is known to work.
I guess it is also possible to override timezone for TDM only by setting TZ environment variable:
  https://unix.stackexchange.com/questions/45803/run-an-application-with-different-timezone

Issue History

Date Modified Username Field Change
12.08.2019 17:46 stgatilov New Issue
12.08.2019 17:46 stgatilov Status new => assigned
12.08.2019 17:46 stgatilov Assigned To => stgatilov
12.08.2019 17:47 stgatilov Relationship added related to 0004864
12.08.2019 17:47 stgatilov Relationship added related to 0003732
12.08.2019 17:54 stgatilov Note Added: 0011816
13.08.2019 03:43 stgatilov Note Added: 0011817
13.08.2019 03:45 stgatilov Relationship added related to 0003224
15.08.2019 16:25 stgatilov Description Updated View Revisions
15.08.2019 16:25 stgatilov Description Updated View Revisions
16.08.2019 07:03 stgatilov Note Added: 0011818
16.08.2019 07:04 stgatilov Note Edited: 0011818 View Revisions
16.08.2019 07:04 stgatilov Note Edited: 0011818 View Revisions