View Ticket
Not logged in
Ticket UUID: 4718b41c56d8c135b018b589c3f031b1262d26c6
Title: windows x86 & x64: file mtime overflows in modification date (2038?, windows 32-bit time_t?)
Type: Bug Version: >= 8.5
Submitter: sebres Created on: 2019-07-04 19:39:47
Subsystem: 16. Commands A-H Assigned To: jan.nijtmans
Priority: 5 Medium Severity: Severe
Status: Closed Last Modified: 2019-09-08 10:16:33
Resolution: Fixed Closed By: fvogel
    Closed on: 2019-09-08 10:16:33
Description:

set fn test.txt; close [open $fn w]
puts [file mtime $fn [clock scan 01/01/2070 -gmt 1]]
puts [clock format [file mtime $fn] -gmt 1]
file delete $fn
this code produces following output on windows (regardless x86 or x64):
-1139207296
Sa Nov 25 17:31:44 GMT 1933

User Comments: fvogel added on 2019-09-08 10:16:33:

I confirm that the two computers I was checking this on (that is on Win7 and Win10), with your latest change [b88d81cb55], the two tests cmdAH-24.20.1 and cmdAH-24.20.2 are 'SKIPPED: time64bit'. So this works, thanks!


jan.nijtmans added on 2019-09-06 07:55:18:
Well, @fvogel's tests indicate that we cannot thrust sizeof(time_t) any-more. Microsoft could any day drop support for USE_32BIT_TIME_T, and our API will be scruwed again. I have an idea how to solve that. Stay tuned....

fvogel added on 2019-09-06 03:54:39:
Visual 2008, not 22008 (of course, sorry).

fvogel added on 2019-09-06 03:53:48:

I confirm the two tests cmdAH-24.20.1 and cmdAH-24.20.2 do npt pass on my Win10 computer at [e0b68fe7c6909818]. They do pass now at [696c59810e].

These tests pass on Vista either at [e0b68fe7c6909818] or at [696c59810e].

Isn't this a bit strange ? Visual Studio Express 22008 on both computers.

Anyway, thanks!


sebres added on 2019-09-05 22:16:27:
But isn't the size of Tcl_StatBuf different for binaries (e.g. modules like tcltest library show us) compiled before and after [e0b68fe7c6909818]?
I think yes... And I can imagine it depends and could get wrong data or even segfault if using Tcl_FSStat and co over statPtr allocated in system stack (yes, I know about Tcl_AllocStatBuf and access functions, but... shit happens).
Also note someone can write a module (binary) targeting both versions (and in 8.5 we had not the access functions like Tcl_GetModificationTimeFromStat, so one would not use it)...
Also note it can affect some builds mixed includes of Tcl-API with something else using time_t, so in best case it can stop to compile (where it was succeeded previously) or simply stop to work in worse case.
Just curious one could designate such thing as binary incompatibility.

jan.nijtmans added on 2019-09-05 21:21:34:
>@Jan: looking at [e0b68fe7c6909818], would it not "cause binary incompatibility" (as you wanted to tell me somewhere below?;)

Nice try ;-)   It - indeed -  means that there's a conflict regarding 3 stub functions: TclpGetDate(), TclpLocalTime() and TclpGmtime(), extensions calling any of those 3 might be in trouble. Lucky: Those functions are in the internal stub table, they will be removed in Tcl 9.0. So they cannot be depended upon already anyway.

