Skip to content
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

Jump to bottom

xml.parsers.expat ParseFile() causes segmentation fault when passed a closed file object #49127

Closed
showard mannequin opened this issue Jan 8, 2009 · 13 comments
Closed

xml.parsers.expat ParseFile() causes segmentation fault when passed a closed file object #49127

showard mannequin opened this issue Jan 8, 2009 · 13 comments
Assignees
@ezio-melotti
Labels
topic-XML type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@showard
Copy link
Mannequin

showard mannequin commented Jan 8, 2009

BPO 4877
Nosy @loewis, @terryjreedy, @ezio-melotti
Files
  • pyexpat_segfault_on_closed_file.patch
  • issue4877.patch
  • issue4877.patch
  • issue4877-2.patch
    		•

    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/ezio-melotti'
closed_at = <Date 2011-04-11.00:56:57.625>
created_at = <Date 2009-01-08.05:01:13.888>
labels = ['expert-XML', 'type-crash']
title = 'xml.parsers.expat ParseFile() causes segmentation fault when passed a closed file object'
updated_at = <Date 2011-04-11.00:56:57.624>
user = 'https://bugs.python.org/showard'

    bugs.python.org fields:

    activity = <Date 2011-04-11.00:56:57.624>
actor = 'ezio.melotti'
assignee = 'ezio.melotti'
closed = True
closed_date = <Date 2011-04-11.00:56:57.625>
closer = 'ezio.melotti'
components = ['XML']
creation = <Date 2009-01-08.05:01:13.888>
creator = 'showard'
dependencies = []
files = ['12646', '21587', '21589', '21590']
hgrepos = []
issue_num = 4877
keywords = ['patch']
message_count = 13.0
messages = ['79398', '109610', '133274', '133332', '133348', '133349', '133350', '133363', '133368', '133369', '133370', '133491', '133492']
nosy_count = 8.0
nosy_names = ['loewis', 'terry.reedy', 'ezio.melotti', 'showard', 'santoso.wijaya', 'BreamoreBoy', 'louiscipher', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue4877'
versions = ['Python 2.7']
    @showard
    Copy link
    Mannequin Author

    showard mannequin commented Jan 8, 2009

    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

    @showard showard mannequin added topic-XML type-crash A hard crash of the interpreter, possibly with a core dump labels Jan 8, 2009
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 8, 2010

    It seems that this is ok in py3k in which case it could be backported to 2.7, or has this already been done?

    @louiscipher
    Copy link
    Mannequin

    louiscipher mannequin commented Apr 7, 2011

    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.

    @santosowijaya
    Copy link
    Mannequin

    santosowijaya mannequin commented Apr 8, 2011

    I concur. Attaching a refreshed patch (against current 2.7 branch) along with a unittest.

    @louiscipher
    Copy link
    Mannequin

    louiscipher mannequin commented Apr 8, 2011

    Modified patch to remove PyErr_SetString() as it doesn't do anything. Patch looks good to me otherwise and ran without issue.

    @santosowijaya
    Copy link
    Mannequin

    santosowijaya mannequin commented Apr 8, 2011

    You mean, PyErr_Clear()?

    @louiscipher
    Copy link
    Mannequin

    louiscipher mannequin commented Apr 8, 2011

    Yes I did. Sorry.

    @terryjreedy
    Copy link
    Member

    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.

    @ezio-melotti
    Copy link
    Member

    See also bpo-9292.

    @ezio-melotti
    Copy link
    Member 

    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

    @ezio-melotti
    Copy link
    Member

    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 bpo-4877.patch.
    The tests segfault without the changes on pyexpat, and pass once they are applied.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 11, 2011

    New changeset 28705a7987c5 by Ezio Melotti in branch '2.7':
    bpo-4877: Fix a segfault in xml.parsers.expat while attempting to parse a closed file.
    http://hg.python.org/cpython/rev/28705a7987c5

    @ezio-melotti
    Copy link
    Member

    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).

    @ezio-melotti ezio-melotti self-assigned this Apr 11, 2011
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @ezio-melotti ezio-melotti

    Labels
    topic-XML type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @terryjreedy @ezio-melotti