You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by ma...@apache.org on 2017/10/17 05:29:12 UTC
[trafficserver] branch quic-latest updated: Fix a bug that packet
number can be decoded with an uninitialized value
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 <ma...@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" <co...@trafficserver.apache.org>'].