classification
Title: PyStructSequence_New() doesn't validate its input type (crashes in os.wait3() and os.wait4() in case of a bad resource.struct_rusage)
Type: crash Stage: patch review
Components: Extension Modules Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2017-09-25 10:29 by Oren Milman, last changed 2017-09-25 14:03 by vstinner.

Pull Requests
URL Status Linked Edit
PR 3750 closed vstinner, 2017-09-25 14:02
Messages (2)
msg302945 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-09-25 10:58
The following code causes the interpreter to crash:
import os
import time
import resource
new_pid = os.fork()
if new_pid == 0:
    time.sleep(0.5)
else:
    resource.struct_rusage = None
    os.wait3(0)
    
We would get a crash also if we replaced 'os.wait3(0)' with 'os.wait4(new_pid, 0)'.

This is because wait_helper() (in in Modules/posixmodule.c) assumes that
resource.struct_rusage is a type object, and passes it to PyStructSequence_New(),
which tries to access the n_fields attribute, and crashes.


In addition, the following code causes a SystemError:
class BadStructRusage:
    n_fields = None

import os
import time
import resource
new_pid = os.fork()
if new_pid == 0:
    time.sleep(0.5)
else:
    resource.struct_rusage = BadStructRusage
    os.wait3(0)

This is because PyStructSequence_New() (in Objects/structseq.c) assumes that it
received a type with a valid n_fields attribute.


Similarly, the following code causes the interpreter to crash:
class BadStructRusage:
    n_fields = 16
    n_sequence_fields = None

import os
import time
import resource
new_pid = os.fork()
if new_pid == 0:
    time.sleep(0.5)
else:
    resource.struct_rusage = BadStructRusage
    os.wait3(0)


ISTM that we can fix these problems by adding checks to wait_helper() and to
PyStructSequence_New().
However, maybe a more simple solution would be to either:
    - Make wait_helper() always use StructRUsageType (defined in Modules/resource.c).
    - Disable assigning to resource.struct_rusage.

Moreover, I don't understand the comment before calling PyStructSequence_New():
/* XXX(nnorwitz): Copied (w/mods) from resource.c, there should be only one. */
Is it relevant to this issue?


Lastly, I am not sure about tests (as I found almost no tests of wait3() and
wait4()).
Should I add to Lib/test/test_wait3.py and Lib/test/test_wait4.py each a class
to test this issue? Or is it too much of a corner case, and a test is not needed?
msg302958 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-25 14:03
Attached PR 3750 tries to validate the input type. The problem is that structseq is not exactly a type. It's hard to really validate everything. See the comment test in my PR.
History
Date User Action Args
2017-09-25 14:03:54vstinnersettitle: PyStructSequence_New() doesn't validate its input type (crashes in os.wait3() and os.wait4() in case of a bad resource.struct_rusage -> PyStructSequence_New() doesn't validate its input type (crashes in os.wait3() and os.wait4() in case of a bad resource.struct_rusage)
2017-09-25 14:03:50vstinnersettitle: crashes in os.wait3() and os.wait4() in case of a bad resource.struct_rusage -> PyStructSequence_New() doesn't validate its input type (crashes in os.wait3() and os.wait4() in case of a bad resource.struct_rusage
2017-09-25 14:03:27vstinnersetnosy: + serhiy.storchaka
2017-09-25 14:03:18vstinnersetnosy: + vstinner
messages: + msg302958
2017-09-25 14:02:22vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request3737
2017-09-25 10:58:17Oren Milmansettitle: struct_rusage -> crashes in os.wait3() and os.wait4() in case of a bad resource.struct_rusage
versions: + Python 3.7
messages: + msg302945

components: + Extension Modules
type: crash
2017-09-25 10:29:33Oren Milmancreate