Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kinokrt opened this issue Sep 11, 2023 · 5 comments

Comments

@kinokrt
Copy link

kinokrt commented Sep 11, 2023

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
Copy link

ktprime commented Sep 14, 2023

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

@kinokrt
Copy link
Author

kinokrt commented Sep 14, 2023

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
Copy link

ktprime commented Sep 14, 2023

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
Copy link
Author

kinokrt commented Sep 14, 2023

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
Copy link

ktprime commented Sep 14, 2023

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;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants