From d27ad0d1822ceb709cf0d87eb49bce8bc9c2d70d Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Mon, 15 Mar 2021 11:42:54 -0700 Subject: [PATCH] fix: revert "refactor: mmap asar files (#24470)" (#28137) This reverts commit 01a2e23194f86df1d0dcc4a3ccb3ea37e8f8c98b. --- lib/asar/fs-wrapper.ts | 49 +++++------- shell/common/api/electron_api_asar.cc | 72 ++---------------- shell/common/asar/archive.cc | 86 +++++++++++++--------- shell/common/asar/archive.h | 8 +- shell/common/asar/scoped_temporary_file.cc | 23 ++++++ shell/common/asar/scoped_temporary_file.h | 6 ++ typings/internal-ambient.d.ts | 3 +- 7 files changed, 114 insertions(+), 133 deletions(-) diff --git a/lib/asar/fs-wrapper.ts b/lib/asar/fs-wrapper.ts index 569e19ff7fc..5f0d0587a31 100644 --- a/lib/asar/fs-wrapper.ts +++ b/lib/asar/fs-wrapper.ts @@ -532,15 +532,17 @@ export const wrapFsWithAsar = (fs: Record) => { return fs.readFile(realPath, options, callback); } + const buffer = Buffer.alloc(info.size); + const fd = archive.getFd(); + if (!(fd >= 0)) { + const error = createError(AsarError.NOT_FOUND, { asarPath, filePath }); + nextTick(callback, [error]); + return; + } + logASARAccess(asarPath, filePath, info.offset); - archive.read(info.offset, info.size).then((buf) => { - const buffer = Buffer.from(buf); - callback(null, encoding ? buffer.toString(encoding) : buffer); - }, (err) => { - const error: AsarErrorObject = new Error(`EINVAL, ${err.message} while reading ${filePath} in ${asarPath}`); - error.code = 'EINVAL'; - error.errno = -22; - callback(error); + fs.read(fd, buffer, 0, info.size, info.offset, (error: Error) => { + callback(error, encoding ? buffer.toString(encoding) : buffer); }); }; @@ -573,19 +575,13 @@ export const wrapFsWithAsar = (fs: Record) => { } const { encoding } = options; + const buffer = Buffer.alloc(info.size); + const fd = archive.getFd(); + if (!(fd >= 0)) throw createError(AsarError.NOT_FOUND, { asarPath, filePath }); logASARAccess(asarPath, filePath, info.offset); - let arrayBuffer: ArrayBuffer; - try { - arrayBuffer = archive.readSync(info.offset, info.size); - } catch (err) { - const error: AsarErrorObject = new Error(`EINVAL, ${err.message} while reading ${filePath} in ${asarPath}`); - error.code = 'EINVAL'; - error.errno = -22; - throw error; - } - const buffer = Buffer.from(arrayBuffer); - return encoding ? buffer.toString(encoding) : buffer; + fs.readSync(fd, buffer, 0, info.size, info.offset); + return (encoding) ? buffer.toString(encoding) : buffer; }; const { readdir } = fs; @@ -697,17 +693,12 @@ export const wrapFsWithAsar = (fs: Record) => { return [str, str.length > 0]; } + const buffer = Buffer.alloc(info.size); + const fd = archive.getFd(); + if (!(fd >= 0)) return []; + logASARAccess(asarPath, filePath, info.offset); - let arrayBuffer: ArrayBuffer; - try { - arrayBuffer = archive.readSync(info.offset, info.size); - } catch (err) { - const error: AsarErrorObject = new Error(`EINVAL, ${err.message} while reading ${filePath} in ${asarPath}`); - error.code = 'EINVAL'; - error.errno = -22; - throw error; - } - const buffer = Buffer.from(arrayBuffer); + fs.readSync(fd, buffer, 0, info.size, info.offset); const str = buffer.toString('utf8'); return [str, str.length > 0]; }; diff --git a/shell/common/api/electron_api_asar.cc b/shell/common/api/electron_api_asar.cc index b72c9304e02..d449e29db79 100644 --- a/shell/common/api/electron_api_asar.cc +++ b/shell/common/api/electron_api_asar.cc @@ -4,7 +4,6 @@ #include -#include "base/numerics/safe_math.h" #include "gin/handle.h" #include "gin/object_template_builder.h" #include "gin/wrappable.h" @@ -13,9 +12,6 @@ #include "shell/common/gin_converters/callback_converter.h" #include "shell/common/gin_converters/file_path_converter.h" #include "shell/common/gin_helper/dictionary.h" -#include "shell/common/gin_helper/error_thrower.h" -#include "shell/common/gin_helper/function_template_extensions.h" -#include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" #include "shell/common/node_util.h" @@ -42,8 +38,7 @@ class Archive : public gin::Wrappable { .SetMethod("readdir", &Archive::Readdir) .SetMethod("realpath", &Archive::Realpath) .SetMethod("copyFileOut", &Archive::CopyFileOut) - .SetMethod("read", &Archive::Read) - .SetMethod("readSync", &Archive::ReadSync); + .SetMethod("getFd", &Archive::GetFD); } const char* GetTypeName() override { return "Archive"; } @@ -109,68 +104,15 @@ class Archive : public gin::Wrappable { return gin::ConvertToV8(isolate, new_path); } - v8::Local ReadSync(gin_helper::ErrorThrower thrower, - uint64_t offset, - uint64_t length) { - base::CheckedNumeric safe_offset(offset); - base::CheckedNumeric safe_end = safe_offset + length; - if (!safe_end.IsValid() || - safe_end.ValueOrDie() > archive_->file()->length()) { - thrower.ThrowError("Out of bounds read"); - return v8::Local(); - } - auto array_buffer = v8::ArrayBuffer::New(thrower.isolate(), length); - auto backing_store = array_buffer->GetBackingStore(); - memcpy(backing_store->Data(), archive_->file()->data() + offset, length); - return array_buffer; - } - - v8::Local Read(v8::Isolate* isolate, - uint64_t offset, - uint64_t length) { - gin_helper::Promise> promise(isolate); - v8::Local handle = promise.GetHandle(); - - base::CheckedNumeric safe_offset(offset); - base::CheckedNumeric safe_end = safe_offset + length; - if (!safe_end.IsValid() || - safe_end.ValueOrDie() > archive_->file()->length()) { - promise.RejectWithErrorMessage("Out of bounds read"); - return handle; - } - - auto backing_store = v8::ArrayBuffer::NewBackingStore(isolate, length); - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE}, - base::BindOnce(&Archive::ReadOnIO, isolate, archive_, - std::move(backing_store), offset, length), - base::BindOnce(&Archive::ResolveReadOnUI, std::move(promise))); - - return handle; + // Return the file descriptor. + int GetFD() const { + if (!archive_) + return -1; + return archive_->GetFD(); } private: - static std::unique_ptr ReadOnIO( - v8::Isolate* isolate, - std::shared_ptr archive, - std::unique_ptr backing_store, - uint64_t offset, - uint64_t length) { - memcpy(backing_store->Data(), archive->file()->data() + offset, length); - return backing_store; - } - - static void ResolveReadOnUI( - gin_helper::Promise> promise, - std::unique_ptr backing_store) { - v8::HandleScope scope(promise.isolate()); - v8::Context::Scope context_scope(promise.GetContext()); - auto array_buffer = - v8::ArrayBuffer::New(promise.isolate(), std::move(backing_store)); - promise.Resolve(array_buffer); - } - - std::shared_ptr archive_; + std::unique_ptr archive_; DISALLOW_COPY_AND_ASSIGN(Archive); }; diff --git a/shell/common/asar/archive.cc b/shell/common/asar/archive.cc index 06ba2c7e250..89c5a8d418c 100644 --- a/shell/common/asar/archive.cc +++ b/shell/common/asar/archive.cc @@ -117,51 +117,78 @@ bool FillFileInfoWithNode(Archive::FileInfo* info, } // namespace -Archive::Archive(const base::FilePath& path) : path_(path) { +Archive::Archive(const base::FilePath& path) + : path_(path), file_(base::File::FILE_OK) { base::ThreadRestrictions::ScopedAllowIO allow_io; - if (base::PathExists(path_) && !file_.Initialize(path_)) { - LOG(ERROR) << "Failed to open ASAR archive at '" << path_.value() << "'"; - } + file_.Initialize(path_, base::File::FLAG_OPEN | base::File::FLAG_READ); +#if defined(OS_WIN) + fd_ = _open_osfhandle(reinterpret_cast(file_.GetPlatformFile()), 0); +#elif defined(OS_POSIX) + fd_ = file_.GetPlatformFile(); +#endif } -Archive::~Archive() {} +Archive::~Archive() { +#if defined(OS_WIN) + if (fd_ != -1) { + _close(fd_); + // Don't close the handle since we already closed the fd. + file_.TakePlatformFile(); + } +#endif + base::ThreadRestrictions::ScopedAllowIO allow_io; + file_.Close(); +} bool Archive::Init() { if (!file_.IsValid()) { + if (file_.error_details() != base::File::FILE_ERROR_NOT_FOUND) { + LOG(WARNING) << "Opening " << path_.value() << ": " + << base::File::ErrorToString(file_.error_details()); + } return false; } - if (file_.length() < 8) { - LOG(ERROR) << "Malformed ASAR file at '" << path_.value() - << "' (too short)"; + std::vector buf; + int len; + + buf.resize(8); + { + base::ThreadRestrictions::ScopedAllowIO allow_io; + len = file_.ReadAtCurrentPos(buf.data(), buf.size()); + } + if (len != static_cast(buf.size())) { + PLOG(ERROR) << "Failed to read header size from " << path_.value(); return false; } uint32_t size; - base::PickleIterator size_pickle( - base::Pickle(reinterpret_cast(file_.data()), 8)); - if (!size_pickle.ReadUInt32(&size)) { - LOG(ERROR) << "Failed to read header size at '" << path_.value() << "'"; + if (!base::PickleIterator(base::Pickle(buf.data(), buf.size())) + .ReadUInt32(&size)) { + LOG(ERROR) << "Failed to parse header size from " << path_.value(); return false; } - if (file_.length() - 8 < size) { - LOG(ERROR) << "Malformed ASAR file at '" << path_.value() - << "' (incorrect header)"; + buf.resize(size); + { + base::ThreadRestrictions::ScopedAllowIO allow_io; + len = file_.ReadAtCurrentPos(buf.data(), buf.size()); + } + if (len != static_cast(buf.size())) { + PLOG(ERROR) << "Failed to read header from " << path_.value(); return false; } - base::PickleIterator header_pickle( - base::Pickle(reinterpret_cast(file_.data() + 8), size)); std::string header; - if (!header_pickle.ReadString(&header)) { - LOG(ERROR) << "Failed to read header string at '" << path_.value() << "'"; + if (!base::PickleIterator(base::Pickle(buf.data(), buf.size())) + .ReadString(&header)) { + LOG(ERROR) << "Failed to parse header from " << path_.value(); return false; } base::Optional value = base::JSONReader::Read(header); if (!value || !value->is_dict()) { - LOG(ERROR) << "Header was not valid JSON at '" << path_.value() << "'"; + LOG(ERROR) << "Failed to parse header"; return false; } @@ -264,24 +291,11 @@ bool Archive::CopyFileOut(const base::FilePath& path, base::FilePath* out) { return true; } - base::CheckedNumeric safe_offset(info.offset); - auto safe_end = safe_offset + info.size; - if (!safe_end.IsValid() || safe_end.ValueOrDie() > file_.length()) - return false; - auto temp_file = std::make_unique(); base::FilePath::StringType ext = path.Extension(); - if (!temp_file->Init(ext)) + if (!temp_file->InitFromFile(&file_, ext, info.offset, info.size)) return false; - base::File dest(temp_file->path(), - base::File::FLAG_OPEN | base::File::FLAG_WRITE); - if (!dest.IsValid()) - return false; - - dest.WriteAtCurrentPos( - reinterpret_cast(file_.data() + info.offset), info.size); - #if defined(OS_POSIX) if (info.executable) { // chmod a+x temp_file; @@ -294,4 +308,8 @@ bool Archive::CopyFileOut(const base::FilePath& path, base::FilePath* out) { return true; } +int Archive::GetFD() const { + return fd_; +} + } // namespace asar diff --git a/shell/common/asar/archive.h b/shell/common/asar/archive.h index 86d84aa8e7f..d97cb03cc78 100644 --- a/shell/common/asar/archive.h +++ b/shell/common/asar/archive.h @@ -11,7 +11,6 @@ #include "base/files/file.h" #include "base/files/file_path.h" -#include "base/files/memory_mapped_file.h" namespace base { class DictionaryValue; @@ -62,13 +61,16 @@ class Archive { // For unpacked file, this method will return its real path. bool CopyFileOut(const base::FilePath& path, base::FilePath* out); - base::MemoryMappedFile* file() { return &file_; } + // Returns the file's fd. + int GetFD() const; + base::FilePath path() const { return path_; } base::DictionaryValue* header() const { return header_.get(); } private: base::FilePath path_; - base::MemoryMappedFile file_; + base::File file_; + int fd_ = -1; uint32_t header_size_ = 0; std::unique_ptr header_; diff --git a/shell/common/asar/scoped_temporary_file.cc b/shell/common/asar/scoped_temporary_file.cc index e51b9dad3b8..b9bc500f04f 100644 --- a/shell/common/asar/scoped_temporary_file.cc +++ b/shell/common/asar/scoped_temporary_file.cc @@ -48,4 +48,27 @@ bool ScopedTemporaryFile::Init(const base::FilePath::StringType& ext) { return true; } +bool ScopedTemporaryFile::InitFromFile(base::File* src, + const base::FilePath::StringType& ext, + uint64_t offset, + uint64_t size) { + if (!src->IsValid()) + return false; + + if (!Init(ext)) + return false; + + std::vector buf(size); + int len = src->Read(offset, buf.data(), buf.size()); + if (len != static_cast(size)) + return false; + + base::File dest(path_, base::File::FLAG_OPEN | base::File::FLAG_WRITE); + if (!dest.IsValid()) + return false; + + return dest.WriteAtCurrentPos(buf.data(), buf.size()) == + static_cast(size); +} + } // namespace asar diff --git a/shell/common/asar/scoped_temporary_file.h b/shell/common/asar/scoped_temporary_file.h index 15f65290a92..e42a244e122 100644 --- a/shell/common/asar/scoped_temporary_file.h +++ b/shell/common/asar/scoped_temporary_file.h @@ -27,6 +27,12 @@ class ScopedTemporaryFile { // Init an empty temporary file with a certain extension. bool Init(const base::FilePath::StringType& ext); + // Init an temporary file and fill it with content of |path|. + bool InitFromFile(base::File* src, + const base::FilePath::StringType& ext, + uint64_t offset, + uint64_t size); + base::FilePath path() const { return path_; } private: diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index 4acb5035712..c97d0864f16 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -76,8 +76,7 @@ declare namespace NodeJS { readdir(path: string): string[] | false; realpath(path: string): string | false; copyFileOut(path: string): string | false; - read(offset: number, size: number): Promise; - readSync(offset: number, size: number): ArrayBuffer; + getFd(): number | -1; } interface AsarBinding {