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

spawnl crash on windows7 #72918

Closed
srinim mannequin opened this issue Nov 18, 2016 · 13 comments
Closed

spawnl crash on windows7 #72918

srinim mannequin opened this issue Nov 18, 2016 · 13 comments
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-windows type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@srinim
Copy link
Mannequin

srinim mannequin commented Nov 18, 2016

BPO 28732
Nosy @pfmoore, @vstinner, @tjguk, @zware, @eryksun, @zooba

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/zooba'
closed_at = <Date 2016-11-20.05:40:20.192>
created_at = <Date 2016-11-18.11:22:35.319>
labels = ['interpreter-core', '3.7', 'OS-windows', 'type-crash']
title = 'spawnl crash on windows7'
updated_at = <Date 2016-11-20.05:40:20.190>
user = 'https://bugs.python.org/srinim'

bugs.python.org fields:

activity = <Date 2016-11-20.05:40:20.190>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2016-11-20.05:40:20.192>
closer = 'steve.dower'
components = ['Interpreter Core', 'Windows']
creation = <Date 2016-11-18.11:22:35.319>
creator = 'srinim'
dependencies = []
files = []
hgrepos = []
issue_num = 28732
keywords = []
message_count = 13.0
messages = ['281095', '281096', '281097', '281111', '281116', '281181', '281239', '281240', '281241', '281242', '281243', '281246', '281248']
nosy_count = 8.0
nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower', 'srinim']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue28732'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

@srinim
Copy link
Mannequin Author

srinim mannequin commented Nov 18, 2016

in window7 (using python 3.4.3,3.5.2) following script crashes

import os
os.spawnl( os.P_NOWAIT, 'C:/Tcl/bin/tclsh.exe' )"

Note: similar issue is already exist https://bugs.python.org/issue8036

@srinim srinim mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Nov 18, 2016
@srinim
Copy link
Mannequin Author

srinim mannequin commented Nov 18, 2016

small correction (removed " )

import os
os.spawnl( os.P_NOWAIT, 'C:/Tcl/bin/tclsh.exe' )

@vstinner
Copy link
Member

Just to make sure: a crash means that the Python process is killed and you get a popup or something like that?

Can you please try to run the script on Python 3.6 using:

python.exe -X faulthandler script.py

It should display the Windows error code at least.

@eryksun
Copy link
Contributor

eryksun commented Nov 18, 2016

spawnl is implemented by calling _spawnv 1, which is documented to invoke the invalid parameter handler if argv points to a NULL pointer. It wants the program name, or whatever, as long as argv[0] isn't NULL or an empty string, e.g. os.spawnl(os.P_NOWAIT, 'C:/Tcl/bin/tclsh.exe', 'tclsh').

The invalid parameter handler should be disabled (_Py_BEGIN_SUPPRESS_IPH) when calling _spawnv[e], which in this case would lead to an EINVAL OSError instead of crashing the process.

For example:

    import os, sys
    from ctypes import *

    ucrt = CDLL('ucrtbase')

    @CFUNCTYPE(None, c_wchar_p, c_wchar_p, c_wchar_p, c_int, c_void_p)
    def iph(*args):
        pass

    ucrt._set_thread_local_invalid_parameter_handler(iph)
    >>> os.spawnl(os.P_NOWAIT, sys.executable)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "C:\Program Files\Python35\lib\os.py", line 977, in spawnl
        return spawnv(mode, file, args)
    OSError: [Errno 22] Invalid argument

Also, in this case a descriptive ValueError would be friendlier, given an empty argv or argv[0] that's an empty string -- at least on Windows.

@vstinner
Copy link
Member

I like the idea of using _Py_BEGIN_SUPPRESS_IPH, but also the idea of
implementing most obvious checks on arguments. Sadly, I don't have
access to Windows yet to write such patch. Eryk, if you write such
patch, I would be happy to review it ;-) Especially if it includes
unit tests :-D

@zooba
Copy link
Member

zooba commented Nov 18, 2016

Looks like a few functions in os module need this. os_execve_impl also doesn't release the GIL at any point, but I don't see why it shouldn't.

I'll try and get to this over the weekend if nobody else comes up with a patch first.

@zooba zooba added the 3.7 (EOL) end of life label Nov 20, 2016
@zooba zooba self-assigned this Nov 20, 2016
@python-dev
Copy link
Mannequin

python-dev mannequin commented Nov 20, 2016

New changeset 02f416441def by Steve Dower in branch '3.5':
Issue bpo-28732: Fix crash in os.spawnv() with no elements in args
https://hg.python.org/cpython/rev/02f416441def

@eryksun
Copy link
Contributor

eryksun commented Nov 20, 2016

A ValueError should also be raised when argv[0] is an empty string.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Nov 20, 2016

New changeset 1a9e4b465497 by Steve Dower in branch '3.6':
Issue bpo-28732: Raise ValueError when os.spawn*() is passed an empty tuple of arguments
https://hg.python.org/cpython/rev/1a9e4b465497

New changeset 75824899f0dd by Steve Dower in branch 'default':
Issue bpo-28732: Raise ValueError when os.spawn*() is passed an empty tuple of arguments
https://hg.python.org/cpython/rev/75824899f0dd

@python-dev
Copy link
Mannequin

python-dev mannequin commented Nov 20, 2016

New changeset e076ace7b0ff by Steve Dower in branch '3.5':
Issue bpo-28732: Raise ValueError when argv[0] is empty.
https://hg.python.org/cpython/rev/e076ace7b0ff

New changeset af78b33704af by Steve Dower in branch '3.6':
Issue bpo-28732: Raise ValueError when argv[0] is empty
https://hg.python.org/cpython/rev/af78b33704af

New changeset fc6f757e53de by Steve Dower in branch 'default':
Issue bpo-28732: Raise ValueError when argv[0] is empty
https://hg.python.org/cpython/rev/fc6f757e53de

@zooba
Copy link
Member

zooba commented Nov 20, 2016

ValueError should also be raised when argv[0] is an empty string.

Added that too.

Python 3.5 is missing the tests for these functions completely, so I only added those to 3.6 and later. Also the original issue was already resolved in 3.6, but I tidied up a few other functions that were missing proper handling.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Nov 20, 2016

New changeset 2e1fb851dfb4 by Steve Dower in branch '3.6':
Issue bpo-28732: Adds new errors to spawnv emulation for platforms that only have fork and execv
https://hg.python.org/cpython/rev/2e1fb851dfb4

New changeset ac6de11fbd50 by Steve Dower in branch 'default':
Issue bpo-28732: Adds new errors to spawnv emulation for platforms that only have fork and execv
https://hg.python.org/cpython/rev/ac6de11fbd50

@zooba
Copy link
Member

zooba commented Nov 20, 2016

Buildbots seem to be happy now so I'm closing the issue.

Feel free to reopen if anyone spots anything in commit review.

@zooba zooba closed this as completed Nov 20, 2016
@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
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-windows type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

No branches or pull requests

3 participants