Thanks for - finally - fixing this. Although tclWinPort.h is not the best location to set _USE_32BIT_TIME_T (it should be done by "configure" or "rules.vc", it's not really worth
to fix that now. API's using Tcl_StatBuf are OK, that's what counts the most because - yes - those functions are in the public API. Pity that sizeof(time_t) cannot be relied upon ...

Again, thanks all!

fvogel added on 2019-09-05 20:03:50:

And now it's also working at [e0b68fe7c6909818] !!!

However this is on a different computer (it's running Vista) than the one I was using earlier today when reporting the fix didn't work (that one was running Win10). Will check the Win10 computer again.


fvogel added on 2019-09-05 19:54:50:

After cleaning and building again, at [696c59810e] I don't see the test fail any more (they are both SKIPPED: time64bit). Great, many thanks!


sebres added on 2019-09-05 17:47:26:

@Jan: looking at [e0b68fe7c6909818], would it not "cause binary incompatibility" (as you wanted to tell me somewhere below?;)

Just kidding (please don't get me wrong, I'm for this fix, just the option "time64bit" does not really make sense anymore, furthermore Tcl_StatBuf has seemed never be usable in 8.6+ for windows x86 before the fix, since 2012:)...

So basically in 8.6+ we can rewrite the whole fix without considering a backwards compat (so without an option "time64bit" and the stuff).


sebres added on 2019-09-05 16:19:33:

Hmm... although I don't know what is specified in SDK used in Express2008, but [696c59810e] should provide more "precise" constraint using the size of same value in structure (not of the time_t anymore).

Can you please try this, François?


jan.nijtmans added on 2019-09-05 15:43:42:

Well, I just did a new clean 32-bit build using VS2019. Everything fine ... Sorry, but I cannot reproduce the problem.


fvogel added on 2019-09-05 15:29:03:
I'm afraid I have cleaned and rebuilt everything...

Am I the only one to see this?

sebres added on 2019-09-05 14:54:59:

Could it be a dependency issue this time (don't know tcltest resp. tclWinTest would be recompiled if tclWinPort.h going changed) - did you try to rebuild tcltest? so for example open and save win/tclWinTest.c once and then build it.

Surely we could check size of Tcl_StatBuf.st_mtime in test-constraing but I'm pretty sure it'd be declared as time_t st_mtime;.


jan.nijtmans added on 2019-09-05 14:41:07:
Somehow, tclWinTest.c is compiled without -D_USE_32BIT_TIME_T (defined in tclWinPort.h). Otherwise [testsize time_t] cannot return 8, it should return 4.  So, what else could it be?

At least, we know now that the test-case build is wrong, not the build of tcl itself.

fvogel added on 2019-09-05 14:12:22:
Unfortunately I'm still seeing the two tests failing. Problem must be elsewhere.

jan.nijtmans added on 2019-09-05 07:11:26:

It turns out, this was a long-standing bug (in the test-suite only), introduced by me in 2012 in this commit: [701864f6d2fc978d]. Since there was no test-case using time_t it was never discovered.

Thanks, again, for the report. And many thanks to @fvogel and @sebres for helping me track down the bug!


jan.nijtmans added on 2019-09-05 07:01:32:

Fixed in [e0b68fe7c6909818]

The problem is that tclWinTest.c is not compiled with -DBUILD_tcl, it shouldn't be part of the check in tclWinPort.h. (In Tcl 8.5 it was correct, no idea how this slipped into 8.6)

Thanks for the report!


fvogel added on 2019-09-05 05:22:28:

Here is the requested output:

puts $::tcl_platform(pointerSize)   -->  4
puts [testsize time_t]              -->  8


sebres added on 2019-09-04 22:11:53:
Hmm... Strange. The constraint is pretty simple and clear: either it is x64 build (sizeof(pointer) is 8, so it should be fixed there), or sizeof(time_t) is 8 (so basically no reason to use 32-bit stat).

To get a clue can you please provide what do you see on:

puts $::tcl_platform(pointerSize);
puts [testsize time_t]

Maybe testing of size of time member in stat structure would be better idea.

fvogel added on 2019-09-04 20:55:17:

With core-8-6-branch at [2ff3869960] (2019-08-30) built with Visual Express 2008 on Windows I'm still seeing tests failures:

==== cmdAH-24.20.1 Tcl_FileObjCmd: atime 64-bit time_t, bug [4718b41c56] FAILED
==== Contents of test case:

    list [file atime $filename 3155760000] [file atime $filename]

---- Result was:
-1139207296 -1139207296
---- Result should have been (exact matching):
3155760000 3155760000
==== cmdAH-24.20.1 FAILED

==== cmdAH-24.20.2 Tcl_FileObjCmd: mtime 64-bit time_t, bug [4718b41c56] FAILED
==== Contents of test case:

    list [file mtime $filename 3155760000] [file mtime $filename]

---- Result was:
-1139207296 -1139207296
---- Result should have been (exact matching):
3155760000 3155760000
==== cmdAH-24.20.2 FAILED

I built without passing OPTS=time64bit therefore it looks as if the constraint 'time64bit' is erroneously true for my setup.


jan.nijtmans added on 2019-07-12 12:42:52:
> why you reverted check of CRT ...

The idea is that if you use --enable-time64bit, you apparently don't care about binary compatibility, so why check for it (that's what the CRT check does) ....

sebres added on 2019-07-12 12:30:53:

Hmmm... not sure __MINGW_USE_VC2005_COMPAT should be necessarily used as define in case of MSVC (it is still mingw option), and why you reverted check of CRT (so it covering only x86 with 32-bit time_t, but not vice versa in case of 64-bit time_t), but OK, accepted as it is :)...

Thx.

Thus merged now in all branches, excepting trunk (9.0 remains time_t 64-bit per default, so no merge point only without functional changes).

Maybe we could add still one travis point for 32-bit build with this option (configure --disable-64bit --enable-time64bit).


jan.nijtmans added on 2019-07-10 21:10:19:
Thanks. I made a few tweaks, hoping you agree with this ;-)

