classification
Title: xml.parsers.expat ParseFile() causes segmentation fault when passed a closed file object
Type: crash Stage: resolved
Components: XML Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: BreamoreBoy, ezio.melotti, loewis, louiscipher, python-dev, santoso.wijaya, showard, terry.reedy
Priority: normal Keywords: patch

Created on 2009-01-08 05:01 by showard, last changed 2011-04-11 00:56 by ezio.melotti. This issue is now closed.

Files
File name Uploaded Description Edit
pyexpat_segfault_on_closed_file.patch showard, 2009-01-08 05:01
issue4877.patch santoso.wijaya, 2011-04-08 17:40 review
issue4877.patch louiscipher, 2011-04-08 22:36
issue4877-2.patch ezio.melotti, 2011-04-09 01:29 review
Messages (13)
msg79398 - (view) Author: Steve Howard (showard) Date: 2009-01-08 05:01
In Python 2.5.4 built from unmodified source:

showard@showardlt:~/src/Python-2.5.4$ ./python
Python 2.5.4 (r254:67916, Jan  7 2009, 20:28:41) 
[GCC 4.2.4 (Ubuntu 4.2.4-1ubuntu3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from xml.parsers import expat
>>> f=open('/tmp/foo')
>>> p=expat.ParserCreate()
>>> f.close()
>>> p.ParseFile(f)
Segmentation fault

The error is in the control flow in xmlparse_ParseFile()
(Modules/pyexpat.c:1000).  When passed a real file object that's been
closed, PyFile_Check() returns true, but then PyFile_AsFile() returns 0
(since f_fp on the file object is set to zero when the file is closed).
 So the local 'fp' is set to 0, and 'readmethod' is left as NULL.  The
conditional at 1033 then fails, and the call to readinst() at 1041
passes readmethod=NULL, leading eventually to a segfault in
PyObject_Call at Objects/abstract.c:1860.

I think it's present in 2.6 as well, but I'm not sure.  It seems to have
been fixed by chance in 3.0 because Guido removed the first branch in
xmlparse_ParseFile altogether in an unrelated change a while ago.

The attached patch simply checks for fp == 0 and raises an exception.  I
don't know if it's the proper solution but you get the idea.

Built with the attached patch:

showard@showardlt:~/src/Python-2.5.4$ ./python
Python 2.5.4 (r254:67916, Jan  7 2009, 20:28:41) 
[GCC 4.2.4 (Ubuntu 4.2.4-1ubuntu3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from xml.parsers import expat
>>> f=open('/tmp/foo')
>>> p=expat.ParserCreate()
>>> f.close()   
>>> p.ParseFile(f)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: attempting to parse closed file
msg109610 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-08 21:21
It seems that this is ok in py3k in which case it could be backported to 2.7, or has this already been done?
msg133274 - (view) Author: Bryce Verdier (louiscipher) Date: 2011-04-07 23:00
I double checked this today. On my linux box with 2.6.6 the commands given did cause a segfault. On my windows VM with 2.7.1 it also created a segfault. And to back up Mark's claim, it did not segfault on my 3.1.2 linux install.
msg133332 - (view) Author: Santoso Wijaya (santoso.wijaya) * Date: 2011-04-08 17:40
I concur. Attaching a refreshed patch (against current 2.7 branch) along with a unittest.
msg133348 - (view) Author: Bryce Verdier (louiscipher) Date: 2011-04-08 22:36
Modified patch to remove PyErr_SetString() as it doesn't do anything. Patch looks good to me otherwise and ran without issue.
msg133349 - (view) Author: Santoso Wijaya (santoso.wijaya) * Date: 2011-04-08 22:56
You mean, PyErr_Clear()?
msg133350 - (view) Author: Bryce Verdier (louiscipher) Date: 2011-04-08 23:00
Yes I did. Sorry.
msg133363 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-04-09 00:47
Elsewhere, Guido said that this appears *not* to be a security bug since the crash is "triggered by a logic bug in the user's code, not by bad data." Hence, not eligible for backport to 2.5/6, which are in security-fix only mode.
msg133368 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-09 01:04
See also #9292.
msg133369 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-09 01:17
FWIW I tried to backport 825041fc8e2c and test_pyexpat runs without problem.
The snippet of code in msg79398 now raises a ValueError:
>>> from xml.parsers import expat
>>> f=open('/tmp/foo')
>>> p=expat.ParserCreate()
>>> f.close()
>>> p.ParseFile(f)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: I/O operation on closed file
msg133370 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-09 01:29
The attached patch combines the changes done to pyexpat in 825041fc8e2c (http://hg.python.org/cpython/diff/825041fc8e2c/Modules/pyexpat.c), the cleanup of c52f5df50448 and the tests of issue4877.patch.
The tests segfault without the changes on pyexpat, and pass once they are applied.
msg133491 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-04-11 00:45
New changeset 28705a7987c5 by Ezio Melotti in branch '2.7':
#4877: Fix a segfault in xml.parsers.expat while attempting to parse a closed file.
http://hg.python.org/cpython/rev/28705a7987c5
msg133492 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-11 00:56
This is now fixed in 2.7, I also removed the unnecessary call to PyErr_Clear in ba699cf9bdbb (2.7), 6b4467e71872 (3.2), and 2d1d9759d3a4 (3.3).
History
Date User Action Args
2011-04-11 00:56:57ezio.melottisetstatus: open -> closed
messages: + msg133492

assignee: ezio.melotti
resolution: fixed
stage: commit review -> resolved
2011-04-11 00:45:56python-devsetnosy: + python-dev
messages: + msg133491
2011-04-09 01:29:09ezio.melottisetfiles: + issue4877-2.patch

messages: + msg133370
2011-04-09 01:17:30ezio.melottisetmessages: + msg133369
2011-04-09 01:04:24ezio.melottisetnosy: + ezio.melotti
messages: + msg133368
2011-04-09 00:47:53terry.reedysetnosy: + terry.reedy

messages: + msg133363
stage: commit review
2011-04-08 23:00:26louisciphersetmessages: + msg133350
2011-04-08 22:56:50santoso.wijayasetmessages: + msg133349
2011-04-08 22:36:47louisciphersetfiles: + issue4877.patch

messages: + msg133348
2011-04-08 17:40:10santoso.wijayasetfiles: + issue4877.patch
nosy: + santoso.wijaya
messages: + msg133332

2011-04-07 23:00:40louisciphersetnosy: + louiscipher
messages: + msg133274
2010-07-08 21:21:24BreamoreBoysetnosy: + loewis, BreamoreBoy

messages: + msg109610
versions: + Python 2.7, - Python 2.5
2009-01-08 05:01:13showardcreate