fstat64 required on Windows

[zooba]

BPO	23152
Nosy	@pitrou, @vstinner, @tjguk, @benjaminp, @hynek, @zware, @zooba, @MojoVampire
Files	py_fstat.patch py_fstat_2.patch py_fstat_3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zooba'
closed_at = <Date 2015-02-22.06:56:51.263>
created_at = <Date 2015-01-02.20:23:52.012>
labels = ['expert-IO', 'type-crash']
title = 'fstat64 required on Windows'
updated_at = <Date 2015-02-22.19:35:42.217>
user = 'https://github.com/zooba'

bugs.python.org fields:

activity = <Date 2015-02-22.19:35:42.217>
actor = 'python-dev'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2015-02-22.06:56:51.263>
closer = 'steve.dower'
components = ['IO']
creation = <Date 2015-01-02.20:23:52.012>
creator = 'steve.dower'
dependencies = []
files = ['37639', '37845', '37888']
hgrepos = []
issue_num = 23152
keywords = ['patch']
message_count = 26.0
messages = ['233327', '233333', '233335', '233336', '233338', '233341', '233638', '233639', '233653', '234092', '234512', '234567', '234647', '234654', '234870', '235790', '236208', '236374', '236376', '236377', '236380', '236393', '236401', '236409', '236413', '236415']
nosy_count = 10.0
nosy_names = ['pitrou', 'vstinner', 'tim.golden', 'benjamin.peterson', 'stutzbach', 'python-dev', 'hynek', 'zach.ware', 'steve.dower', 'josh.r']
pr_nums = []
priority = 'critical'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue23152'
versions = ['Python 3.5']

Linked PRs

• gh-67341: fix comment about windows link stat mapping #136049

[zooba]

Currently test_largefile is failing on the Windows buildbots because an fstat() call in Modules/_io/fileio.c is failing. fstat() returns a 32-bit size, but the file being opened is larger than 2GB.

This appears to be a change in the CRT where it would previously succeed with an incorrect value but now returns an error. (While we're here, there is no exception set in this case, so you get "SystemError: NULL result without error in PyObject_Call", but that's not the core issue.)

