Skip to content

Commit

Permalink
Swallow potential exceptions from showtraceback() (#13934)
Browse files Browse the repository at this point in the history
The nbgrader project is aware of a form of cheating where students
disrupt `InteractiveShell.showtraceback` in hopes of hiding exceptions
to avoid losing points. They have implemented a solution to prevent this
cheating from working on the client side, and have some tests to
demonstrate this technique:


https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb
https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt-alternative.ipynb

In essence, these attacks import the interactive shell and erase the
traceback handler so that their failing tests won't report failures.

```python
import IPython.core.interactiveshell
IPython.core.interactiveshell.InteractiveShell.showtraceback = None
```

The problem is that this causes an exception inside the kernel, leading
to a stalled execution. The kernel has stopped working, but the client
continues to wait for messages. So far, nbgrader's solution to this is
to require a timeout value so the client can eventually decide it is
done. This prevents allowing a value of `None` for `Execute.timeout`
because this would cause a test case to infinitely hang.

This commit addresses the problem by making `InteractiveShell._run_cell`
a little more protective around it's call to `showtraceback()`. There is
already a try/except block around running the cell. This commit adds a
finally clause so that the method will _always_ return an
`ExecutionResult`, even if a new exception is thrown within the except
clause. For the record, the exception thrown is:

    TypeError: 'NoneType' object is not callable

Accepting this change will allow nbgrader to update
`nbgrader.preprocessors.Execute` to support a type of
`Integer(allow_none=True)` as the parent `NotebookClient` intended.

Discussion about this is ongoing in jupyter/nbgrader#1690.
  • Loading branch information
Carreau committed Feb 10, 2023
2 parents 0694b08 + a011765 commit e548ee2
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 1 deletion.
3 changes: 2 additions & 1 deletion IPython/core/interactiveshell.py
Expand Up @@ -3013,14 +3013,15 @@ def _run_cell(
runner = _pseudo_sync_runner

try:
return runner(coro)
result = runner(coro)
except BaseException as e:
info = ExecutionInfo(
raw_cell, store_history, silent, shell_futures, cell_id
)
result = ExecutionResult(info)
result.error_in_exec = e
self.showtraceback(running_compiled_code=True)
finally:
return result

def should_run_async(
Expand Down
46 changes: 46 additions & 0 deletions IPython/core/tests/test_interactiveshell.py
Expand Up @@ -1127,3 +1127,49 @@ def foo(*args, **kwargs):

# clean up
ip.Completer.custom_matchers.pop()


class TestShowTracebackAttack(unittest.TestCase):
"""Test that the interactive shell is resilient against the client attack of
manipulating the showtracebacks method. These attacks shouldn't result in an
unhandled exception in the kernel."""

def setUp(self):
self.orig_showtraceback = interactiveshell.InteractiveShell.showtraceback

def tearDown(self):
interactiveshell.InteractiveShell.showtraceback = self.orig_showtraceback

def test_set_show_tracebacks_none(self):
"""Test the case of the client setting showtracebacks to None"""

result = ip.run_cell(
"""
import IPython.core.interactiveshell
IPython.core.interactiveshell.InteractiveShell.showtraceback = None
assert False, "This should not raise an exception"
"""
)
print(result)

assert result.result is None
assert isinstance(result.error_in_exec, TypeError)
assert str(result.error_in_exec) == "'NoneType' object is not callable"

def test_set_show_tracebacks_noop(self):
"""Test the case of the client setting showtracebacks to a no op lambda"""

result = ip.run_cell(
"""
import IPython.core.interactiveshell
IPython.core.interactiveshell.InteractiveShell.showtraceback = lambda *args, **kwargs: None
assert False, "This should not raise an exception"
"""
)
print(result)

assert result.result is None
assert isinstance(result.error_in_exec, AssertionError)
assert str(result.error_in_exec) == "This should not raise an exception"

0 comments on commit e548ee2

Please sign in to comment.