artiq icon indicating copy to clipboard operation
artiq copied to clipboard

Unsound parameter lifetime handling

Open pca006132 opened this issue 3 years ago • 1 comments

Bug Report

One-Line Summary

The current lifetime check for function parameters is unsound and can lead to memory corruption.

Issue Details

Even if #1497 (compiler: Function return values (?) not lifetime-tracked) is fixed appropriately, the current escape analysis for function parameters is still unsound and can lead to memory corruption. The reason is that the lifetime for function parameters are treated as the same, but they should be treated as incomparable for correct analysis.

For example, consider the following code:

@kernel
def bar(ls):
    a = [1, 2, 3]
    ls[0] = a

This would cause a compilation error, as the compiler correctly identified that the variable a is mutable, and does not outlive the variable ls which is passed from the caller. However, if we change the function a bit:

@kernel
def foo(ls, a):
    ls[0] = a

@kernel
def bar(ls):
    a = [1, 2, 3]
    foo(ls, a)

Instead of assigning it directly, we pass to another function foo which does the assignment. As the current escape analysis treats each parameter to have the same scope, this passes the lifetime check, but the behavior is exactly the same as the previous one, and should be rejected.

Steps to Reproduce

Run the following code:

from artiq.experiment import *

@kernel
def foo(ls, a):
    ls[0] = a

@kernel
def bar(ls):
    a = [1, 2, 3]
    foo(ls, a)

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

    @kernel
    def run(self):
        ls = [[1]]
        bar(ls)
        ls[0][0] = 0
        a = ls[0]
        print(ls[0])
        bar(ls)
        print(ls[0])
        print(a)

Expected Behavior

Either reject the code or print

[0, 2, 3]
[1, 2, 3]
[0, 2, 3]

This is because we saved a = ls[0]. The function bar should assign a new list [1, 2, 3] to the first element in ls, so it should not affect our variable a.

Actual (undesired) Behavior

Prints

[0, 2, 3]
[1, 2, 3]
[1, 2, 3]

Your System (omit irrelevant parts)

  • ARTIQ version: Current master

pca006132 avatar May 20 '21 04:05 pca006132

I think I might have just seen another version of this. We had some code that did something similar to:

class Escape2(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.s = ""

    def run(self):
        self.run1("base")

    @kernel
    def run1(self, base):
        tmp = base + ".extend"
        self.run2(tmp)
        print("self.s", self.s)

    @kernel
    def run2(self, s):
        self.s = s

Which crashes in the UTF-8 validation of the RPC call to write back the value of s (the crash looks similar to https://github.com/m-labs/artiq/issues/1379, but I'm convinced that this is the underlying cause). The original code involved more nesting and an explicit RPC that tried to use a reference that had outlived the lifetime of the value. But the upshot was the same.

mbirtwell avatar Mar 11 '22 10:03 mbirtwell