In my opinion, it's OK to be merged to core-8-5-branch and up now. For 9.0 we only want to support 64bit time, so the time64bit option should be removed there.

Again, thanks for all your work!

sebres added on 2019-07-10 18:30:18:

Branch is rewritten now (see [d263bc472a], this way it is compatible with previous 8.x behavior in 32-bit mode per default).

To enable 64-bit time_t for x86 build:

  • Visual Studio (nmake):

    * either use option time64bit:

    nmake -nologo -f makefile.vc tcltest OPTS=...,time64bit
    
    * or use optimizations (via define _USE_64BIT_TIME_T)
    nmake -nologo -f makefile.vc tcltest OPTS=... OPTIMIZATIONS="-D_USE_64BIT_TIME_T"
    
  • MinGW:

    * either via configure:

    ../configure --disable-64bit --enable-time64bit
    
    * or using make via CFLAGS (via define _USE_64BIT_TIME_T)
    make tcltest CFLAGS=-D_USE_64BIT_TIME_T
    

Both new tests cmdAH-24.20* in branch are conditional (by constraint time64bit), so in x64-build or in x86-build with enabled time64bit they should pass and in x86-build without time64bit they are ignored:


.\tcltest ../../tests/all.test -file "cmdAH.test" -match "*-24.20*"

# x64 / x86 with time64bit: all.tcl: Total 305 Passed 2 Skipped 303 Failed 0

# x86 without time64bit: all.tcl: Total 305 Passed 0 Skipped 305 Failed 0 Sourced 1 Test Files. Number of tests skipped for each constraint:

  1. time64bit

@Jan I'm done with this now, so if it is good enough, I'd like to merge it in 8.5 and above.
If don't, then just close the ticket, I'll then integrate it in my own forks only.


sebres added on 2019-07-09 07:58:40:

Just by the way...

> Just switch to either Tcl 9.0 or Win64 before 2038, there's still more than 18 years left before it becomes a problem.

Nope, in my case it is impossible, because I'd need it right now :)

There are some SAN/NFS/CIFS systems (for example like NetApp Snaplock) which uses an access time as retention time in order to protect a document in WORM against modification/removal at hardware level. Or some others which calculates a retention period from the artificial modification time.

And there are some clusters/documents (e. g. tax or archive relevant things) which should be immutable 20-30 years (and not rare still longer), so we're already in year 2039-2049 ;)

Also to ignore such bug (or silently write wrong time) is not an option in my case.


jan.nijtmans added on 2019-07-09 07:13:01:
No problem. Thanks!

At that time, (in 2011), the activebuild version of Tcl/Tk was - kind of - the standard. So, yes, when builds with other compilers/versions turn out to be binary incompatible with the 'standard' build, that should be fixed. You don't like that, and I don't like to do that ;-), but it's necessary IMHO.

> The alternative would be to use some internal structure instead of Tcl_StatBuf ...

Yes that would be possible. I think it's not really worth it: Just switch to either Tcl 9.0 or Win64 before 2038, there's still more than 18 years left before it becomes a problem. Tcl 8.x on win32 should be dead long before that .... (My guess is that win32 - actually - will die before Tcl 8.x dies).

I'll leave this ticket open for you, in case you want to work on it. If not, feel free to close it as "Fixed, except for Tcl 8.x on win32". Changing Tcl_StatBuf or unsetting _USE_32BIT_TIME_T is forbidden, almost everything else is allowed!

Again, thanks!

sebres added on 2019-07-09 06:44:03:

> But ... branch [bug-4718b41c56] causes binary incompatibility

Sure, therefore I wrote that it should be additionally protected by some define, so allow controlling at compile/configure time (and it can be compatible per default). The branch is just an example (or rather PoC).

The alternative would be to use some internal structure instead of Tcl_StatBuf and rewrite internal handling using this and then wrap it in public APIs into Tcl_StatBuf members if the sizes of both structures are different. But it is a bit too aggressive in my opinion and a define around could be enough as an opportunity to fix 32-bit builds.

Anyway it is better as to say "cannot fix a Tcl-level command, due to the binary compat purposes"...

> We (= the TCT) promise binary compatibility during the full 8.x release, so any extension compiled against earlier version of the Tcl headers should continue to work in future 8.x version. I'm not prepared to break this promise. Sorry.

