[test.dart] Complain if there is non-utf8 formatted data in test output

tools/testing/dart/test_runner.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Classes and methods for executing tests.
  *
  * This module includes:
  * - Managing parallel execution of tests, including timeout checks.
  * - Evaluating the output of each test as pass/fail/crash/timeout.
@@ skipping 14 matching lines @@
 import "path.dart";
 import 'record_and_replay.dart';
 import "runtime_configuration.dart";
 import "status_file_parser.dart";
 import "test_progress.dart";
 import "test_suite.dart";
 import "utils.dart";
 
 const int CRASHING_BROWSER_EXITCODE = -10;
 const int SLOW_TIMEOUT_MULTIPLIER = 4;
+const int NON_UTF_FAKE_EXITCODE = 0xFFFD;
 
 const MESSAGE_CANNOT_OPEN_DISPLAY = 'Gtk-WARNING **: cannot open display';
 const MESSAGE_FAILED_TO_RUN_COMMAND = 'Failed to run command. return code=1';
 
 typedef void TestCaseEvent(TestCase testCase);
 typedef void ExitCodeEvent(int exitCode);
 typedef void EnqueueMoreWork(ProcessQueue queue);
 
 // Some IO tests use these variables and get confused if the host environment
 // variables are inherited so they are excluded.
@@ skipping 932 matching lines @@
       List<int> this.stderr,
       Duration this.time,
       bool this.compilationSkipped,
       int this.pid) {
     diagnostics = [];
   }
 
   Expectation result(TestCase testCase) {
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) return Expectation.TIMEOUT;
-    return hasFailed(testCase) ? Expectation.FAIL : Expectation.PASS;
+    if (hasFailed(testCase)) return Expectation.FAIL;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
+    return Expectation.PASS;
   }
 
   bool get hasCrashed {
     // dart2js exits with code 253 in case of unhandled exceptions.
     // The dart binary exits with code 253 in case of an API error such
     // as an invalid snapshot file.
     // In either case an exit code of 253 is considered a crash.
     if (exitCode == 253) return true;
     if (io.Platform.operatingSystem == 'windows') {
       // The VM uses std::abort to terminate on asserts.
@@ skipping 26 matching lines @@
   bool get successful {
     // FIXME(kustermann): We may need to change this
     return !hasTimedOut && exitCode == 0;
   }
 
   // Reverse result of a negative test.
   bool hasFailed(TestCase testCase) {
     return testCase.isNegative ? !didFail(testCase) : didFail(testCase);
   }
 
+  bool get hasNonUtf8 => exitCode == NON_UTF_FAKE_EXITCODE;
+
   Expectation _negateOutcomeIfNegativeTest(
       Expectation outcome, bool isNegative) {
     if (!isNegative) return outcome;
     if (outcome == Expectation.IGNORE) return outcome;
     if (outcome.canBeOutcomeOf(Expectation.FAIL)) {
       return Expectation.PASS;
     }
     return Expectation.FAIL;
   }
 }
@@ skipping 35 matching lines @@
   BrowserCommandOutputImpl(
       command, exitCode, timedOut, stdout, stderr, time, compilationSkipped)
       : super(command, exitCode, timedOut, stdout, stderr, time,
             compilationSkipped, 0),
         _infraFailure = _failedBecauseOfFlakyInfrastructure(stderr);
 
   Expectation result(TestCase testCase) {
     // Handle crashes and timeouts first
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) return Expectation.TIMEOUT;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     if (_infraFailure) {
       return Expectation.IGNORE;
     }
     var outcome = _getOutcome();
 
     if (testCase.hasRuntimeError) {
       if (!outcome.canBeOutcomeOf(Expectation.RUNTIME_ERROR)) {
         return Expectation.MISSING_RUNTIME_ERROR;
       }
@@ skipping 290 matching lines @@
       List<int> stdout,
       List<int> stderr)
       : super(command, 0, result.didTimeout, stdout, stderr, result.duration,
             false, 0) {
     _result = result;
   }
 
   Expectation result(TestCase testCase) {
     // Handle timeouts first
     if (_result.didTimeout) return Expectation.TIMEOUT;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     // Multitests are handled specially
     if (testCase.hasRuntimeError) {
       if (_rawOutcome == Expectation.RUNTIME_ERROR) return Expectation.PASS;
       return Expectation.MISSING_RUNTIME_ERROR;
     }
 
     return _negateOutcomeIfNegativeTest(_rawOutcome, testCase.isNegative);
   }
 }
