1
0
mirror of https://git.tartarus.org/simon/putty.git synced 2025-05-28 23:34:49 -05:00

Handle crashes in the testcrypt binary more cleanly.

Previously, if the testcrypt subprocess suffered any kind of crash or
assertion failure during a run of the Python-based test system, the
effect would be that ChildProcess.read_line() would get EOF, ignore
it, and silently return the empty string. Then it would carry on doing
that for the rest of the program, leading to a long string of error
reports in tests that were nowhere near the code that actually caused
the crash.

Now ChildProcess.read_line() detects EOF and raises an exception, so
that the test suite won't heedlessly carry on trying to do things once
it's noticed that its subprocess has gone away.

This is more fiddly than it sounds, however, because of the wrinkle
that sometimes that function can be called while a Python __del__
method is asking testcrypt to free something. If that happens, the
exception can't be propagated out of the __del__ (analogously to the
rule that it's a really terrible idea for C++ destructors to throw).
So you get an annoying warning message on standard error, and then the
next command sent to testcrypt will be back in the same position.
Worse still, this can also happen if testcrypt has _already_ crashed,
because the __del__ methods will still run.

To protect against _that_, ChildProcess caches the exception after
throwing it, and then each subsequent write_line() will rethrow it.
And __del__ catches and explicitly ignores the exception (to avoid the
annoying warning if Python has to do the same).

The combined result should be that if testcrypt crashes in normal
(non-__del__) context, we should get a single exception that
terminates the run cleanly without cascade failures, and whose
backtrace localises the problem to the actual operation that caused
the crash. If testcrypt crashes in __del__, we can't quite do that
well, but we can still terminate with an exception at the next
opportunity, avoiding multiple cascade failures.

Also in this commit, I've got rid of the try-finally in
cryptsuite.py's (trivial) main program.
This commit is contained in:
Simon Tatham 2019-03-24 09:33:58 +00:00
parent 6ecc16fc4b
commit 7f9aba638f

View File

@ -22,11 +22,15 @@ def valbytes(b):
b = list(b)
return struct.pack("{:d}B".format(len(b)), *b)
class ChildProcessFailure(Exception):
pass
class ChildProcess(object):
def __init__(self):
self.sp = None
self.debug = None
self.exitstatus = None
self.exception = None
dbg = os.environ.get("PUTTY_TESTCRYPT_DEBUG")
if dbg is not None:
@ -47,12 +51,21 @@ class ChildProcess(object):
self.sp = subprocess.Popen(
cmd, shell=shell, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
def write_line(self, line):
if self.exception is not None:
# Re-raise our fatal-error exception, if it previously
# occurred in a context where it couldn't be propagated (a
# __del__ method).
raise self.exception
if self.debug is not None:
self.debug.write("send: {}\n".format(line))
self.sp.stdin.write(line + b"\n")
self.sp.stdin.flush()
def read_line(self):
line = self.sp.stdout.readline().rstrip(b"\r\n")
line = self.sp.stdout.readline()
if len(line) == 0:
self.exception = ChildProcessFailure("received EOF from testcrypt")
raise self.exception
line = line.rstrip(b"\r\n")
if self.debug is not None:
self.debug.write("recv: {}\n".format(line))
return line
@ -71,8 +84,8 @@ class ChildProcess(object):
def check_return_status(self):
self.wait_for_exit()
if self.exitstatus is not None and self.exitstatus != 0:
raise Exception("testcrypt returned exit status {}"
.format(self.exitstatus))
raise ChildProcessFailure("testcrypt returned exit status {}"
.format(self.exitstatus))
childprocess = ChildProcess()
@ -86,7 +99,22 @@ class Value(object):
return "Value({!r}, {!r})".format(self.typename, self.ident)
def __del__(self):
if self.ident is not None:
childprocess.funcall("free", [self.ident])
try:
childprocess.funcall("free", [self.ident])
except ChildProcessFailure:
# If we see this exception now, we can't do anything
# about it, because exceptions don't propagate out of
# __del__ methods. Squelch it to prevent the annoying
# runtime warning from Python, and the
# 'self.exception' mechanism in the ChildProcess class
# will raise it again at the next opportunity.
#
# (This covers both the case where testcrypt crashes
# _during_ one of these free operations, and the
# silencing of cascade failures when we try to send a
# "free" command to testcrypt after it had already
# crashed for some other reason.)
pass
def __long__(self):
if self.typename != "val_mpint":
raise TypeError("testcrypt values of types other than mpint"