This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9059

procutil: assign stdio objects if they are None
AbandonedPublic

Authored by quark on Sep 18 2020, 8:07 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

On Python 3 stdio objects can be None. That causes crashes. Fix it by opening
devnull automatically.

stdin can be None by using 0<&- in bash, or spawning processes less
carefully, for example, watchman used to cause such None stdin [1] (note:
None is only observable on Python 3).

[1]: https://github.com/facebook/watchman/commit/d241978aaa6b6d7c5b7260bc9e6d699d3a1cea53

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

quark created this revision.Sep 18 2020, 8:07 PM
mjacob added a subscriber: mjacob.Sep 19 2020, 12:41 AM

I think it’s reasonable that Mercurial expects that the stdio streams are initialized properly. Is there a use case for running Mercurial with FDs 0, 1 or 2 set to non-standard streams?

mjacob added inline comments.Sep 19 2020, 1:12 AM
mercurial/utils/procutil.py
119–121

The created streams are replaced later by initstdio(), causing them to get garbage-collected without getting closed explicitly.

If you set environment variable PYTHONWARNINGS=always, you’ll get something like

/home/manu/vcs/hg/mercurial/dispatch.py:189: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='r' encoding='UTF-8'>
  sys.stdin = io.TextIOWrapper(
121

Is there a reason for not opening a dedicated stream for stderr?

indygreg added a subscriber: indygreg.Sep 22 2020, 10:21 PM

This patch breaks my brain because our mucking about with sys.std* is wonky. We are doing really hacky things in both procutil.py and dispatch.py, including reassigning sys.std* in dispatch.py before this patch. I'd strongly prefer to have that logic consolidated.

As for the Python warning, that's super annoying. But I'm not sure we can do anything about that since we don't have an obvious place to insert an __exit__ handler. This is untested, but the best I can come up with is:

if not sys.stdin:
    sys.stdin = open(os.devnull, 'r')
    atexit.register(sys.stdin.close)
quark updated this revision to Diff 23030.Oct 2 2020, 10:45 PM
quark added a comment.Oct 2 2020, 10:55 PM

Updated to remove the warning and open a separate stderr.

quark updated this revision to Diff 23031.Oct 2 2020, 11:17 PM

Updated so only dispatch.py patches sys.std* directly

yuja added a subscriber: yuja.Dec 18 2020, 9:55 PM

As Pulkit pointed out, I and Pulkit made another version in parallel.

https://patchwork.mercurial-scm.org/patch/47937/
https://patchwork.mercurial-scm.org/patch/47938/
https://patchwork.mercurial-scm.org/patch/47939/

sys.std* are left unmodified. procutil.std* are fixed up to raise EBADF.

marmoute requested changes to this revision.Jan 8 2021, 11:07 AM
marmoute added a subscriber: marmoute.

It looks like the alternative got in.

This revision now requires changes to proceed.Jan 8 2021, 11:07 AM
quark abandoned this revision.Jan 16 2021, 11:39 PM

Revision Contents
Changeset List

Diff 23031

mercurial/dispatch.py

# #
# But some parts of Mercurial (e.g. hooks) can still send data to sys.stdout # But some parts of Mercurial (e.g. hooks) can still send data to sys.stdout
# and sys.stderr. They will inherit the line ending normalization settings, # and sys.stderr. They will inherit the line ending normalization settings,
# potentially causing e.g. \r\n to be emitted. Since emitting \n should # potentially causing e.g. \r\n to be emitted. Since emitting \n should
# "just work," here we change the sys.* streams to disable line ending # "just work," here we change the sys.* streams to disable line ending
# normalization, ensuring compatibility with our ui type. # normalization, ensuring compatibility with our ui type.
# write_through is new in Python 3.7. # write_through is new in Python 3.7.
sys.stdout = sys.stdout or io.TextIOWrapper(io.BytesIO())
kwargs = { kwargs = {
"newline": "\n", "newline": "\n",
"line_buffering": sys.stdout.line_buffering, "line_buffering": sys.stdout.line_buffering,
} }
if util.safehasattr(sys.stdout, "write_through"): if util.safehasattr(sys.stdout, "write_through"):
kwargs["write_through"] = sys.stdout.write_through kwargs["write_through"] = sys.stdout.write_through
sys.stdout = io.TextIOWrapper( sys.stdout = io.TextIOWrapper(
sys.stdout.buffer, sys.stdout.encoding, sys.stdout.errors, **kwargs sys.stdout.buffer, sys.stdout.encoding, sys.stdout.errors, **kwargs
) )
sys.stderr = sys.stderr or io.TextIOWrapper(io.BytesIO())
kwargs = { kwargs = {
"newline": "\n", "newline": "\n",
"line_buffering": sys.stderr.line_buffering, "line_buffering": sys.stderr.line_buffering,
} }
if util.safehasattr(sys.stderr, "write_through"): if util.safehasattr(sys.stderr, "write_through"):
kwargs["write_through"] = sys.stderr.write_through kwargs["write_through"] = sys.stderr.write_through
sys.stderr = io.TextIOWrapper( sys.stderr = io.TextIOWrapper(
sys.stderr.buffer, sys.stderr.encoding, sys.stderr.errors, **kwargs sys.stderr.buffer, sys.stderr.encoding, sys.stderr.errors, **kwargs
) )
# No write_through on read-only stream. # No write_through on read-only stream.
sys.stdin = sys.stdin or io.TextIOWrapper(io.BytesIO())
sys.stdin = io.TextIOWrapper( sys.stdin = io.TextIOWrapper(
sys.stdin.buffer, sys.stdin.buffer,
sys.stdin.encoding, sys.stdin.encoding,
sys.stdin.errors, sys.stdin.errors,
# None is universal newlines mode. # None is universal newlines mode.
newline=None, newline=None,
line_buffering=sys.stdin.line_buffering, line_buffering=sys.stdin.line_buffering,
) )

mercurial/utils/procutil.py

# written. # written.
return stream return stream
# In general, the write() method of streams is free to write only part of # In general, the write() method of streams is free to write only part of
# the data. # the data.
return WriteAllWrapper(stream) return WriteAllWrapper(stream)
if pycompat.ispy3: if pycompat.ispy3:
# Stdio objects can be 'None' on Python 3. Most code paths (for example,
# dispatch.initstdio) do not expect that.
stdin = sys.stdin or io.TextIOWrapper(io.BytesIO())
stdout = sys.stdout or io.TextIOWrapper(io.BytesIO())
stderr = sys.stderr or io.TextIOWrapper(io.BytesIO())
mjacobUnsubmitted
Not Done

The created streams are replaced later by initstdio(), causing them to get garbage-collected without getting closed explicitly.

If you set environment variable PYTHONWARNINGS=always, you’ll get something like

/home/manu/vcs/hg/mercurial/dispatch.py:189: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='r' encoding='UTF-8'>
  sys.stdin = io.TextIOWrapper(
mjacob: The created streams are replaced later by `initstdio()`, causing them to get garbage-collected…
mjacobUnsubmitted
Not Done

Is there a reason for not opening a dedicated stream for stderr?

mjacob: Is there a reason for not opening a dedicated stream for stderr?
# Python 3 implements its own I/O streams. # Python 3 implements its own I/O streams.
# TODO: .buffer might not exist if std streams were replaced; we'll need # TODO: .buffer might not exist if std streams were replaced; we'll need
# a silly wrapper to make a bytes stream backed by a unicode one. # a silly wrapper to make a bytes stream backed by a unicode one.
stdin = sys.stdin.buffer stdin = stdin.buffer
stdout = _make_write_all(sys.stdout.buffer) stdout = _make_write_all(stdout.buffer)
stderr = _make_write_all(sys.stderr.buffer) stderr = _make_write_all(stderr.buffer)
if pycompat.iswindows: if pycompat.iswindows:
# Work around Windows bugs. # Work around Windows bugs.
stdout = platform.winstdout(stdout) stdout = platform.winstdout(stdout)
stderr = platform.winstdout(stderr) stderr = platform.winstdout(stderr)
if isatty(stdout): if isatty(stdout):
# The standard library doesn't offer line-buffered binary streams. # The standard library doesn't offer line-buffered binary streams.
stdout = make_line_buffered(stdout) stdout = make_line_buffered(stdout)
else: else:

tests/test-stdio-missing.t

  • This file was added.
$ cat > prompt.py << 'EOF'
> from mercurial import exthelper
> eh = exthelper.exthelper()
> cmdtable = eh.cmdtable
> @eh.command(b'prompt', [], norepo=True)
> def prompt(ui):
> chosen = ui.promptchoice(b"is stdin present? (y/N) $$ &Yes $$ &No", default=1)
> ui.write(b"chosen: %d\n" % chosen)
> EOF
$ hg --config extensions.prompt=prompt.py prompt 0<&-
is stdin present? (y/N) n
chosen: 1