Changeset 257134 in webkit


Ignore:
Timestamp:
Feb 21, 2020, 8:20:55 AM (5 years ago)
Author:
mark.lam@apple.com
Message:

Make support for bytecode caching more robust against file corruption.
​https://bugs.webkit.org/show_bug.cgi?id=207972
<rdar://problem/59260595>

Reviewed by Yusuke Suzuki.

If a bytecode cache file is corrupted, we currently will always crash every time
we try to read it (in perpetuity as long as the corrupted cache file continues to
exist on disk). To guard against this, we'll harden the bytecode caching mechanism
as follows:

  1. Modify the writeCache operation to always write the cache file in a transactional manner i.e. we'll first write to a .tmp file, and then rename the .tmp file to the cache file only if the entire file has been written in completeness.

This ensures that we won't get corrupted cache files due to interrupted writes.

  1. Modify the writeCache operation to also compute a SHA1 hash of the cache file and append the hash at end of the file. Modify the readCache operation to first authenticate the SHA1 hash before allowing the cache file to be used. If the hash does not match, the file is bad, and we'll just delete it.

This ensures that we won't be crashing while decoding a corrupted cache file.

Manually tested with the following scenarios and ensuring that the client recovers
with no crashes:

  1. no cache file on disk.
  2. a 0-sized cache file on a disk.
  3. a truncated cache file on disk.
  4. a corrupted cache file on disk.
  5. an uncorrupted cache file on disk.

Also added some static_asserts in CachedTypes.cpp to document some invariants that
the pre-existing code is dependent on.

  • API/JSScript.mm:

(-[JSScript readCache]):
(-[JSScript writeCache:]):

  • runtime/CachedTypes.cpp:
