m-labs / artiq

A leading-edge control system for quantum information experiments
https://m-labs.hk/artiq
GNU Lesser General Public License v3.0
434 stars 200 forks source link

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

Open dnadlinger opened 1 year ago

dnadlinger commented 1 year ago

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

dnadlinger commented 1 year ago

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 commented 1 year ago

Can confirm this for release-7 branch