( )⚙ D3445 dispatch: validate return type from dispatch() return value

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3445

dispatch: validate return type from dispatch() return value
Needs RevisionPublic

Authored by indygreg on May 6 2018, 12:11 AM.

Details

Reviewers
admin
durin42
Group Reviewers
hg-reviewers
Summary

This commit adds type checking to the result of dispatch() so
we catch non-None and non-int values. This prevents crashes
when we later try to "& 255" this value.

The Python 3 test failures introduced previously have all gone away.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.May 6 2018, 12:11 AM
yuja added a subscriber: yuja.May 6 2018, 8:29 AM

IIRC, SystemExit is caught at callcatch and translated to a status code.

+ # The logic here attempts to mimic how CPython's pythonrun.c does things.
+ # Essentially:
+ #
+ # * Exit is controlled by returning a value or raising SystemExit.
+ # * An integer value exits with that exit code.
+ # * A value of `None is interpreted as 0`.
+ # * A non-integer, non-None value results in that value being printed
+ # and an exit code of `1`.
+ # * SystemExit can have a `code` attribute containing the exit value.

try:
  • status = (dispatch(req) or 0)

+ code = dispatch(req) or 0

except error.StdioError as e:
    err = e
  • status = -1

+ code = -1
+ except SystemExit as err:
+ try:
+ code = err.code
+ err = None
+ except AttributeError:

indygreg edited the summary of this revision. (Show Details)May 12 2018, 12:48 AM
indygreg retitled this revision from dispatch: shore up exit handling to dispatch: validate return type from dispatch() return value.
indygreg updated this revision to Diff 8654.
admin requested changes to this revision.May 31 2018, 3:10 PM
admin added a subscriber: admin.

This looks good, but needs rebased.

This revision now requires changes to proceed.May 31 2018, 3:10 PM
durin42 requested changes to this revision.May 31 2018, 3:11 PM
durin42 added a subscriber: durin42.

(derp, that was me, but my password vault mis-fired and I got admin instead of me.)

Revision Contents
Changeset List

PathPackages
Mmercurial/dispatch.py (50 lines)
Mtests/test-dispatch.t (28 lines)

Diff 8460

mercurial/dispatch.py

if exc is not None: if exc is not None:
raise exc raise exc
def run(): def run():
"run the command in sys.argv" "run the command in sys.argv"
_initstdio() _initstdio()
req = request(pycompat.sysargv[1:]) req = request(pycompat.sysargv[1:])
err = None err = None
# The logic here attempts to mimic how CPython's pythonrun.c does things.
# Essentially:
#
# * Exit is controlled by returning a value or raising SystemExit.
# * An integer value exits with that exit code.
# * A value of ``None`` is interpreted as ``0``.
# * A non-integer, non-None value results in that value being printed
# and an exit code of ``1``.
# * SystemExit can have a ``code`` attribute containing the exit value.
try: try:
status = (dispatch(req) or 0) code = dispatch(req) or 0
except error.StdioError as e: except error.StdioError as e:
err = e err = e
status = -1 code = -1
except SystemExit as err:
try:
code = err.code
err = None
except AttributeError:
pass
# In all cases we flush our stdio streams.
if util.safehasattr(req.ui, 'fout'): if util.safehasattr(req.ui, 'fout'):
try: try:
req.ui.fout.flush() req.ui.fout.flush()
except IOError as e: except IOError as e:
err = e err = e
status = -1 code = -1
if util.safehasattr(req.ui, 'ferr'): if util.safehasattr(req.ui, 'ferr'):
try: try:
if err is not None and err.errno != errno.EPIPE: if err is not None and err.errno != errno.EPIPE:
req.ui.ferr.write('abort: %s\n' % req.ui.ferr.write('abort: %s\n' %
encoding.strtolocal(err.strerror)) encoding.strtolocal(err.strerror))
req.ui.ferr.flush()
# There's not much we can do about an I/O error here. So (possibly) # There's not much we can do about an I/O error here. So (possibly)
# change the status code and move on. # change the status code and move on.
except IOError: except IOError:
status = -1 code = -1
# If we got a non-integer exit code, we print that as well.
if not isinstance(code, int) and util.safehasattr(req.ui, 'ferr'):
try:
req.ui.ferr.write('%r\n' % code)
# This will ignore errors formatting the value and errors printing
# to stderr. That's intended. At this point, there's not much
# we can do about any errors.
except Exception:
pass
code = 1
if util.safehasattr(req.ui, 'ferr'):
try:
req.ui.ferr.flush()
except IOError:
# There's not much we can do about I/O errors at this point.
code = -1
_silencestdio() sys.exit(code & 255)
sys.exit(status & 255)
if pycompat.ispy3: if pycompat.ispy3:
def _initstdio(): def _initstdio():
pass pass
def _silencestdio(): def _silencestdio():
for fp in (sys.stdout, sys.stderr): for fp in (sys.stdout, sys.stderr):
# Check if the file is okay # Check if the file is okay

tests/test-dispatch.t

> [extensions] > [extensions]
> exitmodes = $TESTTMP/exitmodes.py > exitmodes = $TESTTMP/exitmodes.py
> EOF > EOF
$ hg exit returnint 42 $ hg exit returnint 42
[42] [42]
$ hg exit returnstring 'some message' $ hg exit returnstring 'some message'
Traceback (most recent call last): 'some message'
File "*/hg", line *, in <module> (glob)
dispatch.run()
File "*/mercurial/dispatch.py", line *, in run (glob)
sys.exit(status & 255)
TypeError: unsupported operand type(s) for &: 'str' and 'int'
[1] [1]
$ hg exit returnnone $ hg exit returnnone
$ hg exit returnempty $ hg exit returnempty
$ hg exit returndict $ hg exit returndict
Traceback (most recent call last): {'key1': 'value1'}
File "*/hg", line *, in <module> (glob)
dispatch.run()
File "*/mercurial/dispatch.py", line *, in run (glob)
sys.exit(status & 255)
TypeError: unsupported operand type(s) for &: 'dict' and 'int'
[1] [1]
$ hg exit systemexitint 42 $ hg exit systemexitint 42
[42] [42]
$ hg exit systemexitstring 'failure message' $ hg exit systemexitstring 'failure message'
Traceback (most recent call last): 'failure message'
File "*/hg", line *, in <module> (glob)
dispatch.run()
File "*/mercurial/dispatch.py", line *, in run (glob)
sys.exit(status & 255)
TypeError: unsupported operand type(s) for &: 'str' and 'int'
[1] [1]
$ hg exit systemexitnoarg $ hg exit systemexitnoarg
$ hg exit systemexitnone $ hg exit systemexitnone
$ hg exit systemexitdict $ hg exit systemexitdict
Traceback (most recent call last): {'key1': 'value1'}
File "*/hg", line *, in <module> (glob)
dispatch.run()
File "*/mercurial/dispatch.py", line *, in run (glob)
sys.exit(status & 255)
TypeError: unsupported operand type(s) for &: 'dict' and 'int'
[1] [1]