Ticket #2 (assigned)
ogg.vorbis.VorbisFile() destructor kills python (redundant fclose() -> double free())
| Priority: | normal | Reporter: | Greg Ward <gward-pyogg@python.net> |
|---|---|---|---|
| Severity: | blocker | Assigned to: | soyt (accepted) |
| Component: | legacy-vorbis | Status: | assigned |
| Version: | 1.0 | Resolution: | |
| Milestone: | Keywords: |
Description by Greg Ward <gward-pyogg@python.net>:
Using pyogg-1.3 and pyvorbis-1.4 causes Python to die horribly:
$ python
Python 2.4.4c1 (#2, Oct 12 2006, 21:41:47)
[GCC 4.1.2 20061007 (prerelease) (Debian 4.1.1-16)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import ogg.vorbis
>>> ogg.vorbis.VorbisFile("/music/ogg/metallica/master_of_puppets/01.battery.ogg")
<VorbisFile object at 0xb7db4548>
>>> ^D
*** glibc detected *** double free or corruption (!prev): 0x081b3a80 ***
zsh: abort python
This is on a Debian 'unstable' system running Linux 2.6.18 and glibc 2.3.6. I get the same result with Debian's current python2.3 package (which is 2.3.5).
Needless to say, this makes pyvorbis 1.4 unusable.
Attachments
Changelog
Sun Oct 22 18:14:43 2006: Modified by Greg Ward <gerg.ward+pyogg@gmail.com>
- attachment added: pyogg-ticket2-test.patch
Sun Oct 22 18:16:01 2006: Modified by Greg Ward <gerg.ward+pyogg@gmail.com>
Sun Oct 22 20:08:40 2006: Modified by Greg Ward <gerg.ward+pyogg@gmail.com>
OK, I figured it out. The problem is that `py_ov_file_dealloc()` calls `fclose(py_self->c_file)`, but the libvorbis docs say not to:
Also, you should be aware that ov_open(), once successful, takes complete possession of the file resource. After you have opened a file using ov_open(), you MUST close it using ov_clear(), not fclose() or any other function.
(See http://xiph.org/vorbis/doc/vorbisfile/ov_open.html. This is brain damage on the part of libvorbis, but at least it's documented brain damage.)
While auditing the code, I discovered what look like three other memory-related problems. While I am no expert at writing Python extensions, I'm positive that one of these (#3) is a real problem (because I could reproduce it), and fairly sure that the other two are as well.
I'll attach a patch that addresses all four problems:
1. `py_file_new()` incorrectly calls `Py_DECREF` on the returned object. From http://docs.python.org/ext/ownershipRules.html :
The object reference returned from a C function that is called from Python must be an owned reference -- ownership is tranferred from the function to its caller.
2. the redundant `fclose()` call
3. while `py_ov_file_open()` mallocs an OggVorbis_File struct, it is never freed; I've added a `free()` call in `py_ov_file_dealloc()`. The leak is trivial to demonstrate with this script:
from ogg._vorbis import VorbisFile
from ogg.test import common
filename = common.getSampleFilename()
while True:
VorbisFile(filename)
4. The `py_vorbisfile` object is created with PyObject_NEW() but destroyed with PyMem_DEL(). This is wrong; you're not supposed to mix and match memory allocation layers like this. The fix is to use PyObject_DEL().
Sun Oct 22 20:09:48 2006: Modified by Greg Ward <gerg.ward+pyogg@gmail.com>
- attachment added: pyogg-ticket2-fix.patch
Sun Oct 22 20:10:34 2006: Modified by Greg Ward <gerg.ward+pyogg@gmail.com>
- summary changed from ogg.vorbis.VorbisFile() constructor kills python to ogg.vorbis.VorbisFile() destructor kills python (redundant fclose() -> double free())
Mon Oct 23 08:57:12 2006: Modified by soyt
- status changed from new to assigned
Thanks a lot for looking into this. This is very much appreciated. I'll have a look as soon as I have some time, and I'll probably make a new legacy release. The main reason I wanted to go for a rewrite is the amount of C code to maintain in the current code.
Bug is present in latest svn code: I just added a patch to verify this, by adding ogg/test/test_legacy.py.