search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.07k stars 1.56k forks source link

Need better protection from Type.toString() in release modes #46373

Open yjbanov opened 3 years ago

yjbanov commented 3 years ago

Issues from the Flutterverse:

Type.toString in release mode deviating from profile and debug builds frequently leads to surprises. For example, developers write code like this and this, expecting it to work, then file issues when it doesn't.

Because Type.toString always returns something most programs continue running and fail somewhere far from the culprit. It is sometimes hard to trace it back to Type.toString. For example, https://github.com/flutter/flutter/issues/78041 looked like a bug in Flutter and/or dart2js compiler, because the output of Type.toString simply sent the app along a different branch (one that created a Positioned widget).

It seems a static lint-based solution doesn't always work. One of the examples above passes a reference to Type via dynamic, so a static linter wouldn't catch it.

Perhaps a runtime approach would be more effective. For example:

/cc @goderbauer @a14n @sigmundch @rakudrama

sigmundch commented 3 years ago

/cc @lrhn @leafpetersen @natebosch

I agree that the static approach in this case may be too limiting. For reference, there is already a lint, but not only it wont catch patterns like the one we investigated this week, it is also designed to allow other static patterns that are sometimes trouble some (see https://github.com/dart-lang/linter/issues/2610)

lrhn commented 3 years ago

I would love for Type.toString to return "[Instance of Type]". No need to retain the source names at run-time at all. (As a follow up, we could change Object.toString and enumValue.toString as well. That would be glorious!) Would likely break a lot of existing code, though.

natebosch commented 3 years ago

Taking away the debugging help from Type.toString in all modes could be painful. I do think it could make sense to have a compiler flag that changes the behavior.

yjbanov commented 3 years ago

Removing stringified symbols entirely might be a little drastic. A program should be able to generate readable error messages. However, we could require that stringification take place inside an assert context, e.g.:

class Foo {}

void main() {
  print('$Foo'); // prints "[Instance of Type]"
  assert(() {
    print('$Foo'); // prints "Foo"
    return true;
  }());
}
lrhn commented 3 years ago

An interesting idea. An "assert context" needs to be async-aware (you can await inside an assert), so it can't just be a simple global boolean flag set during evaluation of an assert statement. So, effectively something like zone based, which again means that an assert would introduce a new zone. That zone can be captured and reused outside the assert.

Complicated.

It would mean that we only need to retain all source names at run-time when assertions are enabled.

Maybe that's a more direct approach: Only retain source names when asserts are enabled. Otherwise we just obfuscate all the names. Even on the VM.

sigmundch commented 3 years ago

Maybe that's a more direct approach: Only retain source names when asserts are enabled. Otherwise we just obfuscate all the names.

I like that simple model, however, my one concern with coupling it with assertions is that assertions are often enabled for testing, so test coverage with mangled string representation of types will likely be spotty. Would this merit a new kind of development/debug-only flag?

One piece I'd like to consider in connection to the Type.toString problem is what we do with stack traces. This is another kind of source-level info that we implicitly leak. We can't produce dart-source stack traces on the web without some deobfuscation/source-map work. Today dart2js doesn't include any of this information due to the high cost on code-size, instead it provides an offline tool to deobfuscate out of process when errors are logged to a server. In cotrast, DDC does it online synchronously as part of the StackTrace.toString.

I'd like to take deobfsucation out of the critical path in DDC (and I wonder if AOT may have similar needs), and instead rely on an out-of-the client process to deobfuscate stacks.

Note that this doesn't mean that users will have a worse experience, we'd simply make the stack presentation separate/asychronous.

I wonder if a similar principle could apply to Type.toString. That is, always produce mangled names, but give tools to our frameworks to present readable error messages easily by unmangling before the messages are presented to the final user.

yjbanov commented 3 years ago

you can await inside an assert

Can you provide a working example? I tried this and the compiler rejected it:

  assert(() async {
    await Future<void>.value();
    return true;
  }());
yjbanov commented 3 years ago

Oh, nvm, all I need is also await on the result of the function call:

  assert(await () async {
    await Future<void>.value();
    return true;
  }());

Hmm...

yjbanov commented 3 years ago

I'd like to take deobfsucation out of the critical path in DDC (and I wonder if AOT may have similar needs), and instead rely on an out-of-the client process to deobfuscate stacks.

If I understand you correctly, you want to obfuscate in all build modes and always deobfuscate offline? If so, I can get behind this idea. I like that it makes all build modes consistent (obfuscated), and it requires tooling for deobfuscation. While that may sound inconvenient, in practice we always provide tooling, so the inconvenience is limited to the tool vendor, e.g. the Flutter and the Dart teams. The big benefit, however, is that Dart code observes consistent behavior across build modes. If we always produce "minified:xyz" strings, we can have tools/libraries that deobfuscate any string, including error messages. For example, we can produce error strings like this one in all modes:

The minified:x Positioned wants to apply minified:y of type minified:z to a minified:v, which has been set up to accept minified:y of incompatible type minified:w.

Then run it through a deobfuscation tool to produce:

The ParentDataWidget Positioned wants to apply ParentData of type StackParentData to a RenderObject, which has been set up to accept ParentData of incompatible type FlexParentData.

In order to work, such tools and libraries would need access to sourcemaps, which is a reasonable requirement. If, for some reason, a developer wants their production app to deobfuscate stack in-process, that's fine too. They'll just have to provide a sourcemap for their code.

yjbanov commented 3 years ago

Having said that, such a mode would need a pretty major migration. I think if we can figure out a way to restrict deobfuscated names to assert expressions, the delta would be much smaller.

sigmundch commented 3 years ago

If I understand you correctly, you want to obfuscate in all build modes and always deobfuscate offline

Precisely!

.. such a mode would need a pretty major migration

I'm curious about what this would entail, could you expand on that? How much do you believe it would reduce the scope if we only change the web at first (DDC and dart2js --no-minify)?

yjbanov commented 3 years ago

@sigmundch

.. such a mode would need a pretty major migration

I'm curious about what this would entail, could you expand on that? How much do you believe it would reduce the scope if we only change the web at first (DDC and dart2js --no-minify)?

I expect there will be a big impact on tooling. Basically all the logs will start producing obfuscated stack traces and error messages. We'll need to teach all the relevant tools to deobfuscate as necessary, including errors printed by tests running on LUCI and sponge, which could be written to files instead of being print()ed and preprocessed by a deobfuscator. We might also want to provide an easy command to deobfuscate a message from a string or a file, e.g. flutter symbolicate -m 'Something with minified:xD went wrong', in case we miss a few spots (we likely will).

Changing it just in DDC and dart2js may not help. The issue above was filed because the author tested the app in AOT, and it worked fine. Only when they tried on the web did the issue show up, leading to the confusion and issue filing. I'm afraid this only works if it's consistent across all compilers and build modes. Except, one inconsistency (and probably the only one as far as the language behavior goes) that we allow between build modes is asserts, which is well understood by developers. It's tempting to capitalize on it.

@lrhn

An "assert context" needs to be async-aware (you can await inside an assert), so it can't just be a simple global boolean flag set during evaluation of an assert statement.

What if it's not a flag but a "ref count"? Just before the assert statement the counter is incremented, and then decremented immediately after, in a try/finally fashion, something like:

// Original
assert(await () async {
  await Future<void>.value();
  return true;
}());

// Desugared
try {
  $globalAssertRefCount += 1;
  assert(await () async {
    await Future<void>.value();
    return true;
  }());
} finally {
  $globalAssertRefCount -= 1;
}

This also makes it trivial to track reentrant asserts.

lrhn commented 3 years ago

@yjbanov If you do:

assert(await Future.delayed(Duration(seconds: 5), () => true);

then all asynchronous code running during those five seconds would see the flag set and generate readable names. Enabling readable names for your program would be a one-liner:

unawaited(() async { assert(await Completer<bool>().future); }());

After that, all code see the flag as set, forever. Will users use that? Probably. It still only works when assertions are enabled, so I bet some people will think it's perfectly reasonable to enable readable names all the time during development. It might not be a problem, because you do have to go out of your way to do so, so at least you are aware that names are not normally readable.

rakudrama commented 3 years ago

There are two issues here, and I'm not sure which one the suggestions are trying to address:

  1. The user may mistakenly think that Type.toString returns a usable value.
  2. Reducing our cost diagnosing a report of strange behaviour when (1) happens.

To address user hints:

We can't protect against a motivated opponent. We can leave breadcrumbs that will make the average user question that the result is not 'stable'. It is important that all the implementations do this, so that a program developed on the VM or DDC gives the hints. Any change we make to dart2js is too late in the development cycle. We probably don't need to change dart2js at all, since it already has a 'obfuscate type strings' mode in minified mode.

I think we should do something simple that does not hinder debugging, even if the user has no debugger or some other non-standard environment. I don't think the 'assert-context' idea is simple.

I had suggested inserting randomly positioned zero-width spaces in the result. This is somewhat diabolical, but perhaps we could do something visible. At each call to Type.toString we could append a call counter, so '$String' returns 'String (123)' or 'String₁₂₃'. The runtime libraries for type checks can call an internal API to avoid putting this junk in Error messages.

To reduce cost of diagnosing:

We already have the --no-minify flag. If adding --no-minify that makes a program run, there must be a dependency on the unminified names. The flutter environment needs to make that experiment easy for the user to do. The unminified names do not always match the source names exactly. If two libraries each have a class Foo, dart2js uses the names Foo and Foo0 so that it can have a single runtime namespace.

If we want a flag to log calls to Type.toString, we can add a hook for that. The flag can be defined by the environment mechanism. -Dlog_type_tostring=true. Again, if we want the user to perform the experiment, we need to make simple from their environment. We can provide a hook for logging the types, with a default of calling print, and Flutter and override that hook.

lrhn commented 3 years ago

We are up against many years of accumulated code and practices.

One example is test(ClassName, () { ... }); which is in wide-spread use, at least inside Google. (I've argued against it before, because we never promised the content of Type.toString(). If only test would only accept strings as names, then that would come to a quick end ... or be replaced by "$ClassName".) Or other test uses of Type.toString, we even still have some in the SDK: https://github.com/dart-lang/sdk/issues/31054

Another is code parsing Type.toString to figure out the type of lists/maps or similar. I know they used to exist.

If changing it wasn't a breaking change, and likely an unpopular one, I'd have tried to do so long ago.

sigmundch commented 3 years ago

I like to explore what can we do to address the two pain points @rakudrama highlighted:

diagnosis: I'll start first with this one since it seems to be more actionable. We have several potential avenues for this already. I agree with @rakudrama that the most important one would be to enable users to debug issues. That would require:

Flutter changes:

@yjbanov - how feasible would it it be to do this?

Dart2js changes: consider changes to make diagnosis even easier. Ideas include:

I'd hold off and wait for these extra features though, since just having the ease to debug in flutter tools may be enough to start with.

Documentation changes:

prevention: we seem to have several breaking ideas in mind here, is there something partially breaking or non-breaking we can explore?

One idea I am considering is to move this functionality so that all uses are statically known. We could explore a breaking and non-breaking way to do so. For example:

extension TypeExtension on Type {
   /// Returns an internal [String] representation that can be used to
   /// identify the source-level type declaration.
   /// 
   /// Please note that this representation is unstable and changes
   /// between different implementations and even between multiple
   /// compilations of an application over time. This is provided
   /// for debugging purposes, and the result shouldn't
   /// be used for any semantics in the program (such as taking a branch
   /// depending on the value returned here).
   ///
   /// For interface types, this representation is a name that maps one-to-one to
   /// the original class name that defines the type. During development, most implementations
   /// return directly the original class name. However, in programs compiled
   /// for production (for example in dart2js with minification), this will
   /// return an minified name instead. In those environments it is still
   /// possible to lookup the original name outside the application using
   /// debug information produced by the compilers (for example, in the case
   /// of dart2js, with source-map files).
   /// ...
   external String get unstableInternalRepresentationForDebugOnly;
}

We can even automatically fix all statically known usages of Type.toString.

Other non-breaking ideas:

leafpetersen commented 3 years ago

There was discussion in the "Debug only code" document from @vsmenon about adding language support for code which is only reachable from debug contexts. There are different ways we could do it, but one proposal was something like this:

Would this proposal address this issue as well? You could make Type.toString a debug only override of Object.toString.

There was an additional suggestion that we could generalize this to arbitrary domains, e.g. indexed by constants so that debug(foo) defines a slice of the program indexed by foo which is separate from a slice with a different index.

@munificent @jakemac53 not sure whether macros could cover some or all of this?

sigmundch commented 3 years ago

Would this proposal address this issue as well? You could make Type.toString a debug only override of Object.toString

I don't believe so - we've seen many cases where developers need to log error messages in production that include Type.toString. These log messages are deobfuscated server-side with the help of debug/source-map auxiliary data.

leafpetersen commented 3 years ago

I don't believe so - we've seen many cases where developers need to log error messages in production that include Type.toString.

Couldn't you have a non-debug superclass method that logs the obfuscated type info, with a debug method actually on Type that logs the non-obfuscated info? That way the debug user sees clear messages but is prevented from using them in non-debug code, and the prod user sees garbage that can be de-obfuscated?

leafpetersen commented 3 years ago

To clarify what the semantics I think you would get are, here's a rough sketch:

class Object {
  String toString() => "Object"; // Not really, but whatever.
}
class _Type  {
  String toString() => mangledTypeName;
}
class Type extends _Type {
  debug String toString() => usefulTypeName;
}
void test(Type t) {
  t.toString(); // Static error
  (t as Object).toString);  // Ok, prints mangled name
  debug t.toString(); // Ok prints useful information 
}

You probably do want a way to conditionally dispatch on whether you're in debug mode so that you can print out the useful information if it's available.

void printUsefulErrorMessage(Type t) {
  if (debug) {
    print(t.toString());
  } else {
    print((t as Object).toString());
  }
}
sigmundch commented 3 years ago

Thanks for the clarification, that helps. The idea of "mangling" debug members is an interesting piece.

I'll be honest, the fact that (t as Object).toString(); works is a bit surprising. It makes me wonder what are the semantics of debug (t as Object).toString()? Would that hit the debug version or the non-debug version?

What is unfortunate here is that this breaks common patterns for logging error messages:

log('invalid ${o.runtimeType}, expected $Foo or $Baz');

Up until now static extension methods don't shadow instance methods, what if our debug contexts give us a mechanism to do so? For example:

class Type {
   String toString(); // always obfuscated
}
class TypeExtension {
   debug String toString(); // readable debug version, will shadow Type.toString in debug contexts
}

Then debug t.toString() calls the one in TypeExtension, whereas t.toString() alone calls the one in Type?

lrhn commented 3 years ago

Anything that requires debug e to enable different behavior has the same issues as using assert. It should encapsulate the entire computation, not just the local expression. Otherwise it prevents abstraction, you can't use String typeString(Object o) => o.runtimeType.toString() as a helper if the toString call must lexically be in the debug context. Not sure you can even do "$typeObject" or print(typeObject) unless we special case those.

If it is the entire computation, it should also work with asynchrony, and then we're back to using, and introducing, zones. (Well, unless we say that you can't await inside a debug, but that sounds like an uncomfortable restriction).

Having the toString change behavior when asserts are enabled ("debug mode") is something we can do already.

  String toString() {
    bool debug = false;
    assert(debug = true);
    if (!debug) return super.toString();
    // current impl.
  }

Maybe we should try it, and see how much breaks.

leafpetersen commented 3 years ago

@sigmundch

I'll be honest, the fact that (t as Object).toString(); works is a bit surprising. It makes me wonder what are the semantics of debug (t as Object).toString()? Would that hit the debug version or the non-debug version?

non-debug version. It's just name-mangling based on the static type. You're in some sense not actually overriding toString, you're introducing a new method named something like __debug__Type__toString, and in debug contexts, when you see .toString on a receiver of type Type, you treat it as a call to the latter.

What is unfortunate here is that this breaks common patterns for logging error messages:

log('invalid ${o.runtimeType}, expected $Foo or $Baz');

Yes, this would presumably print the Object version.

class Type {
   String toString(); // always obfuscated
}
class TypeExtension {
   debug String toString(); // readable debug version, will shadow Type.toString in debug contexts
}

Then debug t.toString() calls the one in TypeExtension, whereas t.toString() alone calls the one in Type?

Maybe, though it's a bit surprising. There are alternatives if we remove the special static error when calling a debug method in a non-debug context. That is, calling a debug static method would still be an error (because there the method doesn't exist in non-debug mode), but calling a debug method which overrides a non-debug method is fine, because the method exists in non-debug mode (you just hit a different virtual method). Then you have two choices of semantics for what happens in debug mode:

