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.

Sign up for GitHub

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

improve error handling robustness for os.execvpe #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

improve error handling robustness for os.execvpe #47

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions ptyprocess/ptyprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,19 @@ def spawn(
if not isinstance(argv, (list, tuple)):
raise TypeError("Expected a list or tuple for argv, got %r" % argv)

# env vars must be utf-8 encoded strings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always UTF-8? Or should we be trying to apply a locale-dependent encoding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection, unless this comment makes you go "aha, yes, I know just how we should do this!", let's stick to UTF-8 encoding and see if anyone complains. It might not be 100% right, but it's easy to understand, and people who really need something else can still pass environment variables as bytes.

# https://github.com/pexpect/pexpect/issues/512
#
# from https://docs.python.org/3.8/library/os.html#os.execvpe
# Errors will be reported as OSError exceptions.
if isinstance(env, dict):
_text_type = str
if not PY3:
_text_type = unicode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this compatibility bit in the existing if PY3: block at the top of the file.

for k, v in env.items():
if isinstance(v, _text_type):
env[k] = v.encode('utf-8')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC modifying a dict while iterating over it is generally considered a bad idea. Maybe it's better to create a new dict env_b instead.


# Shallow copy of argv so we can modify it
argv = argv[:]
command = argv[0]
Expand Down Expand Up @@ -289,10 +302,14 @@ def spawn(
os.execv(command, argv)
else:
os.execvpe(command, argv, env)
except OSError as err:
except Exception as err:
# [issue #119] 5. If exec fails, the child writes the error
# code back to the parent using the pipe, then exits.
tosend = 'OSError:{}:{}'.format(err.errno, str(err))
if isinstance(err, OSError):
tosend = 'OSError:{}:{}'.format(err.errno, str(err))
else:
cls_name = err.__class__.__name__
tosend = 'Exception:0:{}: {}'.format(cls_name, str(err))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the exception handling around preexec_fn(), maybe this should do '{}:0:{}' (e.g. 'UnicodeEncodeError:0:blah'.

if PY3:
tosend = tosend.encode('utf-8')
os.write(exec_err_pipe_write, tosend)
Expand Down