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 compiler bug that allows an unsafe data access pattern #4458

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

SeanTAllen
Copy link
Member

Last November, Gordon identified that the compiler was allowing code to recover a val from a box field. This is unsafe and very "doh".

After investigation, it was determined that this bug has existed since Sylvan introduced viewpoint adaptation to the compiler almost a decade ago.

I came up with a fix, but after talking with Sylvan, we changed to this fix as it is the "proper pure theory fix".

Fixes #4244

Last November, Gordon identified that the compiler was allowing code
to recover a `val` from a `box` field. This is unsafe and very "doh".

After investigation, it was determined that this bug has existed since
Sylvan introduced viewpoint adaptation to the compiler almost a decade
ago.

I came up with a fix, but after talking with Sylvan, we changed to this
fix as it is the "proper pure theory fix".

Fixes #4244
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Oct 5, 2023
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Oct 5, 2023
@SeanTAllen SeanTAllen requested a review from a team October 5, 2023 13:06
@SeanTAllen SeanTAllen mentioned this pull request Oct 5, 2023
1 task
@jasoncarr0
Copy link
Contributor

jasoncarr0 commented Oct 5, 2023

This works for the specific case but can run into different issues when it comes to generics.
I know it has issues because I've thought of / tried this exact fix before.

If you give me a second I can produce a working example today if there is one. IIRC the core issue is that code can be erroneously accepted because the upper bound is tag which is marked sendable, but some instantiations are nevertheless unsound.

@SeanTAllen
Copy link
Member Author

Unless this breaks something that is currently correct, let's open an issues for other problems.

@jasoncarr0
Copy link
Contributor

jasoncarr0 commented Oct 5, 2023

This does cause some new code to erroneously make it past sendability checks that was correctly blocked before.

But I would recommend to merging it anyway since these are quite hard to exploit.

This code nearly demonstrates, but gets stopped later on by seemingly a bug (you can easily check manually by instantiation that this code should pass typechecking excluding sendability).

class A
class Bad[T]
  let _t: T! // We could also use T: Any #alias but that's more internal 
  new create(t: T!) =>
    this._t = t

  fun bad(): val->(T!) =>
    recover
      this._t    // has type this->(T!)
    end

actor Main
  new create(env: Env) =>
    let a: A ref = A
    let a_val: A val = Bad[A ref](a).bad()

Note that after this change, there is no error message for sendability (but there should be).

@SeanTAllen
Copy link
Member Author

Thanks @jasoncarr0. I'll open 1 or more issues for additional improvements.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Oct 5, 2023
@SeanTAllen
Copy link
Member Author

I've added do not merge to this so I can update the release notes.

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Oct 6, 2023
@SeanTAllen SeanTAllen merged commit 0f4f11a into main Oct 6, 2023
26 checks passed
@SeanTAllen SeanTAllen deleted the issue-4244 branch October 6, 2023 16:29
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Oct 6, 2023
github-actions bot pushed a commit that referenced this pull request Oct 6, 2023
github-actions bot pushed a commit that referenced this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

You can obtain a val from a box field via recover
4 participants