OK, you (= the TCT) have an exclusive prerogative to break the compatibility :)
Because we (!= the TCT) did see that already too often... ;) (and back to the issue) for example the revision [8a10f0c0ea9da7f7] doing almost exactly that for all the 8.x branches and all extensions using tcl-headers together with time_t type (and related like utimebuf, etc), because a definition of _USE_32BIT_TIME_T causes switch of time_t (and several CRT-functions and directives up-to pragma link) to 32-bit.


jan.nijtmans added on 2019-07-08 20:47:58:

But ... branch [bug-4718b41c56] causes binary incompatibility in any extension using Tcl_StatBuf. So at least the "tclvfs" and "trofs" extension need to be recompiled, after this change goes in. That's not acceptable.

We (= the TCT) promise binary compatibility during the full 8.x release, so any extension compiled against earlier version of the Tcl headers should continue to work in future 8.x version. I'm not prepared to break this promise. Sorry.

For Tcl 9.0, this binary compatibility was never promised, so that's the appropriate release to put in the full fix in.


sebres added on 2019-07-08 20:34:28:

Branch bug-4718b41c56 should fix this for x86 build >= 8.5 and CRT supporting 64-bit time_t and provides test-cases covering this bug.

Possibly it can (or should) be additionally protected by some define (or configure option/CFLAGS/etc) for binary compatibility across several windows platform if compiled and/or executed with different CRT-libraries.

Or we could simply rewrite it using something like this:

+ #if defined(_USE_64BIT_TIME_T) || defined(TCL_FORCE_64BIT_TIME)
...
+ #endif


sebres added on 2019-07-08 15:29:07:

Hmmm... amend for my last comment with better diff:

- #ifndef _WIN64
+ #if !defined(_TIME_T_DEFINED) && !defined(_WIN64) && __MSVCRT_VERSION__ < 0x0800


sebres added on 2019-07-08 15:26:17:

I meant not the trunk (so your fix was not fulfilled for 8.5 - 8.7)...

Anyway it is fixed now in [ce186ca403] for x64 builds.

And x86 build with 64-bit time_t (newest CRT, I guess >= 8.0) is prevented at the moment by checkin [8a10f0c0ea9da7f7] for [3354324], so it forces this way to use 32-bit time_t (via define _USE_32BIT_TIME_T) regardless CRT can use 64-bit time_t.

May be this "hack" should be removed or we should extend it like here:

- #ifndef _WIN64
+ #if !defined(_TIME_T_DEFINED) && !defined(_WIN64) && !defined(__MSVCRT_VERSION__ < 0x0800)
  /* See [Bug 3354324]: file mtime sets wrong time */
  1. define _USE_32BIT_TIME_T #endif


jan.nijtmans added on 2019-07-08 15:09:38:
OK, this is what I get with tclsh90.exe, built with mingw-w64 (32-bit) from trunk:

tclsh90.exe
% set fn test.txt; close [open $fn w]
% puts [file mtime $fn [clock scan 01/01/2070 -gmt 1]]
3155760000
% puts [clock format [file mtime $fn] -gmt 1]
Wed Jan 01 00:00:00 GMT 2070
% file delete $fn
% parray tcl_platform
tcl_platform(byteOrder)     = littleEndian
tcl_platform(engine)        = Tcl
tcl_platform(machine)       = intel
tcl_platform(os)            = Windows NT
tcl_platform(osVersion)     = 10.0
tcl_platform(pathSeparator) = ;
tcl_platform(platform)      = windows
tcl_platform(pointerSize)   = 4
tcl_platform(threaded)      = 1
tcl_platform(user)          = Jan.Nijtmans
tcl_platform(wordSize)      = 4
%

So, what version of mingw did you compile with? 64-bit time_t for win32 was introduced in Visual Studio 2005, and only recently adopted in mingw-w64 (I tried this a few years ago, then it didn't work ....)

sebres added on 2019-07-08 13:54:57:
Thanks Jan, but...

- it still does not work if compiled with mingw (neither x64 nor x86);
- it should also work for x86 - sorry but a binary incompatibility as an argument regarding the Tcl-level commands (like file mtime) sounds like a nonsense, imho;

WiP

jan.nijtmans added on 2019-07-05 13:36:09:
Addendum: on trunk is it now fixed for win32 as well. For the 8.x range that cannot be done, because it would create a potential binary incompatibility in the API. For 9.0, it's OK to do that.

jan.nijtmans added on 2019-07-05 12:13:35:
Fixed for x64 in all active branches (8.5, 8.6, 8.7, 9.0)

jan.nijtmans added on 2019-07-05 09:13:36:
Reproduced. Thanks!  Fix underway.