Ban variables whose capability is unsound to alias #3953

jasoncarr0 · 2021-12-31T19:22:59Z

This PR introduces checks around variables and other unaliasing points, to reject programs which attempt to create variables whose capabilities are inherently unsound. The checks about comptability of intersections have been moved to this check after investigation of the origin, as it is not needed for soundness, but this can be reinstated in the original location if helpful for engineering purposes.

Fixes #3931

Some generic code may still produce unsoundness after instantiating, but this change is hopefully a step-forwarding to introducing the necessary constraints.

src/libponyc/pass/verify.c

src/libponyc/expr/reference.c

src/libponyc/type/alias.c

src/libponyc/type/cap.c

src/libponyc/expr/call.c

SeanTAllen · 2022-01-30T13:31:33Z

I don't really understand the usage of errorframe_append but it appears that the new code doesn't use it the same as the existing code.

SeanTAllen · 2022-01-30T13:32:53Z

I believe after reading the error closing for a while is that we are getting a fatal error like a segfault or something from some of the tests apparently perhaps when run in conjunction with others. It makes me think there's some memory clobbering or something going on.

src/libponyc/type/viewpoint.c

SeanTAllen · 2022-01-30T14:28:56Z

Ok so StableTypeTest.EphemeralIsoParameter in my testing:

has a fatal error:

/home/sean/code/ponylang/ponyc/test/libponyc/util.cc:362: Failure
Expected equality of these values:
  (void*)__null
    Which is: NULL
  program
    Which is: 0x7ff5884b7d00
/home/sean/code/ponylang/ponyc/test/libponyc/stable_type.cc:186: Failure
Expected: test_error(src, "syntax") doesn't generate new fatal failures in the current thread.
  Actual: it does.

when I have it set to only go through the "syntax" path.

From this I conclude that the vast majority of the code in this PR "can't be at fault".

I don't know if the alias.c changes would impact if we go through syntax.
or the changes in cap.c
or the changes in viewpoint.c

So I'm left with a few possible conclusions given that the same code when compiled with ponyc works fine without a kaboom.

This is somehow a weird interaction with other tests that arent part of the stable types suite.

It can't be internal to itself as I tried with only that test in the stable type tests.

There's a bug in the changes in one of the 3 files above

But I don't think they impact at the syntax stage

There's a bug in the test that we are triggering
How we compile for the test harness is surfacing a bug that we don't get for ponyc

SeanTAllen · 2022-01-30T14:31:58Z

This is somehow a weird interaction with other tests that arent part of the stable types suite.

It's not this as it still happens if it is THE ONLY test at all for libponyc.tests

SeanTAllen · 2022-01-30T14:39:46Z

This fails:

TEST_F(StableTypeTest, Foo)
{
  // parameters should reject even if never called
  const char* x =
    "interface Foo";

  DO(test_error(x, "syntax"));
}

So I'm horribly confused at this point. It fails by itself after make clean.

SeanTAllen · 2022-01-30T14:52:33Z

I started with a totally fresh new file in case "something weird in the file".

And I get this:

I have no idea how this could be the case and other tests work.

Given that the test is only "see if interface Foo compiles".

SeanTAllen · 2022-01-30T14:58:36Z

So the error we are getting back is...

i.e. we have a null ast after running

build_package(pass, src, _first_pkg_path, true, NULL, false);

because this goes boom

package = ast_child(program);

in

void PassTest::test_compile(const char* src, const char* pass)

SeanTAllen · 2022-01-30T15:00:56Z

Ok so test compile requires there to be a main actor. That explains some of that.

SeanTAllen · 2022-01-30T15:09:16Z

I think I know what might be going on.

This parameter code path isnt generating errors in the way expected.

SeanTAllen · 2022-01-30T15:10:09Z

Yup, issue confirmed look at the output when run with ponyc:

~/pony-scratch/stable-test ➜ /home/sean/code/ponylang/ponyc/build/debug/ponyc
Building builtin -> /home/sean/code/ponylang/ponyc/packages/builtin
Building . -> /home/sean/pony-scratch/stable-test
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
 Descriptors
Optimising
Writing ./stable-test.o
Linking ./stable-test
Error:
/home/sean/pony-scratch/stable-test/main.pony:5:14: illegal type for parameter
  fun bad(a: A iso^) =>

We made it to link AND we reported an error. That means the error isnt reported correctly "as an error".

SeanTAllen · 2022-01-30T15:10:57Z

the return false here is the fix:

SeanTAllen · 2022-01-30T15:15:24Z

I need to do some stuff now, but Ill continue fixing up later and push changes.

SeanTAllen · 2022-01-30T16:54:01Z

Ok I have the tests passing.

SeanTAllen · 2022-01-30T16:57:35Z

I fixed what I believe was a bug in the new code in viewpoint.c.

You had

              a_with = consume_type(with, TK_NONE);
            {
              ast_t* c_with = consume_type(with, TK_NONE);
              if (c_with != NULL)
              {
                a_with = with;

but a_with is already with
so I changed to a_with = c_with

as I assume that was what you intended.

I removed the verify_definitions that you noted wasn't needed.

Did some small cleanup.

Added what appeared to be some missing frame appending for errors

There's still some cleanup but I think this should be much further along.

I've pushed to the branch so you'll need to pull.

src/libponyc/expr/array.c

src/libponyc/type/viewpoint.c

src/libponyc/verify/fun.c

(This should be rejected earlier but in case the compiler has a bug it's the code's fault)

… unsound)

SeanTAllen · 2022-02-14T21:49:15Z

I'll be doing the last of the fixes for this soon-ish.

SeanTAllen · 2022-02-15T15:11:56Z

@jasoncarr0 do you have time to give this a quick review to make sure it looks good? if not, i can move ahead as i believe i got everything from our conversations handled. i feel "pretty darn confident" on that.

jasoncarr0 · 2022-02-15T15:33:41Z

src/libponyc/type/cap.h

+ * Normalizes the capability and alias modifier to apply aliasing
+ * and remove unnecessary ephemeral modifiers.
+ */
+void cap_aliasing(token_id* cap, token_id* eph);


Is this worth leaving in? Or should we revert

It is still used elsewhere.

ponylang-main · 2022-02-15T16:45:48Z

Hi @jasoncarr0,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3953.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

This reverts commit 3ea2647.

jasoncarr0 changed the title ~~[WIP] Ban unstable variables~~ Ban unstable variables Jan 16, 2022

jasoncarr0 changed the title ~~Ban unstable variables~~ Ban variables whose capabillity is unsound to alias Jan 16, 2022

jasoncarr0 commented Jan 18, 2022

View reviewed changes

src/libponyc/pass/verify.c Outdated Show resolved Hide resolved

jemc reviewed Jan 18, 2022

View reviewed changes

src/libponyc/expr/reference.c Outdated Show resolved Hide resolved

src/libponyc/type/alias.c Outdated Show resolved Hide resolved

src/libponyc/type/cap.c Show resolved Hide resolved