The former is possibly the most useful - it gives you meaningful output in your logging code, for example - but also possibly surprising.

@lrhn

It should encapsulate the entire computation, not just the local expression.

This would be a pretty different feature.

Otherwise it prevents abstraction, you can't use String typeString(Object o) => o.runtimeType.toString() as a helper if the toString call must lexically be in the debug context.

Sure you can, you just have to write it as debug String typeString(Object o) => o.runtimeType.toString(). You are blocked from using your helper in a non-debug context, of course, but that's the same as async, no?

Not sure you can even do "$typeObject" or print(typeObject) unless we special case those.

These would be fine, since they can be interpreted to access it through the Object interface.

 String toString() {
    bool debug = false;
    assert(debug = true);
    if (!debug) return super.toString();
    // current impl.
  }

This basically gives you the semantics of the first option I propose above, where you are allowed to call a debug method from non-debug code when it overrides a non-debug method, and you will hit the debug method when running in debug mode, and the release method when running in release mode. Confusing, but that's debug only code for you... :(

munificent commented 3 years ago

One example is test(ClassName, () { ... }); which is in wide-spread use, at least inside Google. (I've argued against it before, because we never promised the content of Type.toString(). If only test would only accept strings as names, then that would come to a quick end.)

test() used to only take strings as descriptions. We expanded to Object explicitly to support the pattern of passing a class reference the description. That way, if you refactor the class name, it gets fixed in test descriptions too. Stuffing source code identifiers into string literals makes them opaque to automated refactoring.

natebosch commented 3 years ago

That way, if you refactor the class name, it gets fixed in test descriptions too. Stuffing source code identifiers into string literals makes them opaque to automated refactoring.

A nameof feature would be more generally useful and maintain this safety. https://github.com/dart-lang/language/issues/251