dart-lang / language

Design of the Dart language
Other
2.65k stars 203 forks source link

Macros build restrictions #3951

Open davidmorgan opened 3 months ago

davidmorgan commented 3 months ago

We've talked at various points about macros being limited to what libraries they can use, to prevent e.g. file access; we need to design+implement this restriction :)

jakemac53 commented 3 months ago

Fwiw the restriction should be enforced at the CFE "target" level, which can control which dart: libraries are available.

lrhn commented 2 months ago

Agree that disallowing entire libraries is the only viable way to disable functionality. That probably means disabling dart:io. I don't think disabling dart:isolate should be necessary, but if we want to make sure that the macro implementation doesn't leave running isolates behind, we may want a way to shut down all isolates spawned by the macro code. It's easier to just not allow isolates, but that may put restrictions on which packages a macro can use. (Likely not many, it's rare to use isolates and not dart:io.) Allowing only the core, platform-independent SDK libraries (core, async, collection, convert, math, typed_data, maybe developer while developing) is the simplest approach.

davidmorgan commented 1 month ago

Hmmmmmm.

Now that I think about it, I don't see how we could fully "disable" these dart packages: macros will be built by combining user-written code with "bootstrap" code that necessarily includes communication with the host, meaning dart:io and/or dart:isolate. The macro considered as an executable definitely needs to do I/O.

So what we are talking about is not nearly as neat as a whole-program restriction.

Given that we have dynamic access this seems nontrivial, e.g. what if some private package:macros class happens to have a helper method:

File openFile(String path) => dart_io.File(path);

and macro code can get to an instance of it? Then it can do (foo as dynamic).openFile(...).

I guess this needs some careful design+review, we also need it to be convincing that we can maintain correctness as we continue to release new language+SDK+library versions.

jakemac53 commented 1 month ago

So what we are talking about is not nearly as neat as a whole-program restriction.

True, although we could write some more specialized validator (via kernel transform) pretty easily, which only looks at the libraries which define macro classes adhere to the allowed imports.

Given that we have dynamic access this seems nontrivial, e.g. what if some private package:macros class happens to have a helper method:

True, I don't know the best way to ensure we don't accidentally leak something like this.

lrhn commented 1 month ago

Not allowing access to dart:io is a reasonable and predictable exception. It's a library that cross-platform code is used to not having access to.

I think we can make the entire isolate running the macro unable to use dart:io. And dart:ffi, because using fopen directly isn't better. It needs to communicate with a macro server, which can access a lot of things, but that server may be in a different isolate.

It's harder to make the macro unable to use dart:isolate, since it likely uses that to communicate with with the macro server. It doesn't have to, we can use external methods in package:macros for communication, which are filled in by the compiler to something that "just works". (It might be sending port messages anyway, just using a low-level external void _portSend(int portId, Object? value); method.)

Or we can make dart:io, dart:isolate and dart:ffi unavailable to user code compiled into a macro implementation, while allowing package:macros to ignore that restriction. Then we probably need to disallow dart:mirrors too, otherwise the workaround is trivial. (And good riddance!) It's trickier, leaves more room for leaking capabilities, but it should be doable.

(We should probably disallow importing package:macros/src/anything directly from user code. Just in case we want to have "secret sauce" libraries in there.)