Location:
trunk/Source/JavaScriptCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/API/JSScript.mm

    r254152 r257134  
    11/*
    2  * Copyright (C) 2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2019-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4141#import <wtf/FileMetadata.h>
    4242#import <wtf/FileSystem.h>
     43#import <wtf/SHA1.h>
    4344#import <wtf/Scope.h>
    4445#import <wtf/WeakObjCPtr.h>
    … …  
    154155        return;
    155156
    156     auto fd = FileSystem::openAndLockFile([m_cachePath path].UTF8String, FileSystem::FileOpenMode::Read, {FileSystem::FileLockMode::Exclusive, FileSystem::FileLockMode::Nonblocking});
     157    NSString *cachePathString = [m_cachePath path];
     158    const char* cacheFilename = cachePathString.UTF8String;
     159
     160    auto fd = FileSystem::openAndLockFile(cacheFilename, FileSystem::FileOpenMode::Read, {FileSystem::FileLockMode::Exclusive, FileSystem::FileLockMode::Nonblocking});
    157161    if (!FileSystem::isHandleValid(fd))
    158162        return;
    … …  
    165169    if (!success)
    166170        return;
     171
     172    const uint8_t* fileData = reinterpret_cast<const uint8_t*>(mappedFile.data());
     173    unsigned fileTotalSize = mappedFile.size();
     174
     175    // Ensure we at least have a SHA1::Digest to read.
     176    if (fileTotalSize < sizeof(SHA1::Digest)) {
     177        FileSystem::deleteFile(cacheFilename);
     178        return;
     179    }
     180
     181    unsigned fileDataSize = fileTotalSize - sizeof(SHA1::Digest);
     182
     183    SHA1::Digest computedHash;
     184    SHA1 sha1;
     185    sha1.addBytes(fileData, fileDataSize);
     186    sha1.computeHash(computedHash);
     187
     188    SHA1::Digest fileHash;
     189    memcpy(&fileHash, fileData + fileDataSize, sizeof(SHA1::Digest));
     190
     191    if (computedHash != fileHash) {
     192        FileSystem::deleteFile(cacheFilename);
     193        return;
     194    }
    167195
    168196    Ref<JSC::CachedBytecode> cachedBytecode = JSC::CachedBytecode::create(WTFMove(mappedFile));
    … …  
    174202        m_cachedBytecode = WTFMove(cachedBytecode);
    175203    else
    176         ftruncate(fd, 0);
     204        FileSystem::truncateFile(fd, 0);
    177205}
    178206
    … …  
    267295    }
    268296
    269     int fd = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
     297    // We want to do the write as a transaction (i.e. we guarantee that it's all
     298    // or nothing). So, we'll write to a temp file first, and rename the temp
     299    // file to the cache file only after we've finished writing the whole thing.
     300
     301    NSString *cachePathString = [m_cachePath path];
     302    const char* cacheFileName = cachePathString.UTF8String;
     303    const char* tempFileName = [cachePathString stringByAppendingString:@".tmp"].UTF8String;
     304    int fd = open(cacheFileName, O_CREAT | O_WRONLY | O_EXLOCK | O_NONBLOCK, 0600);
    270305    if (fd == -1) {
    271306        error = makeString("Could not open or lock the bytecode cache file. It's likely another VM or process is already using it. Error: ", strerror(errno));
    272307        return NO;
    273308    }
     309
    274310    auto closeFD = makeScopeExit([&] {
    275311        close(fd);
     312    });
     313
     314    int tempFD = open(tempFileName, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0600);
     315    if (tempFD == -1) {
     316        error = makeString("Could not open or lock the bytecode cache temp file. Error: ", strerror(errno));
     317        return NO;
     318    }
     319
     320    auto closeTempFD = makeScopeExit([&] {
     321        close(tempFD);
    276322    });
    277323
    … …  
    281327    switch (m_type) {
    282328    case kJSScriptTypeModule:
    283         m_cachedBytecode = JSC::generateModuleBytecode(vm, sourceCode, fd, cacheError);
     329        m_cachedBytecode = JSC::generateModuleBytecode(vm, sourceCode, tempFD, cacheError);
    284330        break;
    285331    case kJSScriptTypeProgram:
    286         m_cachedBytecode = JSC::generateProgramBytecode(vm, sourceCode, fd, cacheError);
     332        m_cachedBytecode = JSC::generateProgramBytecode(vm, sourceCode, tempFD, cacheError);
    287333        break;
    288334    }
    … …  
    295341    }
    296342
     343    SHA1::Digest computedHash;
     344    SHA1 sha1;
     345    sha1.addBytes(m_cachedBytecode->data(), m_cachedBytecode->size());
     346    sha1.computeHash(computedHash);
     347    FileSystem::writeToFile(tempFD, reinterpret_cast<const char*>(&computedHash), sizeof(computedHash));
     348
     349    fsync(tempFD);
     350    rename(tempFileName, cacheFileName);
    297351    return YES;
    298352}

  • trunk/Source/JavaScriptCore/ChangeLog

    r257034 r257134  
     12020-02-20  Mark Lam  <mark.lam@apple.com>
     2
     3        Make support for bytecode caching more robust against file corruption.
     4        https://bugs.webkit.org/show_bug.cgi?id=207972
     5        <rdar://problem/59260595>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        If a bytecode cache file is corrupted, we currently will always crash every time
     10        we try to read it (in perpetuity as long as the corrupted cache file continues to
     11        exist on disk).  To guard against this, we'll harden the bytecode caching mechanism
     12        as follows:
     13
     14        1. Modify the writeCache operation to always write the cache file in a transactional
     15           manner i.e. we'll first write to a .tmp file, and then rename the .tmp file to
     16           the cache file only if the entire file has been written in completeness.
     17
     18           This ensures that we won't get corrupted cache files due to interrupted writes.
     19
     20        2. Modify the writeCache operation to also compute a SHA1 hash of the cache file
     21           and append the hash at end of the file.  Modify the readCache operation to
     22           first authenticate the SHA1 hash before allowing the cache file to be used.
     23           If the hash does not match, the file is bad, and we'll just delete it.
     24
     25           This ensures that we won't be crashing while decoding a corrupted cache file.
     26
     27        Manually tested with the following scenarios and ensuring that the client recovers
     28        with no crashes:
     29
     30        1. no cache file on disk.
     31        2. a 0-sized cache file on a disk.
     32        3. a truncated cache file on disk.
     33        4. a corrupted cache file on disk.
     34        5. an uncorrupted cache file on disk.
     35
     36        Also added some static_asserts in CachedTypes.cpp to document some invariants that
     37        the pre-existing code is dependent on.
     38
     39        * API/JSScript.mm:
     40        (-[JSScript readCache]):
     41        (-[JSScript writeCache:]):
     42        * runtime/CachedTypes.cpp:
     43
    1442020-02-19  Ross Kirsling  <ross.kirsling@sony.com>
    245

  • trunk/Source/JavaScriptCore/runtime/CachedTypes.cpp

    r255703 r257134  
    11/*
    2  * Copyright (C) 2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2019-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    23192319};
    23202320
     2321static_assert(alignof(GenericCacheEntry) <= alignof(std::max_align_t));
     2322
    23212323template<typename UnlinkedCodeBlockType>
    23222324class CacheEntry : public GenericCacheEntry {
    … …  
    23552357    CachedPtr<CachedCodeBlockType<UnlinkedCodeBlockType>> m_codeBlock;
    23562358};
     2359
     2360static_assert(alignof(CacheEntry<UnlinkedProgramCodeBlock>) <= alignof(std::max_align_t));
     2361static_assert(alignof(CacheEntry<UnlinkedModuleProgramCodeBlock>) <= alignof(std::max_align_t));
    23572362
    23582363bool GenericCacheEntry::decode(Decoder& decoder, std::pair<SourceCodeKey, UnlinkedCodeBlock*>& result) const
Note: See TracChangeset for help on using the changeset viewer.