Changeset 273196 in webkit


Ignore:
Timestamp:
Feb 19, 2021 10:56:06 PM (3 years ago)
Author:
Chris Dumez
Message:

Crash under Decoder::Decoder()
https://bugs.webkit.org/show_bug.cgi?id=222192
<rdar://31392681>

Reviewed by Geoffrey Garen.

We are sometimes crashing under Decoder's copyBuffer(), inside the memcpy()
call, with a null address. I have no idea how this is happening and this
code has not changed in a long time so I have made the following hardening:

  1. Update copyBuffer() to use tryFastMalloc() instead of fastMalloc(). Log and return null if tryFastMalloc() failed instead of calling memcpy().
  2. Update Decoder::create() to log and return early if the input buffer is null.
  3. Update Connection's createMessageDecoder() to use CheckedSize when computing the bodySize that is being passed to Decoder::create(). If we overflow, log and return null.

No new tests, no idea how this can happen in practice.

  • Platform/IPC/Decoder.cpp:

(IPC::copyBuffer):
(IPC::Decoder::create):
(IPC::Decoder::Decoder):

  • Platform/IPC/cocoa/ConnectionCocoa.mm:

(IPC::createMessageDecoder):

Location:
trunk/Source/WebKit
Files:
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r273195 r273196  
     12021-02-19  Chris Dumez  <cdumez@apple.com>
     2
     3        Crash under Decoder::Decoder()
     4        https://bugs.webkit.org/show_bug.cgi?id=222192
     5        <rdar://31392681>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        We are sometimes crashing under Decoder's copyBuffer(), inside the memcpy()
     10        call, with a null address. I have no idea how this is happening and this
     11        code has not changed in a long time so I have made the following hardening:
     12        1. Update copyBuffer() to use tryFastMalloc() instead of fastMalloc(). Log
     13           and return null if tryFastMalloc() failed instead of calling memcpy().
     14        2. Update Decoder::create() to log and return early if the input buffer
     15           is null.
     16        3. Update Connection's createMessageDecoder() to use CheckedSize when
     17           computing the bodySize that is being passed to Decoder::create(). If
     18           we overflow, log and return null.
     19
     20        No new tests, no idea how this can happen in practice.
     21
     22        * Platform/IPC/Decoder.cpp:
     23        (IPC::copyBuffer):
     24        (IPC::Decoder::create):
     25        (IPC::Decoder::Decoder):
     26        * Platform/IPC/cocoa/ConnectionCocoa.mm:
     27        (IPC::createMessageDecoder):
     28
    1292021-02-19  BJ Burg  <bburg@apple.com>
    230
  • trunk/Source/WebKit/Platform/IPC/Decoder.cpp

    r271916 r273196  
    2929#include "ArgumentCoders.h"
    3030#include "DataReference.h"
     31#include "Logging.h"
    3132#include "MessageFlags.h"
    3233#include <stdio.h>
     
    4142static const uint8_t* copyBuffer(const uint8_t* buffer, size_t bufferSize)
    4243{
    43     auto bufferCopy = static_cast<uint8_t*>(fastMalloc(bufferSize));
     44    uint8_t* bufferCopy;
     45    if (!tryFastMalloc(bufferSize).getValue(bufferCopy)) {
     46        RELEASE_LOG_FAULT(IPC, "Decoder::copyBuffer: tryFastMalloc(%lu) failed", bufferSize);
     47        return nullptr;
     48    }
     49
    4450    memcpy(bufferCopy, buffer, bufferSize);
    45 
    4651    return bufferCopy;
    4752}
     
    4954std::unique_ptr<Decoder> Decoder::create(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment>&& attachments)
    5055{
    51     auto decoder = makeUnique<Decoder>(buffer, bufferSize, bufferDeallocator, WTFMove(attachments));
     56    ASSERT(buffer);
     57    if (UNLIKELY(!buffer)) {
     58        RELEASE_LOG_FAULT(IPC, "Decoder::create() called with a null buffer (bufferSize: %lu)", bufferSize);
     59        return nullptr;
     60    }
     61
     62    const uint8_t* bufferCopy;
     63    if (!bufferDeallocator) {
     64        bufferCopy = copyBuffer(buffer, bufferSize);
     65        ASSERT(bufferCopy);
     66        if (UNLIKELY(!bufferCopy))
     67            return nullptr;
     68    } else
     69        bufferCopy = buffer;
     70
     71    auto decoder = std::unique_ptr<Decoder>(new Decoder(bufferCopy, bufferSize, bufferDeallocator, WTFMove(attachments)));
    5272    return decoder->isValid() ? WTFMove(decoder) : nullptr;
    5373}
    5474
    5575Decoder::Decoder(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment>&& attachments)
    56     : m_buffer { bufferDeallocator ? buffer : copyBuffer(buffer, bufferSize) }
     76    : m_buffer { buffer }
    5777    , m_bufferPos { m_buffer }
    5878    , m_bufferEnd { m_buffer + bufferSize }
  • trunk/Source/WebKit/Platform/IPC/Decoder.h

    r272058 r273196  
    5151public:
    5252    static std::unique_ptr<Decoder> create(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment>&&);
    53     explicit Decoder(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment>&&);
    5453    ~Decoder();
    5554
     
    142141
    143142private:
     143    Decoder(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment>&&);
     144
    144145    enum ConstructWithoutHeaderTag { ConstructWithoutHeader };
    145146    Decoder(const uint8_t* buffer, size_t bufferSize, ConstructWithoutHeaderTag);
  • trunk/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm

    r272058 r273196  
    2929#import "DataReference.h"
    3030#import "ImportanceAssertion.h"
     31#import "Logging.h"
    3132#import "MachMessage.h"
    3233#import "MachPort.h"
     
    420421        // We have a simple message.
    421422        uint8_t* body = reinterpret_cast<uint8_t*>(header + 1);
    422         size_t bodySize = header->msgh_size - sizeof(mach_msg_header_t);
    423 
    424         return Decoder::create(body, bodySize, nullptr, Vector<Attachment> { });
     423        auto bodySize = CheckedSize { header->msgh_size } - sizeof(mach_msg_header_t);
     424        if (UNLIKELY(bodySize.hasOverflowed())) {
     425            RELEASE_LOG_FAULT(IPC, "createMessageDecoder: Overflow when computing bodySize (header->msgh_size: %lu, sizeof(mach_msg_header_t): %lu)", static_cast<unsigned long>(header->msgh_size), sizeof(mach_msg_header_t));
     426            ASSERT_NOT_REACHED();
     427            return nullptr;
     428        }
     429
     430        return Decoder::create(body, bodySize.unsafeGet(), nullptr, Vector<Attachment> { });
    425431    }
    426432
     
    467473
    468474    uint8_t* messageBody = descriptorData;
    469     size_t messageBodySize = header->msgh_size - (descriptorData - reinterpret_cast<uint8_t*>(header));
    470 
    471     return Decoder::create(messageBody, messageBodySize, nullptr, WTFMove(attachments));
     475    ASSERT(descriptorData >= reinterpret_cast<uint8_t*>(header));
     476    auto messageBodySize = CheckedSize { header->msgh_size } - static_cast<size_t>(descriptorData - reinterpret_cast<uint8_t*>(header));
     477    if (UNLIKELY(messageBodySize.hasOverflowed())) {
     478        RELEASE_LOG_FAULT(IPC, "createMessageDecoder: Overflow when computing bodySize (header->msgh_size: %lu, (descriptorData - reinterpret_cast<uint8_t*>(header)): %lu)", static_cast<unsigned long>(header->msgh_size), static_cast<unsigned long>(descriptorData - reinterpret_cast<uint8_t*>(header)));
     479        ASSERT_NOT_REACHED();
     480        return nullptr;
     481    }
     482
     483    return Decoder::create(messageBody, messageBodySize.unsafeGet(), nullptr, WTFMove(attachments));
    472484}
    473485
  • trunk/Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp

    r271626 r273196  
    238238        messageBody = reinterpret_cast<uint8_t*>(oolMessageBody->data());
    239239
    240     auto decoder = makeUnique<Decoder>(messageBody, messageInfo.bodySize(), nullptr, WTFMove(attachments));
     240    auto decoder = Decoder::create(messageBody, messageInfo.bodySize(), nullptr, WTFMove(attachments));
     241    ASSERT(decoder);
     242    if (!decoder)
     243        return false;
    241244
    242245    processIncomingMessage(WTFMove(decoder));
  • trunk/Source/WebKit/Platform/IPC/win/ConnectionWin.cpp

    r248846 r273196  
    140140        if (!m_readBuffer.isEmpty()) {
    141141            // We have a message, let's dispatch it.
    142             Vector<Attachment> attachments(0);
    143             auto decoder = makeUnique<Decoder>(m_readBuffer.data(), m_readBuffer.size(), nullptr, WTFMove(attachments));
     142            auto decoder = Decoder::create(m_readBuffer.data(), m_readBuffer.size(), nullptr, { });
     143            ASSERT(decoder);
     144            if (!decoder)
     145                return;
    144146            processIncomingMessage(WTFMove(decoder));
    145147        }
  • trunk/Source/WebKit/UIProcess/LegacySessionStateCodingNone.cpp

    r261254 r273196  
    4949bool decodeLegacySessionState(const uint8_t* data, size_t dataSize, SessionState& sessionState)
    5050{
    51     IPC::Decoder decoder(data, dataSize, nullptr, Vector<IPC::Attachment>());
     51    auto decoder = IPC::Decoder::create(data, dataSize, nullptr, Vector<IPC::Attachment>());
     52    if (!decoder)
     53        return false;
    5254
    5355    Optional<BackForwardListState> backForwardListState;
    54     decoder >> backForwardListState;
     56    *decoder >> backForwardListState;
    5557    if (!backForwardListState)
    5658        return false;
     
    5860
    5961    Optional<uint64_t> renderTreeSize;
    60     decoder >> renderTreeSize;
     62    *decoder >> renderTreeSize;
    6163    if (!renderTreeSize)
    6264        return false;
     
    6466
    6567    Optional<URL> provisionalURL;
    66     decoder >> provisionalURL;
     68    *decoder >> provisionalURL;
    6769    if (!provisionalURL)
    6870        return false;
  • trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

    r273183 r273196  
    397397        auto buffer = message.encoder->buffer();
    398398        auto bufferSize = message.encoder->bufferSize();
    399         std::unique_ptr<IPC::Decoder> decoder = makeUnique<IPC::Decoder>(buffer, bufferSize, nullptr, Vector<IPC::Attachment> { });
     399        auto decoder = IPC::Decoder::create(buffer, bufferSize, nullptr, { });
     400        ASSERT(decoder);
     401        if (!decoder)
     402            return false;
     403
    400404        LoadParameters loadParameters;
    401405        URL resourceDirectoryURL;
Note: See TracChangeset for help on using the changeset viewer.