I can fix this instance, but I suspect we may need to fix this in multiple places. fstat64 (or _fstat64) seems to exist on multiple platforms, and the docs I found (e.g. http://linux.die.net/man/2/fstat64) suggest that EOVERFLOW is always going to occur in this case, so is there any reason not to switch to fstat64 everywhere? Maybe enable it through pyport.h/pyconfig.h and have a #define that is set based on availability?

[MojoVampire]

I believe explicitly calling the 64 bit version of a function is usually frowned upon. At least on *NIX systems, the standard solution is to define -D_FILE_OFFSET_BITS=64 during the build process, so off_t seamlessly becomes a 64 bit value, and the 64 bit version of appropriate functions is called and linked (fstat64, mmap64, etc.).

[MojoVampire]

Ugh. Looking into it further, POSIX systems tend to support _FILE_OFFSET_BITS=64, but Windows isn't POSIX-y enough. So Windows might need to be special cased regardless. Blech.

[pitrou]

Josh is right, we would only need to use fstat64() under Windows here. fstat() under POSIX defines st_size as a off_t, which should usually be large enough even on modern 32-bit systems.

[zooba]

Okay, I'll try and find a way to redefine it only under Windows. Unfortunately, the CRT defines fstat() as a function, which makes it hard to redefine without eagerly including sys/stat.h.

[zooba]

Looks like the easiest fix here is to remove the HAVE_SYS_STAT_H definition and replace it with the include directly:

/* Define to 1 if you have the <sys/stat.h> header file. */
/* #define HAVE_SYS_STAT_H 1 */
#ifndef MS_WINCE
/* Rather than define HAVE_SYS_STAT_H, we include it now and
 rename two of the functions. The rename must be after the
 header is included. */
#include <sys/stat.h>
#define fstat _fstati64
#define stat _stati64
#endif

Does anyone know whether this sort of thing will cause problems with the build? It seems fine to me, but someone with more experience may know better.

[vstinner]

Here is a patch adding a new _Py_fstat() function which uses signed 64-bit integer to store the file size and so is not limited to 2 GB files. I just moved the code from posixmodule.c to fileutils.c.

The patch replaces calls to fstat() with _Py_stat() (and also change the stat structure to _Py_stat_struct).

I didn't modified _Py_stat() which calls _wstat() on Windows and so also have this issue (limited to 2 GB files on Windows). _Py_stat() is called in zipimport.c, so zipped packages are limited to 2 GB on Windows. I don't know if it's a severe issue or not. os.stat() implementation is more complex than os.fstat() on Windows.

I'm unable to test my patch on Windows because Visual Studio 2010 is unable to open the new PCbuild/pcbuild.sln: see my comment on the issue bpo-22919.

[vstinner]

#define fstat _fstati64

This change (alone) is not safe because _fstati64() doesn't use "struct stat" but "struct __stat64".

I don't like such global define, I may have unexpected side effect. I prefer to modify directly calls to fstat() in .c files (like the change implemented in my patch).

[zooba]

I prefer your patch too. (I've posted on the other thread about the build problems, and I'll test this when I get a chance.)

[zooba]

Victor, I've been testing your patch and it's mostly good (a few obscure errors you'd never find without a compiler), but I think we also need to update the win32_xstat functions in posixmodule.c, since they all try and use the same struct.

I don't know how familiar you are with the posixmodule functions, so I can study up on them if needed. It's probably due for some simplifications anyway, since we can now assume that Vista's APIs are always available. I think a lot of the functionality there is now in fileutils.c too, so we don't need so much duplicated code.

[vstinner]

Currently test_largefile is failing on the Windows buildbots

Oh yes, I just noticed the following bug on "AMD64 Windows8 3.x":
http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x/builds/307/steps/test/logs/stdio

[352/391/1] test_largefile
Assertion failed: (result != NULL && !PyErr_Occurred()) || (result == NULL && PyErr_Occurred()), file ..\Objects\abstract.c, line 2095
R6010

• abort() has been called
  Fatal Python error: Aborted

Current thread 0x00000ec8 (most recent call first):
File "D:\buildarea\3.x.bolen-windows8\build\lib\test\test_largefile.py", line 83 in test_lseek
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\case.py", line 577 in run
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\case.py", line 625 in __call__
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\suite.py", line 125 in run
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\suite.py", line 87 in __call__
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\suite.py", line 125 in run
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\suite.py", line 87 in __call__
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\suite.py", line 125 in run
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\suite.py", line 87 in __call__
File "D:\buildarea\3.x.bolen-windows8\build\lib\unittest\runner.py", line 168 in run
File "D:\buildarea\3.x.bolen-windows8\build\lib\test\support\init.py", line 1770 in _run_suite
File "D:\buildarea\3.x.bolen-windows8\build\lib\test\support\init.py", line 1804 in run_unittest
File "D:\buildarea\3.x.bolen-windows8\build\PCbuild\..\lib\test\regrtest.py", line 1283 in test_runner
File "D:\buildarea\3.x.bolen-windows8\build\PCbuild\..\lib\test\regrtest.py", line 1284 in runtest_inner
File "D:\buildarea\3.x.bolen-windows8\build\PCbuild\..\lib\test\regrtest.py", line 967 in runtest
File "D:\buildarea\3.x.bolen-windows8\build\PCbuild\..\lib\test\regrtest.py", line 763 in main
File "D:\buildarea\3.x.bolen-windows8\build\PCbuild\..\lib\test\regrtest.py", line 1568 in main_in_temp_cwd
File "D:\buildarea\3.x.bolen-windows8\build\PCbuild\..\lib\test\regrtest.py", line 1593 in <module>

[zooba]

Yeah, that's the sole buildbot currently running VS 2015. I'm expecting to have more after VS 2015 RC is released, since that will be "basically finished". Until then, I'm also regularly building with the latest internal versions and tracking issues, but nothing seems to have been introduced since Preview, so that buildbot is a very good guide.

[zooba]

I updated and tested your patch - basically making posixmodule.c use the new _Py_stat_struct throughout instead of the Win32 definition. The tests run fine on my VC14 machine.

Should _Py_fstat be conditioned on Py_LIMITED_API in some way, since it's not available pre-3.5? I've never figured out exactly how that should work.

[zooba]

Huh, okay, looks like the patch still isn't sufficient (I forgot to put -uall when testing it...) - there are calls in fileio.c that need changing too.

I'll try and do a thorough sweep of calls to fstat and post another patch in the next day or so.