This is an automated email from the ASF dual-hosted git repository. maskit pushed a commit to branch quic-latest in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/quic-latest by this push: new 0a9919e Fix a bug that packet number can be decoded with an uninitialized value 0a9919e is described below commit 0a9919e60538d162d9d28767f6ab43b4d2059c49 Author: Masakazu Kitajo <mas...@apache.org> AuthorDate: Tue Oct 17 14:26:02 2017 +0900 Fix a bug that packet number can be decoded with an uninitialized value --- iocore/net/P_QUICNetVConnection.h | 7 +++-- iocore/net/P_UDPNet.h | 1 - iocore/net/QUICNetProcessor.cc | 4 ++- iocore/net/QUICNetVConnection.cc | 43 ++++++++++++++++++++------ iocore/net/QUICPacketHandler.cc | 3 +- iocore/net/quic/Mock.h | 1 - iocore/net/quic/QUICFrame.h | 1 - iocore/net/quic/QUICIncomingFrameBuffer.h | 1 - iocore/net/quic/QUICPacket.cc | 24 ++++++++------ iocore/net/quic/QUICPacket.h | 8 ++--- iocore/net/quic/test/test_QUICPacket.cc | 14 ++------- iocore/net/quic/test/test_QUICPacketFactory.cc | 10 ++---- 12 files changed, 64 insertions(+), 53 deletions(-) diff --git a/iocore/net/P_QUICNetVConnection.h b/iocore/net/P_QUICNetVConnection.h index 339f92e..c249d20 100644 --- a/iocore/net/P_QUICNetVConnection.h +++ b/iocore/net/P_QUICNetVConnection.h @@ -144,7 +144,6 @@ class QUICNetVConnection : public UnixNetVConnection, public QUICConnection public: QUICNetVConnection() {} - void init(UDPConnection *, QUICPacketHandler *); // UnixNetVConnection @@ -159,7 +158,7 @@ public: int state_connection_closing(int event, Event *data); int state_connection_closed(int event, Event *data); void start(SSL_CTX *); - void push_packet(QUICPacketUPtr packet); + void push_packet(UDPPacket *packet); void free(EThread *t) override; UDPConnection *get_udp_con(); @@ -224,8 +223,9 @@ private: QUICRemoteFlowController *_remote_flow_controller = nullptr; QUICLocalFlowController *_local_flow_controller = nullptr; - Queue<QUICPacket> _packet_recv_queue; + Queue<UDPPacket> _packet_recv_queue; Queue<QUICPacket> _packet_send_queue; + std::queue<QUICPacketUPtr> _quic_packet_recv_queue; // `_frame_send_queue` is the queue for any type of frame except STREAM frame. // The flow contorl doesn't blcok frames in this queue. // `_stream_frame_send_queue` is the queue for STREAM frame. @@ -259,6 +259,7 @@ private: void _init_flow_control_params(const std::shared_ptr<const QUICTransportParameters> &local_tp, const std::shared_ptr<const QUICTransportParameters> &remote_tp); void _handle_error(QUICErrorUPtr error); + QUICPacketUPtr _dequeue_recv_packet(); QUICStatelessToken _token; }; diff --git a/iocore/net/P_UDPNet.h b/iocore/net/P_UDPNet.h index dcc28e4..5afb9c9 100644 --- a/iocore/net/P_UDPNet.h +++ b/iocore/net/P_UDPNet.h @@ -62,7 +62,6 @@ class PacketQueue { public: PacketQueue() { init(); } - virtual ~PacketQueue() {} int nPackets = 0; ink_hrtime lastPullLongTermQ = 0; diff --git a/iocore/net/QUICNetProcessor.cc b/iocore/net/QUICNetProcessor.cc index 291dfab..608f3d7 100644 --- a/iocore/net/QUICNetProcessor.cc +++ b/iocore/net/QUICNetProcessor.cc @@ -97,9 +97,11 @@ QUICNetProcessor::allocate_vc(EThread *t) QUICNetVConnection *vc; if (t) { - vc = THREAD_ALLOC_INIT(quicNetVCAllocator, t); + vc = THREAD_ALLOC(quicNetVCAllocator, t); + new (vc) QUICNetVConnection(); } else { if (likely(vc = quicNetVCAllocator.alloc())) { + new (vc) QUICNetVConnection(); vc->from_accept_thread = true; } } diff --git a/iocore/net/QUICNetVConnection.cc b/iocore/net/QUICNetVConnection.cc index 4229c56..ab3cad0 100644 --- a/iocore/net/QUICNetVConnection.cc +++ b/iocore/net/QUICNetVConnection.cc @@ -259,11 +259,9 @@ QUICNetVConnection::get_packet_transmitter_mutex() } void -QUICNetVConnection::push_packet(QUICPacketUPtr packet) +QUICNetVConnection::push_packet(UDPPacket *packet) { - DebugQUICCon("type=%s pkt_num=%" PRIu64 " size=%u", QUICDebugNames::packet_type(packet->type()), packet->packet_number(), - packet->size()); - this->_packet_recv_queue.enqueue(const_cast<QUICPacket *>(packet.release())); + this->_packet_recv_queue.enqueue(packet); } void @@ -381,7 +379,7 @@ QUICNetVConnection::state_handshake(int event, Event *data) switch (event) { case QUIC_EVENT_PACKET_READ_READY: { - QUICPacketUPtr p = QUICPacketUPtr(this->_packet_recv_queue.dequeue(), &QUICPacketDeleter::delete_packet); + QUICPacketUPtr p = this->_dequeue_recv_packet(); net_activity(this, this_ethread()); switch (p->type()) { @@ -400,7 +398,7 @@ QUICNetVConnection::state_handshake(int event, Event *data) case QUICPacketType::ONE_RTT_PROTECTED_KEY_PHASE_0: case QUICPacketType::ONE_RTT_PROTECTED_KEY_PHASE_1: // Postpone processing the packet - this->push_packet(std::move(p)); + this->_quic_packet_recv_queue.push(std::move(p)); this_ethread()->schedule_imm_local(this, event); break; default: @@ -703,11 +701,9 @@ QUICNetVConnection::_state_connection_established_process_packet(QUICPacketUPtr packet->packet_number(), packet->header(), packet->header_size(), packet->key_phase())) { DebugQUICCon("Decrypt Packet, pkt_num: %" PRIu64 ", header_len: %hu, payload_len: %zu", packet->packet_number(), packet->header_size(), plain_txt_len); - return this->_recv_and_ack(plain_txt.get(), plain_txt_len, packet->packet_number()); } else { DebugQUICCon("CRYPTOGRAPHIC Error"); - return QUICConnectionErrorUPtr(new QUICConnectionError(QUICErrorClass::CRYPTOGRAPHIC, QUICErrorCode::CRYPTOGRAPHIC_ERROR)); } } @@ -716,7 +712,7 @@ QUICErrorUPtr QUICNetVConnection::_state_common_receive_packet() { QUICErrorUPtr error = QUICErrorUPtr(new QUICNoError()); - QUICPacketUPtr p = QUICPacketUPtr(this->_packet_recv_queue.dequeue(), &QUICPacketDeleter::delete_packet); + QUICPacketUPtr p = this->_dequeue_recv_packet(); net_activity(this, this_ethread()); switch (p->type()) { @@ -949,3 +945,32 @@ QUICNetVConnection::_handle_error(QUICErrorUPtr error) this->close(QUICConnectionErrorUPtr(cerror)); } } + +QUICPacketUPtr +QUICNetVConnection::_dequeue_recv_packet() +{ + QUICPacketUPtr quic_packet = QUICPacketUPtr(nullptr, &QUICPacketDeleter::delete_null_packet); + UDPPacket *udp_packet = this->_packet_recv_queue.dequeue(); + if (udp_packet) { + ats_unique_buf pkt = ats_unique_malloc(udp_packet->getPktLength()); + IOBufferBlock *b = udp_packet->getIOBlockChain(); + size_t written = 0; + while (b) { + memcpy(pkt.get() + written, b->buf(), b->read_avail()); + written += b->read_avail(); + b = b->next.get(); + } + udp_packet->free(); + quic_packet = QUICPacketFactory::create(std::move(pkt), written, this->largest_received_packet_number()); + } + + if (this->_quic_packet_recv_queue.size() > 0) { + QUICPacketUPtr p = std::move(this->_quic_packet_recv_queue.front()); + this->_quic_packet_recv_queue.pop(); + this->_quic_packet_recv_queue.push(std::move(quic_packet)); + quic_packet.reset(p.release()); + } + DebugQUICCon("type=%s pkt_num=%" PRIu64 " size=%u", QUICDebugNames::packet_type(quic_packet->type()), + quic_packet->packet_number(), quic_packet->size()); + return quic_packet; +} diff --git a/iocore/net/QUICPacketHandler.cc b/iocore/net/QUICPacketHandler.cc index e2e0c22..e86fed4 100644 --- a/iocore/net/QUICPacketHandler.cc +++ b/iocore/net/QUICPacketHandler.cc @@ -166,8 +166,7 @@ QUICPacketHandler::_recv_packet(int event, UDPPacket *udpPacket) this->action_->continuation->handleEvent(NET_EVENT_ACCEPT, vc); } - QUICPacketUPtr qPkt = QUICPacketFactory::create(block, vc->largest_received_packet_number()); - vc->push_packet(std::move(qPkt)); + vc->push_packet(udpPacket); // send to EThread eventProcessor.schedule_imm(vc, ET_CALL, QUIC_EVENT_PACKET_READ_READY, nullptr); diff --git a/iocore/net/quic/Mock.h b/iocore/net/quic/Mock.h index 41d5390..65e9713 100644 --- a/iocore/net/quic/Mock.h +++ b/iocore/net/quic/Mock.h @@ -373,7 +373,6 @@ class MockQUICApplication : public QUICApplication { public: MockQUICApplication() : QUICApplication(new MockQUICConnection) { SET_HANDLER(&MockQUICApplication::main_event_handler); } - int main_event_handler(int event, Event *data) { diff --git a/iocore/net/quic/QUICFrame.h b/iocore/net/quic/QUICFrame.h index 9f2b356..dad9a6c 100644 --- a/iocore/net/quic/QUICFrame.h +++ b/iocore/net/quic/QUICFrame.h @@ -41,7 +41,6 @@ public: virtual void reset(const uint8_t *buf, size_t len); static QUICFrameType type(const uint8_t *buf); virtual ~QUICFrame() {} - LINK(QUICFrame, link); protected: diff --git a/iocore/net/quic/QUICIncomingFrameBuffer.h b/iocore/net/quic/QUICIncomingFrameBuffer.h index ab79a44..66b637a 100644 --- a/iocore/net/quic/QUICIncomingFrameBuffer.h +++ b/iocore/net/quic/QUICIncomingFrameBuffer.h @@ -33,7 +33,6 @@ class QUICIncomingFrameBuffer { public: QUICIncomingFrameBuffer(QUICStream *stream) : _stream(stream) {} - ~QUICIncomingFrameBuffer(); std::shared_ptr<const QUICStreamFrame> pop(); diff --git a/iocore/net/quic/QUICPacket.cc b/iocore/net/quic/QUICPacket.cc index 5af7e63..276cc7d 100644 --- a/iocore/net/quic/QUICPacket.cc +++ b/iocore/net/quic/QUICPacket.cc @@ -445,11 +445,10 @@ QUICPacketShortHeader::store(uint8_t *buf, size_t *len) const // // QUICPacket // -QUICPacket::QUICPacket(IOBufferBlock *block, QUICPacketNumber base_packet_number) : _block(block) +QUICPacket::QUICPacket(ats_unique_buf buf, size_t len, QUICPacketNumber base_packet_number) { - this->_size = block->size(); - this->_header = - QUICPacketHeader::load(reinterpret_cast<const uint8_t *>(this->_block->buf()), this->_block->size(), base_packet_number); + this->_size = len; + this->_header = QUICPacketHeader::load(reinterpret_cast<const uint8_t *>(buf.release()), len, base_packet_number); } QUICPacket::QUICPacket(QUICPacketType type, QUICConnectionId connection_id, QUICPacketNumber packet_number, @@ -628,8 +627,8 @@ bool QUICPacket::has_valid_fnv1a_hash() const { uint8_t hash[FNV1A_HASH_LEN]; - fnv1a(reinterpret_cast<const uint8_t *>(this->_block->buf()), this->_block->size() - FNV1A_HASH_LEN, hash); - return memcmp(this->_block->buf() + this->_block->size() - FNV1A_HASH_LEN, hash, 8) == 0; + fnv1a(reinterpret_cast<const uint8_t *>(this->_header->buf()), this->size() - FNV1A_HASH_LEN, hash); + return memcmp(this->_header->buf() + this->size() - FNV1A_HASH_LEN, hash, 8) == 0; } void @@ -669,8 +668,7 @@ QUICPacket::encode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, si } bool -QUICPacket::decode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, size_t len, - QUICPacketNumber largest_acked) +QUICPacket::decode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, size_t len, QUICPacketNumber largest_acked) { ink_assert(len == 1 || len == 2 || len == 4); @@ -686,6 +684,12 @@ QUICPacket::decode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, si dst = candidate2; } + Debug("quic_packet_factory", "----------------------- src: %" PRIu64, src); + Debug("quic_packet_factory", "----------------------- base: %" PRIu64, base); + Debug("quic_packet_factory", "----------------------- c1: %" PRIu64, candidate1); + Debug("quic_packet_factory", "----------------------- c2: %" PRIu64, candidate2); + Debug("quic_packet_factory", "----------------------- dst: %" PRIu64, dst); + return true; } @@ -693,10 +697,10 @@ QUICPacket::decode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, si // QUICPacketFactory // QUICPacketUPtr -QUICPacketFactory::create(IOBufferBlock *block, QUICPacketNumber base_packet_number) +QUICPacketFactory::create(ats_unique_buf buf, size_t len, QUICPacketNumber base_packet_number) { QUICPacket *packet = quicPacketAllocator.alloc(); - new (packet) QUICPacket(block, base_packet_number); + new (packet) QUICPacket(std::move(buf), len, base_packet_number); return QUICPacketUPtr(packet, &QUICPacketDeleter::delete_packet); } diff --git a/iocore/net/quic/QUICPacket.h b/iocore/net/quic/QUICPacket.h index 9ed9632..987b90e 100644 --- a/iocore/net/quic/QUICPacket.h +++ b/iocore/net/quic/QUICPacket.h @@ -129,7 +129,7 @@ class QUICPacket { public: QUICPacket(){}; - QUICPacket(IOBufferBlock *block, QUICPacketNumber base_packet_number); + QUICPacket(ats_unique_buf buf, size_t len, QUICPacketNumber base_packet_number); QUICPacket(QUICPacketType type, QUICConnectionId connection_id, QUICPacketNumber packet_number, QUICPacketNumber base_packet_number, QUICVersion version, ats_unique_buf payload, size_t len, bool retransmittable); QUICPacket(QUICPacketType type, QUICPacketNumber packet_number, QUICPacketNumber base_packet_number, ats_unique_buf payload, @@ -156,13 +156,11 @@ public: QUICKeyPhase key_phase() const; static uint8_t calc_packet_number_len(QUICPacketNumber num, QUICPacketNumber base); static bool encode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, size_t len); - static bool decode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, size_t len, - QUICPacketNumber largest_acked); + static bool decode_packet_number(QUICPacketNumber &dst, QUICPacketNumber src, size_t len, QUICPacketNumber largest_acked); LINK(QUICPacket, link); private: - IOBufferBlock *_block = nullptr; ats_unique_buf _protected_payload = ats_unique_buf(nullptr, [](void *p) { ats_free(p); }); size_t _size = 0; size_t _protected_payload_size = 0; @@ -207,7 +205,7 @@ public: class QUICPacketFactory { public: - static QUICPacketUPtr create(IOBufferBlock *block, QUICPacketNumber base_packet_number); + static QUICPacketUPtr create(ats_unique_buf buf, size_t len, QUICPacketNumber base_packet_number); QUICPacketUPtr create_version_negotiation_packet(const QUICPacket *packet_sent_by_client, QUICPacketNumber base_packet_number); QUICPacketUPtr create_server_cleartext_packet(QUICConnectionId connection_id, QUICPacketNumber base_packet_number, ats_unique_buf payload, size_t len, bool retransmittable); diff --git a/iocore/net/quic/test/test_QUICPacket.cc b/iocore/net/quic/test/test_QUICPacket.cc index 679cd3b..8c2f2bf 100644 --- a/iocore/net/quic/test/test_QUICPacket.cc +++ b/iocore/net/quic/test/test_QUICPacket.cc @@ -39,12 +39,7 @@ TEST_CASE("Loading Long Header Packet", "[quic]") size_t len; packet1.store(buf, &len); - IOBufferBlock *block = new_IOBufferBlock(); - block->alloc(iobuffer_size_to_index(len)); - memcpy(block->end(), buf, len); - block->fill(len); - - const QUICPacket packet2(block, 0); + const QUICPacket packet2(ats_unique_buf(buf, [](void *p) { ats_free(p); }), len, 0); CHECK(packet2.type() == QUICPacketType::CLIENT_CLEARTEXT); CHECK(packet2.connection_id() == 0xffddbb9977553311ULL); @@ -73,12 +68,7 @@ TEST_CASE("Loading Short Header Packet", "[quic]") size_t len; packet1.store(buf, &len); - IOBufferBlock *block = new_IOBufferBlock(); - block->alloc(iobuffer_size_to_index(len)); - memcpy(block->end(), buf, len); - block->fill(len); - - const QUICPacket packet2(block, 0); + const QUICPacket packet2(ats_unique_buf(buf, [](void *p) { ats_free(p); }), len, 0); CHECK(packet2.type() == QUICPacketType::ONE_RTT_PROTECTED_KEY_PHASE_0); CHECK(packet2.packet_number() == 0xffcc9966); diff --git a/iocore/net/quic/test/test_QUICPacketFactory.cc b/iocore/net/quic/test/test_QUICPacketFactory.cc index fbf6a87..942b49c 100644 --- a/iocore/net/quic/test/test_QUICPacketFactory.cc +++ b/iocore/net/quic/test/test_QUICPacketFactory.cc @@ -29,7 +29,7 @@ TEST_CASE("QUICPacketFactory_Create_VersionNegotiationPacket", "[quic]") { QUICPacketFactory factory; - const uint8_t client_initial_packet_data[] = { + uint8_t client_initial_packet_data[] = { 0x82, // Type 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, // Connection id 0x00, 0x00, 0x00, 0x00, // Packet number @@ -37,12 +37,8 @@ TEST_CASE("QUICPacketFactory_Create_VersionNegotiationPacket", "[quic]") 0x00 // Payload }; - IOBufferBlock *block = new_IOBufferBlock(); - block->alloc(iobuffer_size_to_index(sizeof(client_initial_packet_data))); - memcpy(block->end(), client_initial_packet_data, sizeof(client_initial_packet_data)); - block->fill(sizeof(client_initial_packet_data)); - - QUICPacket client_initial_packet(block, 0); + QUICPacket client_initial_packet(ats_unique_buf(client_initial_packet_data, [](void *p) { ats_free(p); }), + sizeof(client_initial_packet_data), 0); QUICPacketUPtr packet = factory.create_version_negotiation_packet(&client_initial_packet, 0); CHECK(packet->type() == QUICPacketType::VERSION_NEGOTIATION); -- To stop receiving notification emails like this one, please contact ['"commits@trafficserver.apache.org" <commits@trafficserver.apache.org>'].