google / quiche

BSD 3-Clause "New" or "Revised" License
633 stars 132 forks source link

Undefined behavior sanitizer (ubsan) => Null pointer passed as argument 2, which is declared to never be null. #52

Open kinokrt opened 1 year ago

kinokrt commented 1 year ago

It is about calling memcpy(...) with null for src argument from WriteBytes(...) which is called from AppendStreamFrame(...) - In Process of packet serialization. It seems this is completely valid for stream frame - To have zero data. It is not good to call memcpy where this causes undefined behavior according to C++ standard:

The behavior is undefined if either dest or src is an invalid or null pointer.

In my memcpy implementation see __nonnull attribute:

extern void memcpy (void _restrict dest, const void *_restrict __src, size_t n) THROW nonnull ((1, 2));

The problematic code is here calling WriteBytes(...) which leads to calling memcpy(...) with nullptr for src argument:

bool QuicFramer::AppendStreamFrame(const QuicStreamFrame& frame,
                                   bool no_stream_frame_length,
                                   QuicDataWriter* writer) {

...
...

  if (!writer->WriteBytes(frame.data_buffer, frame.data_length)) {
    QUIC_BUG(quic_bug_10850_84) << "Writing frame data failed.";
    return false;
  }
  return true;

...
...

}
ktprime commented 1 year ago

if (!writer->WriteBytes(frame.data_buffer, frame.data_length)) never be called image

kinokrt commented 1 year ago

From test it is reachable. In the end it calls memcpy with nullptr for src argument:

quiche/common/quiche_data_writer.cc:102:9: runtime error: null pointer passed as argument 2, which is declared to never be null
    #0 0x55d4a2884ac5 in quiche::QuicheDataWriter::WriteBytes(void const*, unsigned long) quiche/common/quiche_data_writer.cc:102
    #1 0x55d4a312e880 in quic::QuicFramer::AppendStreamFrame(quic::QuicStreamFrame const&, bool, quic::QuicDataWriter*) quiche/quic/core/quic_framer.cc:5585
    #2 0x55d4a309b6a4 in quic::QuicFramer::BuildDataPacket(quic::QuicPacketHeader const&, absl::InlinedVector<quic::QuicFrame, 1ul, std::allocator<quic::QuicFrame> > const&, char*, unsigned long, quic::EncryptionLevel) quiche/quic/core/quic_framer.cc:937
    #3 0x55d4a3180d6d in quic::QuicPacketCreator::SerializePacket(quic::QuicOwnedPacketBuffer, unsigned long, bool) quiche/quic/core/quic_packet_creator.cc:882
    #4 0x55d4a22b51bc in quic::test::QuicPacketCreatorPeer::SerializeAllFrames(quic::QuicPacketCreator*, absl::InlinedVector<quic::QuicFrame, 1ul, std::allocator<quic::QuicFrame> > const&, char*, unsigned long) quiche/quic/test_tools/quic_packet_creator_peer.cc:110
    #5 0x55d49f7f8f44 in ConstructPacket quiche/quic/core/quic_connection_test.cc:834
    #6 0x55d49f7f933d in ProcessFramesPacketWithAddresses quiche/quic/core/quic_connection_test.cc:847
    #7 0x55d49f7f8ba3 in ProcessFramePacketWithAddresses quiche/quic/core/quic_connection_test.cc:817
    #8 0x55d49f81a635 in TestBody quiche/quic/core/quic_connection_test.cc:1663

This is the related test from quic_connection_test.cpp: image

ktprime commented 1 year ago

From the code, data_producer_is always initializesed in function QuicSession::Initialize() so the test case is not useful.

void QuicSession::Initialize() {
  connection_->set_visitor(this);
  connection_->SetSessionNotifier(this);
  connection_->SetDataProducer(this);
  connection_->SetUnackedMapInitialCapacity();
  connection_->SetFromConfig(config_);
  if (perspective_ == Perspective::IS_CLIENT) {
    if (config_.HasClientRequestedIndependentOption(kAFFE, perspective_) &&
        version().HasIetfQuicFrames()) {
      connection_->set_can_receive_ack_frequency_frame();
      config_.SetMinAckDelayMs(kDefaultMinAckDelayTimeMs);
    }
  }
kinokrt commented 1 year ago

If the situation is never possible I would propose to reflect this into tests. Also remove dead code WriteBytes(...) from AppendStreamFrame(...) which is never called.

ktprime commented 1 year ago

in fact I use the follow code for long time.

  if (true || data_producer_ != nullptr) {
    QUICHE_DCHECK_EQ(nullptr, frame.data_buffer);
    QUICHE_DCHECK(frame.data_length > 0);
    if (false && frame.data_length == 0) {
      return true;
    }
    return data_producer_->WriteStreamData(frame.stream_id, frame.offset,
                                        frame.data_length,
                                        writer);
  }

  if (!writer->WriteBytes(frame.data_buffer, frame.data_length)) {
    QUIC_BUG(quic_bug_10850_84) << "Writing frame data failed.";
    return false;
  }
  return true;
}