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

fix: don't allow lambdas to leak captures #1440

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

scolsen
Copy link
Contributor

@scolsen scolsen commented Oct 30, 2022

When lambdas close over some external environment variable, if that variable is a linear value, their environment becomes the owner of the captured value and the value will be freed with the environment. If the lambda moves this variable out of its own scope, however, e.g. by returning it in its body, both the caller and the lambda environment will wind up owning the value, resulting in double frees.

For now, we prevent this scenario by simply disallowing functions to leak variables captured from another scope. Doing so is now an error. Of course, one can still copy these values without issue.

We achieve this through set operations. If the memory state loses the fake deleters for the set of a lambdas captured bindings after its body is evaluated, we've encountered an ownership leak, and report an error.

(defn example []
  (let [capture @""
        lambda (fn [] capture)])) ;; this is now an error

(defn example []
  (let [capture @""
        lambda (fn [] @&capture)])) ;; this is still OK

fixes #1040

When lambdas close over some external environment variable, if that
variable is a linear value, their environment becomes the owner of the
captured value and the value will be freed with the environment. If the
lambda moves this variable out of its own scope, however, e.g. by
returning it in its body, both the caller and the lambda environment
will wind up owning the value, resulting in double frees.

For now, we prevent this scenario by simply disallowing functions to
leak variables captured from another scope. Doing so is now an error. Of
course, one can still copy these values without issue.

We achieve this through set operations. If the memory state loses the
fake deleters for the set of a lambdas captured bindings after its body
is evaluated, we've encountered an ownership leak, and report an error.

```clojure
(defn example []
  (let [capture @""
        lambda (fn [] capture)])) ;; this is now an error

(defn example []
  (let [capture @""
        lambda (fn [] @&capture)])) ;; this is still OK
```

fixes carp-lang#1040
@scolsen
Copy link
Contributor Author

scolsen commented Oct 30, 2022

Note: here's an example of what the error currently looks like:

The function (defn <s : String> _Lambda_foo_12_env [_env] s) gives away the captured variables: s. Functions must keep ownership of variables captured from another environment. at REPL:2:2.

Traceback:
  (defn foo [] (let [s (copy "") f (fn [] s)] (f))) at REPL:2:1.

fairly unhelpful function name b/c we check this after the lambda has been lifted. I'll tackle this as a later improvement. I gotta add an error test as well.

@scolsen
Copy link
Contributor Author

scolsen commented Oct 30, 2022

op, looks like there are some regressions I'll have to take care of first

Adds tests to ensure lambdas do not leak ownership of their captured
variables. In addition, the nested lambda test now needs to accept a
reference to a function instead of a function value (otherwise an
ownership leak occurs).
@scolsen
Copy link
Contributor Author

scolsen commented Oct 31, 2022

Note: MacOS failures are due to new github runner images which have a new version of clang that throws a waring that our generated C has triggered. I'll open a separate PR to fix this.

Copy link
Collaborator

@eriksvedang eriksvedang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good way to do this!

At some point (I believe version 13.0.0?) clang added a warning that
catches variables that were assigned but unused. This version of clang
(or later) is now bundled w/ github's macos images and is causing our
tests to fail in continuous integration. We can currently generate C
code that trips this warning, so for now I've disabled it as we do some
other warnings related to variable usages.
@scolsen
Copy link
Contributor Author

scolsen commented Nov 1, 2022

oh no... looks like the clang version diffs on different platforms are leading to problems. Linux and Nix CI images are using older clang versions which means the recent macOS CI fix breaks them.

I guess we need to fix this on the CI side instead of baking in warning flag options into default Carp projects.

@eriksvedang
Copy link
Collaborator

Dang... perhaps the cleanest thing would be to check for clang version in dynamic carp and add the warning flag there (while loading core) if it is needed. Or can you think of an easier way?

@scolsen
Copy link
Contributor Author

scolsen commented Nov 1, 2022

Dang... perhaps the cleanest thing would be to check for clang version in dynamic carp and add the warning flag there (while loading core) if it is needed. Or can you think of an easier way?

It looks like github has other images of linux at least that have later versions of clang, so we might be able to fix our CI issues just by figuring out how to use those images instead.

As for the more general problem of default project settings--checking the compiler version dynamically seems like a good idea.

The ultimate fix would be to make our generated code -Wall compliant for at least clang since we use that as a default. Maybe we're better off just spending time on that and removing the -W-no warning disable flags?

At the moment, it seems like the only thing we'd have to implement is some check in the emitter that drops unused variables and remove some self-assigns. Not sure how much work that will be.

Another hacky fix would be to remove the -Werror flag from the project during CI runs, which we could probably do in the tests/ script.

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.

Taking ownership of value captured by a lambda env is unsafe
2 participants