-
-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Crash with mmap and sparse files on Mac OS X #55486
Comments
Following r88460 (bpo-10276), test_zlib crashes on the Snow Leopard buildbot (apparently in the new "test_big_buffer" test case). |
Do adler32() and crc32() support length up to UINT32_MAX? Or should we maybe limit the length to INT32_MAX? |
I've tried INT_MAX and it didn't change anything. |
Current OS X zlib is 1.2.3. Test crashes with most recently released zlib, 1.2.5, as well. |
Program received signal EXC_BAD_ACCESS, Could not access memory. (gdb) backtrace PyZlib_crc32(PyObject *self, PyObject *args)
...
while (len > (size_t) UINT_MAX) {
crc32val = crc32(crc32val, buf, UINT_MAX);
... |
So on my system, that 'while' loop is executed once (put a printf() after the bug and len adjustments and it was never hit). |
>>> from test.support import _4G
>>> _4G
4294967296
>>> mapping.size()
4294967300 pbuf.len = 4294967300, len = 4294967300
UINT_MAX = 4294967295 |
Does it matter that _4G < UINT_MAX? |
You mean _4G > UINT_MAX, right? |
'Have no glue, but Ned Daily's patch (msg129011) seems to be required for adler, too. (You know...) |
Well, it's not a patch, just a traceback :) |
Wait a few minutes, i'll write this simple patch for adler and crc. But excessive testing and such is beyond my current capabilities. |
File: bpo-11277.patch. |
Sorry - that was a mess. |
Er, how is this patch different from r88460? |
I guess not at all. Well. |
test_zlib.py (with my patch but that's somewhat identical in the end, say) does .............................s....... OK (skipped=1) This is on Snow Leopard 64 bit, 02b70cb59701 (r88451) -> Python 3.3a0. |
Have you tried "./python -m test -v -uall test_zlib" ? |
No, i've got no idea of this framework... Just did 'python3 test_zlib.py' directly. Thanks for the switch. But i can't test your thing due to bpo-11285, so this may take a while (others have more knowledge anyway).. (P.S.: your constant-folding stack patch is a great thing, just wanted to say this once..) |
So here is this (with my patch, but this is for real: bpo-11277.2.patch): == CPython 3.3a0 (py3k, Feb 22 2011, 14:00:52) [GCC 4.2.1 (Apple Inc. build 5664)] ---------------------------------------------------------------------- OK (skipped=1) |
(Is not that much help for a >4GB error, huh?) |
Just stepping ... with c8d1f99f25eb/r88476: == CPython 3.3a0 (py3k, Feb 22 2011, 14:18:19) [GCC 4.2.1 (Apple Inc. build 5664)] |
Right, that's what we should investigate :) |
.. even with a self-compiled 1.2.3, INT_MAX/1000 ... nothing. if (pbuf.len > 1024*5) {
unsigned char *buf = pbuf.buf;
Py_ssize_t len = pbuf.len;
Py_ssize_t i;
fprintf(stderr, "CRC 32 2.1\n");
for(i=0; (size_t)i < (size_t)len;++i)
*buf++ = 1;
fprintf(stderr, "CRC 32 2.2\n"); 2.2 is never reached (in fact accessing buf[1] already causes fault). |
(P.S.: of course talking about ChecksumBigBufferTestCase and the 4GB, say.) |
Thank you! So it's perhaps a bug in mmap on Snow Leopard. |
Snippet if (pbuf.len > 1024*5) {
volatile unsigned char *buf = pbuf.buf;
Py_ssize_t len = pbuf.len;
Py_ssize_t i = 0;
volatile unsigned char au[100];
volatile unsigned char*x = au;
fprintf(stderr, "CRC ENTER, buffer=%p\n", buf);
for (i=0; (size_t)i < (size_t)len; ++i) {
fprintf(stderr, "%ld, buf=%p\n", (signed long)i, buf);
*x = *buf++;
} results in test_big_buffer (test.test_zlib.ChecksumBigBufferTestCase) ... CRC ENTER, buffer=0x1014ab000 |
Out of curiosity, could you try the following patch? Index: Lib/test/test_zlib.py --- Lib/test/test_zlib.py (révision 88500)
+++ Lib/test/test_zlib.py (copie de travail)
@@ -70,7 +70,7 @@
with open(support.TESTFN, "wb+") as f:
f.seek(_4G)
f.write(b"asdf")
- f.flush()
+ with open(support.TESTFN, "rb") as f:
self.mapping = mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ)
def tearDown(self): |
Unless I'm mistaken, in the test the file is mapped with PROT_READ, so it's normal to get SIGSEGV when writting to it: def setUp(self):
with open(support.TESTFN, "wb+") as f:
f.seek(_4G)
f.write(b"asdf")
f.flush()
self.mapping = mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ)
But it seems you're also getting segfaults when only reading it, right ? I've got a stupid question: how much memory do you have ? On Linux, the page cache won't fill forever, so you don't need to have enough free memory to accomodate the whole file (the page cache should grow, but not forever). But on OS-X, it seems that the page replacement algorithm seems to retain mmaped pages in the page cache much longer, which could potentially trigger an OOM later (because of overcommitting, mmap can very well return a valid address range which leads to a segfault when accessed later). |
New changeset 7f3cab59ef3e by Victor Stinner in branch '2.7': |
New changeset e6a4deb84e47 by Victor Stinner in branch '2.7': |
@Haypo: Oh. Not: if sys.maxsize > _4G:
# (64 bits system) crc32() and adler32() stores the buffer size into an
# int, the maximum filesize is INT_MAX (0x7FFFFFFF)
filesize = 0x7FFFFFFF
crc_res = 0x709418e7
adler_res = -2072837729
else:
# (32 bits system) On a 32 bits OS, a process cannot usually address
# more than 2 GB, so test only 1 GB
filesize = _1G
crc_res = 0x2b09ee11
adler_res = -1002962529
I'm not that fast. |
@sdaoden(, @pitrou): Antoine proposes to skip the zlib "big buffer" (1 GB) test on 32 bits system. What do you think? On 64 bits system, we check a buffer of 2 GB-1 byte (0x7FFFFFFF bytes). Is the test useful or not? What do we test? Can you check if the test crashs on Mac OS X on a 32 bits system (1 GB buffer) if you disable F_FULLFSYNC in mmapmodule.c? Same question on a 64 bits system (2 GB-1 byte buffer)? The most important test if to test crc32 & adler32 with a buffer bigger than 4 GB, but we cannot write such test in Python 2.7 because the zlib module stores buffer sizes into int variables. So the "big buffer" test of Python 2.7 test_zlib is maybe just useful (on 32 and 64 bits). Can we just remove the test? |
@Haypo: trouble, trouble on the dev-list, as i've seen currently. This is all s**t. These are mmap(2) related issues and should be
We do test that mmap.mmap materializes a buffer which can be
Aeh - F_FULLFSYNC was not yet committed at that time in 2.7.
If i - choke! - need to write tests, i try to catch corner cases. Can we remove it? I would keep it, Apple is preparing the next |
In fact i like my idea of using iterations.
|
haypo> Can we just remove the test? I think so. The test was originally intended to catch the case where crc32() or sdaoden> Can we remove it? I would keep it, Apple is preparing the next I initially thought the same thing, but it turns out that the OS X sparsefile sdaoden> Unfortunately we cannot test 0x80000000+ no more because That wouldn't accomplish the same thing. The point of the test is to pick up |
On Fri, 6 May 2011 02:54:07 +0200, Nadeem Vawda wrote:
So i followed your suggestion and did not do something on zlib no
May the juice be with you |
Thanks for the tests; I'll review and commit them tomorrow morning.
Bear in mind that the test is only to be removed from 2.7; it will still @Haypo, @pitrou: Are there any objections to removing test_big_buffer() |
I now agree Antoine: the test is useless. It can be removed today. About mmap: add a new test for this issue (mmap on Mac OS X and F_FULLSYNC) is a good idea. I suppose that we will need to backport the F_FULLSYNC fix too. |
New changeset 201dcfc56e86 by Nadeem Vawda in branch '2.7': |
New changeset d5d4f2967879 by Nadeem Vawda in branch '3.1': New changeset e447a68742e7 by Nadeem Vawda in branch '3.2': New changeset bc13badf10a1 by Nadeem Vawda in branch 'default': |
New changeset 8d27d2b22394 by Nadeem Vawda in branch '2.7': |
@nadeem: note that the committed versions of the tests would not
|
(Of course this may also be intentional, say. |
New changeset 9b9f0de19684 by Nadeem Vawda in branch '2.7': New changeset b112c72f8c01 by Nadeem Vawda in branch '3.1': New changeset a9da17fcb564 by Nadeem Vawda in branch '3.2': New changeset b3a94906c4a0 by Nadeem Vawda in branch 'default': |
sdaoden> @nadeem: note that the committed versions of the tests would not Thanks for catching that. Should be fixed now. haypo> I now agree Antoine: the test is useless. It can be removed today. Done and done. haypo> I suppose that we will need to backport the F_FULLSYNC fix too. It has already been backported, as changeset 618c3e971e80. |
Aehm, note that Apple has fixed the mmap(2) bug!! Maybe Ned Deily should have a look at the version check, which |
Thanks for the update. Since the fix will be in a future version of OS X 10.7 Lion, and which has not been released yet, so it is not appropriate to change mmap until there has been an opportunity to test it. But even then, we would need to be careful about adding a compile-time test as OS X binaries are often built to be compatible for a range of operating system version so avoid adding compilation conditionals unless really necessary. If after 10.7 is released and someone is able to test that it works as expected, the standard way to support it would be to use the Apple-supplied availability macros to test for the minimum supported OS level of the build assuming it makes enough of a performance difference to bother to do so: http://developer.apple.com/library/mac/#technotes/tn2064/_index.html (Modules/_ctypes/darwin/dlfcn_simple.c is one of the few that has this kind of test.) |
@ Ned Deily <report@bugs.python.org> wrote (2011-06-07 19:43+0200):
It's really working fine. That i see that day!
Of course it only moves the delay from before mmap(2) to after kern.version = Darwin Kernel Version 10.7.0: Sat Jan 29 15:17:16 PST 2011 This is obviously not the right one. :)Ciao, Steffen |
Yes, you should check the Mac OS X version at runtime (as you should check the Linux kernel at runtime). platform.mac_ver() uses something like: sysv = _gestalt.gestalt('sysv')
if sysv:
major = (sysv & 0xFF00) >> 8
minor = (sysv & 0x00F0) >> 4
patch = (sysv & 0x000F) Note: patch is not reliable with 'sysv', you have to use ('sys1','sys2','sys3'). So if you would like to check that you have Mac OS 10.7 or later, you can do something like: sysv = _gestalt.gestalt('sysv')
__MAC_10_7 = (sysv and (sysv >> 4) >= 0x0a7) In C, it should be something like: I'm not sure of 0x73797376, I used hex(struct.unpack('!I', 'sysv')[0]). |
Victor, please do not use magic constants like that in C. The symbolic values are available in include files: #include <CoreServices/CoreServices.h>
SInt32 major = 0;
SInt32 minor = 0;
Gestalt(gestaltSystemVersionMajor, &major);
Gestalt(gestaltSystemVersionMinor, &minor);
if ((major == 10 && minor >= 7) || major >= 11) { ... } (See, for instance, http://www.cocoadev.com/index.pl?DeterminingOSVersion and http://stackoverflow.com/questions/2115373/os-version-checking-in-cocoa. The code in platform and _gestalt.c could stand to be updated at some point.) But, again, mmap should *not* be changed until 10.7 has been released and the Apple fix is verified and only if it makes sense to add the additional complexity. |
Ok, this patch could be used.
platform.py: _mac_ver_xml() should be dropped entirely according By the way: where do you get the info from? "sys1", "sys2", Note that i've mailed Apple. I did not pay 99$ or even 249$, so
|
Steffen: _mac_ver_xml should not be dropped, it is a perfectly fine way to determine the system version. Discussing it is also off-topic for this issue, please keep the discussion focussed. Wrt. mailing Apple: I wouldn't expect and answer. Is there something specific you want to know? I'm currently at WWDC and might be able to ask the question at one of the labs (where Apple's engineers hang out). If it is really necessary to check for the OS version to enable the OSX-specific bugfix it is possible to look at the uname information instead of using gestalt. In particular something simular to this Python code: v = os.uname()[2]
major = int(v.split('.')[0])
if major <= 10:
# We're on OSX 10.6 or earlier
enableWorkaround() This tests the kernel version instead of the system version, but until now the major version of the kernel has increased with every major release of the OS and I have no reason to suspect that Lion will be any different. BTW2: OSX 10.7 is not released yet and should not be discussed in public fora, as you should know if you have legal access. |
@ Ronald Oussoren wrote:
(You sound as if you participate in an interesting audiophonic
|
steffen: I have no idea what you are trying to say in your last message. Could you please try to stay on topic. |
So sorry that i'm stressing this, hopefully it's the final message.
I.e. the new patch uses >10.7.0 or >=10.6.8 to avoid that
P.S.: i still have no idea how to do '-framework CoreServices'
|
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: