From 4de9e805ed64571fc5fb0d94ef77683ca8dc5caf Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Wed, 27 Oct 2021 16:34:00 +0000 Subject: [PATCH] [ CLI ] Don't resolve paths as URIs for known SDK artifacts Handles cases where the SDK is installed on a path which includes percent encoded characters. The OS doesn't decode these characters, so decoding them before trying to load a snapshot with said characters in their path will result in files not being found. Fixes https://github.com/dart-lang/sdk/issues/46364 TEST=pkg/dartdev/test/regress_46364_test.dart Change-Id: I5f78e8a2049cc0c83555528fcc8f41be946141f5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/218082 Reviewed-by: Ryan Macnak --- pkg/dartdev/pubspec.yaml | 2 ++ pkg/dartdev/test/regress_46364_test.dart | 40 ++++++++++++++++++++++++ runtime/bin/dfe.cc | 20 +++++++----- runtime/bin/dfe.h | 6 ++-- runtime/bin/main.cc | 16 +++++----- runtime/bin/snapshot_utils.cc | 18 +++++++---- runtime/bin/snapshot_utils.h | 3 +- 7 files changed, 82 insertions(+), 23 deletions(-) create mode 100644 pkg/dartdev/test/regress_46364_test.dart diff --git a/pkg/dartdev/pubspec.yaml b/pkg/dartdev/pubspec.yaml index 9fbdb4bafde..0145fab20d3 100644 --- a/pkg/dartdev/pubspec.yaml +++ b/pkg/dartdev/pubspec.yaml @@ -34,6 +34,8 @@ dependencies: usage: any dev_dependencies: + expect: + path: ../expect lints: any pub_semver: any test: any diff --git a/pkg/dartdev/test/regress_46364_test.dart b/pkg/dartdev/test/regress_46364_test.dart new file mode 100644 index 00000000000..09edc21be54 --- /dev/null +++ b/pkg/dartdev/test/regress_46364_test.dart @@ -0,0 +1,40 @@ +// Copyright (c) 2021, 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. + +import 'dart:io'; + +import 'package:expect/expect.dart'; +import 'package:path/path.dart' as p; + +// Copied from package:io +Future copyPath(String from, String to) async { + await Directory(to).create(recursive: true); + await for (final file in Directory(from).list(recursive: true)) { + final copyTo = p.join(to, p.relative(file.path, from: from)); + if (file is Directory) { + await Directory(copyTo).create(recursive: true); + } else if (file is File) { + await File(file.path).copy(copyTo); + } else if (file is Link) { + await Link(copyTo).create(await file.target(), recursive: true); + } + } +} + +Future main() async { + final exePath = Platform.resolvedExecutable; + final sdkDir = p.dirname(p.dirname(exePath)); + // Try to run the VM located on a path with % encoded characters. The VM + // should not try and resolve the path as a URI for SDK artifacts (e.g., + // dartdev.dart.snapshot). + final d = Directory.systemTemp.createTempSync('dart_symlink%3A'); + try { + await copyPath(sdkDir, d.path); + final path = '${d.path}/bin/dart'; + final result = await Process.run(path, ['help']); + Expect.equals(result.exitCode, 0); + } finally { + await d.delete(recursive: true); + } +} diff --git a/runtime/bin/dfe.cc b/runtime/bin/dfe.cc index b6c27fa9ddc..701967b45f4 100644 --- a/runtime/bin/dfe.cc +++ b/runtime/bin/dfe.cc @@ -231,9 +231,11 @@ void DFE::CompileAndReadScript(const char* script_uri, void DFE::ReadScript(const char* script_uri, uint8_t** kernel_buffer, - intptr_t* kernel_buffer_size) const { + intptr_t* kernel_buffer_size, + bool decode_uri) const { int64_t start = Dart_TimelineGetMicros(); - if (!TryReadKernelFile(script_uri, kernel_buffer, kernel_buffer_size)) { + if (!TryReadKernelFile(script_uri, kernel_buffer, kernel_buffer_size, + decode_uri)) { return; } if (!Dart_IsKernel(*kernel_buffer, *kernel_buffer_size)) { @@ -274,9 +276,12 @@ static bool TryReadSimpleKernelBuffer(uint8_t* buffer, /// /// If successful, newly allocated buffer with file contents is returned in /// [buffer], file contents byte count - in [size]. -static bool TryReadFile(const char* script_uri, uint8_t** buffer, - intptr_t* size) { - void* script_file = DartUtils::OpenFileUri(script_uri, false); +static bool TryReadFile(const char* script_uri, + uint8_t** buffer, + intptr_t* size, + bool decode_uri = true) { + void* script_file = decode_uri ? DartUtils::OpenFileUri(script_uri, false) + : DartUtils::OpenFile(script_uri, false); if (script_file == nullptr) { return false; } @@ -417,12 +422,13 @@ static bool TryReadKernelListBuffer(const char* script_uri, bool DFE::TryReadKernelFile(const char* script_uri, uint8_t** kernel_ir, - intptr_t* kernel_ir_size) { + intptr_t* kernel_ir_size, + bool decode_uri) { *kernel_ir = nullptr; *kernel_ir_size = -1; uint8_t* buffer; - if (!TryReadFile(script_uri, &buffer, kernel_ir_size)) { + if (!TryReadFile(script_uri, &buffer, kernel_ir_size, decode_uri)) { return false; } diff --git a/runtime/bin/dfe.h b/runtime/bin/dfe.h index 649cc321b11..038bca755b1 100644 --- a/runtime/bin/dfe.h +++ b/runtime/bin/dfe.h @@ -92,7 +92,8 @@ class DFE { // valid kernel file, false otherwise. void ReadScript(const char* script_uri, uint8_t** kernel_buffer, - intptr_t* kernel_buffer_size) const; + intptr_t* kernel_buffer_size, + bool decode_uri = true) const; bool KernelServiceDillAvailable() const; @@ -103,7 +104,8 @@ class DFE { // was returned. static bool TryReadKernelFile(const char* script_uri, uint8_t** kernel_buffer, - intptr_t* kernel_buffer_size); + intptr_t* kernel_buffer_size, + bool decode_uri = true); // We distinguish between "intent to use Dart frontend" vs "can actually // use Dart frontend". The method UseDartFrontend tells us about the diff --git a/runtime/bin/main.cc b/runtime/bin/main.cc index 64f17153323..41939659054 100644 --- a/runtime/bin/main.cc +++ b/runtime/bin/main.cc @@ -431,8 +431,9 @@ static Dart_Isolate CreateAndSetupKernelIsolate(const char* script_uri, AppSnapshot* app_snapshot = NULL; // Kernel isolate uses an app snapshot or uses the dill file. if ((kernel_snapshot_uri != NULL) && - (app_snapshot = Snapshot::TryReadAppSnapshot(kernel_snapshot_uri)) != - NULL) { + (app_snapshot = Snapshot::TryReadAppSnapshot( + kernel_snapshot_uri, /*force_load_elf_from_memory=*/false, + /*decode_uri=*/false)) != nullptr) { const uint8_t* isolate_snapshot_data = NULL; const uint8_t* isolate_snapshot_instructions = NULL; const uint8_t* ignore_vm_snapshot_data; @@ -603,8 +604,9 @@ static Dart_Isolate CreateAndSetupDartDevIsolate(const char* script_uri, AppSnapshot* app_snapshot = nullptr; bool isolate_run_app_snapshot = true; if (dartdev_path.get() != nullptr && - (app_snapshot = Snapshot::TryReadAppSnapshot(dartdev_path.get())) != - nullptr) { + (app_snapshot = Snapshot::TryReadAppSnapshot( + dartdev_path.get(), /*force_load_elf_from_memory=*/false, + /*decode_uri=*/false)) != nullptr) { const uint8_t* isolate_snapshot_data = NULL; const uint8_t* isolate_snapshot_instructions = NULL; const uint8_t* ignore_vm_snapshot_data; @@ -613,8 +615,8 @@ static Dart_Isolate CreateAndSetupDartDevIsolate(const char* script_uri, &ignore_vm_snapshot_data, &ignore_vm_snapshot_instructions, &isolate_snapshot_data, &isolate_snapshot_instructions); isolate_group_data = - new IsolateGroupData(dartdev_path.get(), packages_config, app_snapshot, - isolate_run_app_snapshot); + new IsolateGroupData(DART_DEV_ISOLATE_NAME, packages_config, + app_snapshot, isolate_run_app_snapshot); isolate_data = new IsolateData(isolate_group_data); isolate = Dart_CreateIsolateGroup( DART_DEV_ISOLATE_NAME, DART_DEV_ISOLATE_NAME, isolate_snapshot_data, @@ -642,7 +644,7 @@ static Dart_Isolate CreateAndSetupDartDevIsolate(const char* script_uri, uint8_t* application_kernel_buffer = NULL; intptr_t application_kernel_buffer_size = 0; dfe.ReadScript(dartdev_path.get(), &application_kernel_buffer, - &application_kernel_buffer_size); + &application_kernel_buffer_size, /*decode_uri=*/false); isolate_group_data->SetKernelBufferNewlyOwned( application_kernel_buffer, application_kernel_buffer_size); diff --git a/runtime/bin/snapshot_utils.cc b/runtime/bin/snapshot_utils.cc index cf9589a46ed..754043ac3ce 100644 --- a/runtime/bin/snapshot_utils.cc +++ b/runtime/bin/snapshot_utils.cc @@ -330,13 +330,19 @@ static AppSnapshot* TryReadAppSnapshotDynamicLibrary(const char* script_name) { #endif // defined(DART_PRECOMPILED_RUNTIME) AppSnapshot* Snapshot::TryReadAppSnapshot(const char* script_uri, - bool force_load_elf_from_memory) { - auto decoded_path = File::UriToPath(script_uri); - if (decoded_path == nullptr) { - return nullptr; + bool force_load_elf_from_memory, + bool decode_uri) { + Utils::CStringUniquePtr decoded_path(nullptr, std::free); + const char* script_name = nullptr; + if (decode_uri) { + decoded_path = File::UriToPath(script_uri); + if (decoded_path == nullptr) { + return nullptr; + } + script_name = decoded_path.get(); + } else { + script_name = script_uri; } - - const char* script_name = decoded_path.get(); if (File::GetType(nullptr, script_name, true) != File::kIsFile) { // If 'script_name' refers to a pipe, don't read to check for an app // snapshot since we cannot rewind if it isn't (and couldn't mmap it in diff --git a/runtime/bin/snapshot_utils.h b/runtime/bin/snapshot_utils.h index 57d892955f3..ed96fb12035 100644 --- a/runtime/bin/snapshot_utils.h +++ b/runtime/bin/snapshot_utils.h @@ -41,7 +41,8 @@ class Snapshot { static AppSnapshot* TryReadAppendedAppSnapshotElf(const char* container_path); static AppSnapshot* TryReadAppSnapshot( const char* script_uri, - bool force_load_elf_from_memory = false); + bool force_load_elf_from_memory = false, + bool decode_uri = true); static void WriteAppSnapshot(const char* filename, uint8_t* vm_data_buffer, intptr_t vm_data_size,