Issue585792
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2002-07-24 08:52 by phr, last changed 2022-04-10 16:05 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
mmap.patch | nnorwitz, 2002-08-12 15:47 | mmap patch 3 |
Messages (18) | |||
---|---|---|---|
msg11663 - (view) | Author: paul rubin (phr) | Date: 2002-07-24 08:52 | |
This is under Red Hat 7.2, Linux kernel 2.4.7. "x" is an empty file, i.e. zero bytes long. import mmap f = open(x,"r+")m = mmap.mmap(f.fileno(), 10) print m[1] Crashes Python with a bus error. Python should catch the bus error signal and raise an appropriate exception that the script can catch. |
|||
msg11664 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-07-28 15:00 | |
Logged In: YES user_id=33168 Attached is a patch which fixes this problem. I'm not sure this patch it correct nor appropriate. But it stops the crash. I'm hoping some mmap expert can shed some light on this. |
|||
msg11665 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-11 15:05 | |
Logged In: YES user_id=33168 Martin, could you please review the patch? |
|||
msg11666 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2002-08-12 07:15 | |
Logged In: YES user_id=21627 Ah, an old-style diff. I assume that goes into mmap_item? I'd rather avoid the stat call on every access, and check the size on map creation. The Solaris man page says this about changing files: If the size of the mapped file changes after the call to mmap() as a result of some other operation on the mapped file, the effect of references to portions of the mapped region that correspond to added or removed portions of the file is unspecified. So it appears that this is a case of "don"t do this, then", and that it atleast won't crash - but we might want to perform a few experiments. |
|||
msg11667 - (view) | Author: Nobody/Anonymous (nobody) | Date: 2002-08-12 07:45 | |
Logged In: NO Please don't even think of adding a stat call to every access. The whole idea of mmap is to speed up access by eliminating system calls when you touch the file contents. Checking the file size at map creation is reasonable. Some note should also be added to the mmap doc about possible crashes if the file size changes. From a Python script writer's viewpoint, the most convenient fix is for Python to catch the bus error signal and inspect the stack to figure out that the trap occurred in the mmap routine, then generate and return an appropriate Python exception. However, doing that is pretty platform-dependent, and attempting it in all the Python ports that support mmap doesn't sound practical. |
|||
msg11668 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-12 13:30 | |
Logged In: YES user_id=33168 I wasn't sure if I could change the size. For Solaris at least, it seems this is the right thing to do. Is it likely that other systems allow the file size to change? I've forced the size to be the max(size_arg, size_of_file). This has the added benefit of working for writing too. Prior to this patch, writing past the end crashed on Linux. There are 2 comments in the patch: one worrying about the file changing, the other producing a warning message if the size argument is larger than the file. Do we have to worry about the size changing? Should we tell the user what they did? Sorry about the old style diff. I remembered to do the cvs -f diff part, but forgot the -C5. My defaults are unified. New context patch attached. |
|||
msg11669 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2002-08-12 14:45 | |
Logged In: YES user_id=21627 I'd say it is a ValueError if the map size is larger than the file size, instead of silently decreasing the size: Errors should never pass silently, Unless explicitly silenced. On the trick of new_buffersize: I'm not sure I understand what you are referring to: we don't use stdio in mmap at all. |
|||
msg11670 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-12 15:47 | |
Logged In: YES user_id=33168 I thought the st_size was updated by the code in new_buffersize. It was only the position, so the comment should go. ValueError is fine with me. Should I add an entry to NEWS? New patch attached w/NEWS and test. There is another issue--Windows. I how Windows behaves and I can't test it. |
|||
msg11671 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-12 15:49 | |
Logged In: YES user_id=33168 That last comment was supposed to be: I don't know how Windows behaves... |
|||
msg11672 - (view) | Author: Tim Peters (tim.peters) * | Date: 2002-08-12 15:59 | |
Logged In: YES user_id=31435 mmap() on WIndows doesn't care if you map more bytes than exist in the file -- if you do, it grows the file to match. """ If an application specifies a size for the file mapping object that is larger than the size of the actual named file on disk, the file on disk is grown to match the specified size of the file mapping object. If the file cannot be grown, this results in a failure to create the file mapping object. GetLastError will return ERROR_DISK_FULL. """ Like so: >>> import mmap >>> import os >>> os.path.getsize('abc.abc') 12L >>> f = file('abc.abc', 'r+') >>> m = mmap.mmap(f.fileno(), 10000) >>> m.close() >>> os.path.getsize('abc.abc') 10000L >>> |
|||
msg11673 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2002-08-12 16:31 | |
Logged In: YES user_id=21627 I think we should special-case Windows, then. For the mmapmodule patch proper, nothing needs to be done; for the testsuite test, pass and fail are exactly reversed on Windows (but then, we'd test the implementation of Windows, so skipping that test sounds reasonable as well). With those changes, the patch is fine. |
|||
msg11674 - (view) | Author: Nobody/Anonymous (nobody) | Date: 2002-08-13 02:11 | |
Logged In: NO I'd say the following is reasonable: - on mmap object creation, stat the file and check its size against the size arg passed to mmap. Signal an error if the file is smaller than the requested mmap size. - On access, compare the subscript to the mmap object size and signal an error if out of bounds (presumably this is done already) - Figure out if another process can truncate the file while the mmap is alive. If yes, maybe add a method to the mmap object that re-stats the file and makes sure it's still larger than the mmap size. - Add warnings to the mmap documentation that weirdness can result from the file size changing etc. |
|||
msg11675 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-13 17:39 | |
Logged In: YES user_id=33168 I can't test windows. I've attached a patch which tries to test windows doesn't raise an exception. I also modified the NEWS to state this change affects non-Windows platforms. Tim/Martin, can you test (or at least review the test code) that this works on Windows before I check it in? Or let me know if you want me to check it in. It should be easy to fix if it doesn't work on Windows. Thanks. |
|||
msg11676 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-17 16:08 | |
Logged In: YES user_id=33168 Martin, would you like one last review and test on windows before I check in? Also, should this be backported to 2.2.? |
|||
msg11677 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2002-08-17 17:09 | |
Logged In: YES user_id=21627 I may be missing something: How precisely does this special-case Windows in the test? Apart from that, the patch is fine. I'm uncertain on backporting it, because I'm uncertain how backporting works at all since the PBF got instantiated. So I'd suggest to mark this as a backport candidate, and let the patch czar decide. |
|||
msg11678 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-09-05 22:33 | |
Logged In: YES user_id=33168 Sorry Martin, I apparently didn't upload the last patch. Feel free to fix the test if you see a problem. Checked in as: Modules/mmapmodule.c: 2.41, 2.35.6.3 Lib/test/test_mmap.py: 1.24, 1.19.8.3 Lib/test/output/test_mmap: 1.9, 1.8.6.1 Misc/NEWS: 1.480, 1.337.2.4.2.34 I backported the fix, since your last message Guido seemed to indicate that we are still supposed to do it. |
|||
msg11679 - (view) | Author: Alastair Tse (liquidx) (liqx) | Date: 2003-04-09 23:23 | |
Logged In: YES user_id=338973 i'm having some problems with this patch. how does one use mmap on a device file with on a unix device file? # example - with a webcam on /dev/video0: import os, mmap fd = os.open("/dev/video0",os.O_RDONLY) mm = mmap.mmap(fd, 1024, mmap.MAP_SHARED) # this will result in: Traceback (most recent call last): File "<stdin>", line 1, in ? ValueError: mmap length is greater than file size even though this is a perfectly valid way to access the buffer for video devices on linux (not sure about other unicies) |
|||
msg11680 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2003-04-10 17:13 | |
Logged In: YES user_id=33168 The problem is that character devices should be excluded from the offset check. This fix is in patch 708374. That part of the patch should be backported to 2.2.3. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-10 16:05:31 | admin | set | github: 36920 |
2002-07-24 08:52:15 | phr | create |