Repository navigation
fstat64 required on Windows #67341
Description
| BPO | 23152 |
|---|---|
| Nosy | @pitrou, @vstinner, @tjguk, @benjaminp, @hynek, @zware, @zooba, @MojoVampire |
| Files |
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']
Activity
zooba commentedon Jan 2, 2015
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 commentedon Jan 3, 2015
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 commentedon Jan 3, 2015
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 commentedon Jan 3, 2015
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 commentedon Jan 3, 2015
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 commentedon Jan 3, 2015
Looks like the easiest fix here is to remove the HAVE_SYS_STAT_H definition and replace it with the include directly:
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 commentedon Jan 8, 2015
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 commentedon Jan 8, 2015
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 commentedon Jan 8, 2015
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 commentedon Jan 15, 2015
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 commentedon Jan 22, 2015
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
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 commentedon Jan 23, 2015
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 commentedon Jan 25, 2015
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 commentedon Jan 25, 2015
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.
4 remaining items
zooba commentedon Feb 19, 2015
Last call before I let the buildbots be the reviewers :)
python-dev commentedon Feb 21, 2015
New changeset 4f6f4aa0d80f by Steve Dower in branch 'default':
Issue bpo-23152: Implement _Py_fstat() to support files larger than 2 GB on Windows.
https://hg.python.org/cpython/rev/4f6f4aa0d80f
vstinner commentedon Feb 21, 2015
The name of attribute_data_to_stat() and other shared functions must be
prefixed by "_Py".
+/* Return size of file in bytes; < 0 if unknown or INT_MAX if too big */
static off_t getfilesize(FILE *fp)
Hum since we have a type able yo store the file size and the function is
private, you should use the type able to store the size. Maybe off_t on
POSIX and something else on Windows. Is Py_off_t type 64 bits?
I'm the sure that the caller works if getfilesize() returns a truncated
size.
@@ -420,9 +420,11 @@ fileio_init(PyObject *oself, PyObject *a }
self->blksize = DEFAULT_BUFFER_SIZE; -#ifdef HAVE_FSTAT - if
(fstat(self->fd, &fdfstat) < 0) +#if defined(HAVE_FSTAT) ||
defined(MS_WINDOWS) + if (_Py_fstat(self->fd, &fdfstat) < 0) { +
PyErr_SetFromErrno(PyExc_OSError); goto error; + }
Why do you raise an exception here? Is it a bug fix? (I cannot read the
code at error label right now.)
zooba commentedon Feb 21, 2015
The caller to getfilesize is only using it to check whether it's small enough to load the file into memory all at once, so "too big" is an okay response (that function is in marshal.c and not used anywhere else).
The error label just returns back to the interpreter loop, previously without an exception set, so we got a SystemError. I mentioned this in my first post.
Happy to rename the now shared functions - I just took those straight from your patch :)
python-dev commentedon Feb 21, 2015
New changeset 307713759a62 by Steve Dower in branch 'default':
Issue bpo-23152: Renames attribute_data_to_stat to _Py_attribute_data_to_stat
https://hg.python.org/cpython/rev/307713759a62
python-dev commentedon Feb 21, 2015
New changeset a824c40e8fc0 by Steve Dower in branch 'default':
Issue bpo-23152: Renames time_t_to_FILE_TIME to _Py_time_t_to_FILE_TIME, removes unused struct win32_stat and return value
https://hg.python.org/cpython/rev/a824c40e8fc0
vstinner commentedon Feb 22, 2015
You didn't reply to my question on getfilesize().
zooba commentedon Feb 22, 2015
Two posts ago: "The caller to getfilesize is only using it to check whether it's small enough to load the file into memory all at once, so "too big" is an okay response (that function is in marshal.c and not used anywhere else)."
python-dev commentedon Feb 22, 2015
New changeset b4f9e787b857 by Serhiy Storchaka in branch 'default':
Issue bpo-23152: Move declaration into a header and exclude from stable API.
https://hg.python.org/cpython/rev/b4f9e787b857
python-dev commentedon Feb 22, 2015
New changeset 72cf174cc0eb by Serhiy Storchaka in branch 'default':
Issue bpo-23152: Move declarations back to posixmodule.c.
https://hg.python.org/cpython/rev/72cf174cc0eb
gh-67341: fix a typo in `Include/fileutils.h` (#136049)
pythongh-67341: fix a typo in `Include/fileutils.h` (python#136049)
pythongh-67341: fix a typo in `Include/fileutils.h` (python#136049)