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.

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

Swallow potential exceptions from showtraceback() #13934

Merged
merged 3 commits into from Feb 10, 2023
Merged

Conversation

shreve
Copy link
Contributor

@shreve shreve commented Feb 8, 2023

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.

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.

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.

    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.
@shreve
Copy link
Contributor Author

shreve commented Feb 8, 2023

Of course! The tests all pass on my machine, so I have no idea why they're failing in CI. Converting to draft until I figure this out.

@Carreau Carreau marked this pull request as ready for review February 8, 2023 14:19
@Carreau Carreau added this to the 8.10 milestone Feb 8, 2023
@Carreau
Copy link
Member

Carreau commented Feb 8, 2023

That looks good to me, as test are now passing I'm marking as ready to merge, I'll leave the be for a few hours/a day and will merge, unless someone else stop by and merge in the meantime.
Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants