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

Exception marshalling vs. accidental format string syntax in messages #2058

Open
dnadlinger opened this issue Mar 3, 2023 · 2 comments · May be fixed by #2061
Open

Exception marshalling vs. accidental format string syntax in messages #2058

dnadlinger opened this issue Mar 3, 2023 · 2 comments · May be fixed by #2061
Assignees

Comments

@dnadlinger
Copy link
Collaborator

dnadlinger commented Mar 3, 2023

Bug Report

One-Line Summary

Exception messages containing substrings looking like format string messages (e.g. {foo}) lead to internal error when marshalled between host and core device.

Issue Details

from artiq.experiment import *

class TestExceptionInterpolation(EnvExperiment):
    def build(self):
        self.setattr_device("core")

    @kernel
    def run(self):
        self.throw()

    def throw(self):
        raise Exception("{foo}")

leads to

Traceback (most recent call last):
  File "/home/ion/scratch-bob/artiq/artiq/master/worker_impl.py", line 417, in main
    exp_inst.run()
  File "/home/ion/scratch-bob/artiq/artiq/language/core.py", line 54, in run_on_core
    return getattr(self, arg).run(run_on_core, ((self,) + k_args), k_kwargs)
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/core.py", line 140, in run
    self._run_compiled(kernel_library, embedding_map, symbolizer, demangler)
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/core.py", line 130, in _run_compiled
    self.comm.serve(embedding_map, symbolizer, demangler)
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/comm_kernel.py", line 716, in serve
    self._serve_exception(embedding_map, symbolizer, demangler)
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/comm_kernel.py", line 698, in _serve_exception
    raise python_exn
RuntimeError: Exception type=<class 'Exception'>, which couldn't be reconstructed ('foo')

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/nix/store/lbn7f0d2k36i4bgfdrjdwj7npy3r3h5d-python3-3.10.8/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/nix/store/lbn7f0d2k36i4bgfdrjdwj7npy3r3h5d-python3-3.10.8/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/ion/scratch-bob/artiq/artiq/master/worker_impl.py", line 446, in <module>
    main()
  File "/home/ion/scratch-bob/artiq/artiq/master/worker_impl.py", line 439, in main
    put_exception_report()
  File "/home/ion/scratch-bob/artiq/artiq/master/worker_impl.py", line 338, in put_exception_report
    lines.append(str(exc.artiq_core_exception))
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/exceptions.py", line 101, in __str__
    tracebacks = [self.single_traceback(i) for i in range(len(self.exceptions))]
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/exceptions.py", line 101, in <listcomp>
    tracebacks = [self.single_traceback(i) for i in range(len(self.exceptions))]
  File "/home/ion/scratch-bob/artiq/artiq/coredevice/exceptions.py", line 80, in single_traceback
    lines.append("{}({}): {}".format(name, exn_id, message.format(*params)))
KeyError: 'foo'

This isn't easy to work around, as the exception message might not come directly from user code (perhaps even another language where {…} isn't related to string interpolation, as was the case with Julia in the application here).

This is probably a regression introduced during the recent exception message rework, though I haven't actually tested older versions.

Your System

  • ARTIQ version: Recent master based on 8dc6902.
@dnadlinger
Copy link
Collaborator Author

dnadlinger commented Mar 3, 2023

Hacky patch when tracking down where this came from:

diff --git a/artiq/coredevice/comm_kernel.py b/artiq/coredevice/comm_kernel.py
index 3d5b8dea9..d37173bbc 100644
--- a/artiq/coredevice/comm_kernel.py
+++ b/artiq/coredevice/comm_kernel.py
@@ -14,6 +14,12 @@ from sipyco.keepalive import create_connection
 logger = logging.getLogger(__name__)


+def format_core_exception(message, params):
+    for i, p in enumerate(params):
+        message = message.replace("{" + str(i) + "}", str(p))
+    return message
+
+
 class Request(Enum):
     SystemInfo = 3

@@ -687,8 +693,7 @@ class CommKernel:
             python_exn_type = embedding_map.retrieve_object(core_exn.id)

         try:
-            python_exn = python_exn_type(
-                nested_exceptions[-1][1].format(*nested_exceptions[0][2]))
+            python_exn = python_exn_type(format_core_exception(nested_exceptions[-1][1], nested_exceptions[0][2]))
         except Exception as ex:
             python_exn = RuntimeError(
                 f"Exception type={python_exn_type}, which couldn't be "
diff --git a/artiq/coredevice/exceptions.py b/artiq/coredevice/exceptions.py
index 7b6967743..ffaae1d36 100644
--- a/artiq/coredevice/exceptions.py
+++ b/artiq/coredevice/exceptions.py
@@ -14,6 +14,12 @@ RuntimeError = builtins.RuntimeError
 AssertionError = builtins.AssertionError


+def format_core_exception(message, params):
+    for i, p in enumerate(params):
+        message = message.replace("{" + str(i) + "}", str(p))
+    return message
+
+
 class CoreException:
     """Information about an exception raised or passed through the core device."""
     def __init__(self, exceptions, exception_info, traceback, stack_pointers):
@@ -70,14 +76,13 @@ class CoreException:
                           self.stack_pointers[start_backtrace_index:]))
         exception = self.exceptions[exception_index]
         name = exception[0]
-        message = exception[1]
-        params = exception[2]
+        message = format_core_exception(exception[1], exception[2])
         if ':' in name:
             exn_id, name = name.split(':', 2)
             exn_id = int(exn_id)
         else:
             exn_id = 0
-        lines.append("{}({}): {}".format(name, exn_id, message.format(*params)))
+        lines.append("{}({}): {}".format(name, exn_id, message))
         zipped.append(((exception[3], exception[4], exception[5], exception[6],
                        None, []), None))

The python_exn_type(nested_exceptions[-1][1].format(*nested_exceptions[0][2])) looks potentially dodgy as well (mixing of -1 and 0 outer indices).

@thomasfire
Copy link
Contributor

Can confirm this for release-7 branch

@thomasfire thomasfire self-assigned this Mar 9, 2023
@thomasfire thomasfire linked a pull request Mar 9, 2023 that will close this issue
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 a pull request may close this issue.

2 participants