@@ skipping 12 matching lines @@
             compilationSkipped, 0);
 
   Expectation result(TestCase testCase) {
     // TODO(kustermann): If we run the analyzer not in batch mode, make sure
     // that command.exitCodes matches 2 (errors), 1 (warnings), 0 (no warnings,
     // no errors)
 
     // Handle crashes and timeouts first
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) return Expectation.TIMEOUT;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     // Get the errors/warnings from the analyzer
     List<String> errors = [];
     List<String> warnings = [];
     parseAnalyzerOutput(errors, warnings);
 
     // Handle errors / missing errors
     if (testCase.expectCompileError) {
       if (errors.length > 0) {
         return Expectation.PASS;
@@ skipping 69 matching lines @@
 
   VmCommandOutputImpl(Command command, int exitCode, bool timedOut,
       List<int> stdout, List<int> stderr, Duration time, int pid)
       : super(command, exitCode, timedOut, stdout, stderr, time, false, pid);
 
   Expectation result(TestCase testCase) {
     // Handle crashes and timeouts first
     if (exitCode == DART_VM_EXITCODE_DFE_ERROR) return Expectation.DARTK_CRASH;
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) return Expectation.TIMEOUT;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     // Multitests are handled specially
     if (testCase.expectCompileError) {
       if (exitCode == DART_VM_EXITCODE_COMPILE_TIME_ERROR) {
         return Expectation.PASS;
       }
       return Expectation.MISSING_COMPILETIME_ERROR;
     }
     if (testCase.hasRuntimeError) {
       // TODO(kustermann): Do we consider a "runtimeError" only an uncaught
@@ skipping 40 matching lines @@
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) {
       bool isWindows = io.Platform.operatingSystem == 'windows';
       bool isBrowserTestCase =
           testCase.commands.any((command) => command is BrowserTestCommand);
       // TODO(26060) Dart2js batch mode hangs on Windows under heavy load.
       return (isWindows && isBrowserTestCase)
           ? Expectation.IGNORE
           : Expectation.TIMEOUT;
     }
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     // Handle dart2js specific crash detection
     if (exitCode == DART2JS_EXITCODE_CRASH ||
         exitCode == VmCommandOutputImpl.DART_VM_EXITCODE_COMPILE_TIME_ERROR ||
         exitCode == VmCommandOutputImpl.DART_VM_EXITCODE_UNCAUGHT_EXCEPTION) {
       return Expectation.CRASH;
     }
 
     // Multitests are handled specially
     if (testCase.expectCompileError) {
@@ skipping 57 matching lines @@
 class JsCommandlineOutputImpl extends CommandOutputImpl
     with UnittestSuiteMessagesMixin {
   JsCommandlineOutputImpl(Command command, int exitCode, bool timedOut,
       List<int> stdout, List<int> stderr, Duration time)
       : super(command, exitCode, timedOut, stdout, stderr, time, false, 0);
 
   Expectation result(TestCase testCase) {
     // Handle crashes and timeouts first
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) return Expectation.TIMEOUT;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     if (testCase.hasRuntimeError) {
       if (exitCode != 0) return Expectation.PASS;
       return Expectation.MISSING_RUNTIME_ERROR;
     }
 
     var outcome = exitCode == 0 ? Expectation.PASS : Expectation.RUNTIME_ERROR;
     outcome = _negateOutcomeIfIncompleteAsyncTest(outcome, decodeUtf8(stdout));
     return _negateOutcomeIfNegativeTest(outcome, testCase.isNegative);
   }
 }
 
 class PubCommandOutputImpl extends CommandOutputImpl {
   PubCommandOutputImpl(PubCommand command, int exitCode, bool timedOut,
       List<int> stdout, List<int> stderr, Duration time)
       : super(command, exitCode, timedOut, stdout, stderr, time, false, 0);
 
   Expectation result(TestCase testCase) {
     // Handle crashes and timeouts first
     if (hasCrashed) return Expectation.CRASH;
     if (hasTimedOut) return Expectation.TIMEOUT;
+    if (hasNonUtf8) return Expectation.NON_UTF8_ERROR;
 
     if (exitCode == 0) {
       return Expectation.PASS;
     } else if ((command as PubCommand).command == 'get') {
       return Expectation.PUB_GET_ERROR;
     } else {
       return Expectation.FAIL;
     }
   }
 }
@@ skipping 61 matching lines @@
  * it is longer than MAX_HEAD characters, and just keeps the head and
  * the last TAIL_LENGTH characters of the output.
  */
 class OutputLog {
   static const int MAX_HEAD = 100 * 1024;
   static const int TAIL_LENGTH = 10 * 1024;
   List<int> head = <int>[];
   List<int> tail;
   List<int> complete;
   bool dataDropped = false;
+  bool hasNonUtf8 = false;
 
   OutputLog();
 
   void add(List<int> data) {
     if (complete != null) {
       throw new StateError("Cannot add to OutputLog after calling toList");
     }
     if (tail == null) {
       head.addAll(data);
       if (head.length > MAX_HEAD) {
         tail = head.sublist(MAX_HEAD);
         head.length = MAX_HEAD;
       }
     } else {
       tail.addAll(data);
     }
     if (tail != null && tail.length > 2 * TAIL_LENGTH) {
       tail = _truncatedTail();
       dataDropped = true;
     }
   }
 
   List<int> _truncatedTail() => tail.length > TAIL_LENGTH
       ? tail.sublist(tail.length - TAIL_LENGTH)
       : tail;
 
+
+  void _checkUtf8(List<int> data) {
+    try {
+      UTF8.decode(data, allowMalformed: false);
+    } on FormatException catch (e) {
+      hasNonUtf8 = true;
+      String malformed = UTF8.decode(data, allowMalformed: true);
+      data..clear()
+          ..addAll(malformed.codeUnits)

[kustermann, 2017/02/24 16:24:17]

The input 'List<int> data' here is a list of bytes (i.e. the entries are 0 <=
value <= 255).

Here you add codeUnits, which are not bytes but rather UTF16 code points (as far
as I know).

The places where this list is used use UTF8.decode(data) which assumes [data]
contains only bytes / 8-bit values.


(Maybe this should have been written using a BytesBuilder and Utf8List instead
to make this more clear and avoid having these growable Lists with Smis in
them.)

[kustermann, 2017/02/25 11:37:48]

To clarify, what I mean is changing:

  data..clear()..addAll(malformed.codeUnits)

to 

  data..clear()..addAll(UTF8.encode(malformed))


Because later on code will do

UTF8.decode(testCase.lastCommandOutput.stdout /* = data */ )

and UTF8.decode() takes only bytes, not UTF16 code points.

[zra, 2017/02/27 19:05:58]

Making this fix here: https://codereview.chromium.org/2721683002/

+          ..addAll("""
+
+  *****************************************************************************
+
+  test.dart: The output of this test contained non-UTF8 formatted data.
+
+  *****************************************************************************
+
+  """
+          .codeUnits);
+    }
+  }
+
+
   List<int> toList() {
     if (complete == null) {
       complete = head;
       if (dataDropped) {
         complete.addAll("""
 
 *****************************************************************************
 
-Data removed due to excessive length
+test.dart: Data was removed due to excessive length
 
 *****************************************************************************
 
 """
             .codeUnits);
         complete.addAll(_truncatedTail());
       } else if (tail != null) {
         complete.addAll(tail);
       }
       head = null;
       tail = null;
+      _checkUtf8(complete);
     }
     return complete;
   }
 }
 
 // Helper to get a list of all child pids for a parent process.
 // The first element of the list is the parent pid.
 Future<List<int>> _getPidList(pid, diagnostics) async {
   var pid_list = [pid];
   var lines;
@@ skipping 214 matching lines @@
 
   void _commandComplete(int exitCode) {
     if (timeoutTimer != null) {
       timeoutTimer.cancel();
     }
     var commandOutput = _createCommandOutput(command, exitCode);
     completer.complete(commandOutput);
   }
 
   CommandOutput _createCommandOutput(ProcessCommand command, int exitCode) {
+    List<int> stdoutData = stdout.toList();
+    List<int> stderrData = stderr.toList();
+    if (stdout.hasNonUtf8 || stderr.hasNonUtf8) {
+      // If the output contained non-utf8 formatted data, then make the exit
+      // code non-zero if it isn't already.
+      if (exitCode == 0) {
+        exitCode = NON_UTF_FAKE_EXITCODE;
+      }
+    }
     var commandOutput = createCommandOutput(
         command,
         exitCode,
         timedOut,
-        stdout.toList(),
-        stderr.toList(),
+        stdoutData,
+        stderrData,
         new DateTime.now().difference(startTime),
         compilationSkipped,
         pid);
     commandOutput.diagnostics.addAll(diagnostics);
     return commandOutput;
   }
 
   StreamSubscription _drainStream(
       Stream<List<int>> source, OutputLog destination) {
     return source.listen(destination.add);
@@ skipping 1257 matching lines @@
     }
   }
 
   void eventAllTestsDone() {
     for (var listener in _eventListener) {
       listener.allDone();
     }
     _allDone();
   }
 }