Do not use Packet::ingress_length in parser for PacketTooShort

This may not be appropriate for architectures with multiple parsers
(e.g. egress parser in PSA).

This commit also ensures that ingress_length is updated properly for
recirculated packets in simple_switch.

Fixes #626

include/bm/bm_sim/packet.h

@@ -150,6 +150,11 @@ class Packet final {
   //! length of the outgoing packet may be different.
   int get_ingress_length() const { return ingress_length; }
 
+  //! Sets the ingress_length. This may be needed for some architectures,
+  //! although usually it is better to create a new Packet object. See
+  //! get_ingress_length() for more information about ingress_length.
+  void set_ingress_length(int length) { ingress_length = length; }
+
   void set_payload_size(size_t size) { payload_size = size; }
 
   size_t get_payload_size() const { return payload_size; }

src/bm_sim/deparser.cpp

@@ -42,7 +42,7 @@ Deparser::get_headers_size(const PHV &phv) const {
 
 void
 Deparser::deparse(Packet *pkt) const {
-  PHV *phv = pkt->get_phv();
+  const PHV *phv = pkt->get_phv();
   BMELOG(deparser_start, *pkt, *this);
   // TODO(antonin)
   // this is temporary while we experiment with the debugger
@@ -56,16 +56,14 @@ Deparser::deparse(Packet *pkt) const {
   // invalidating headers, and resetting header stacks is done in the Packet
   // destructor, when the PHV is released
   for (auto it = headers.begin(); it != headers.end(); ++it) {
-    Header &header = phv->get_header(*it);
+    const auto &header = phv->get_header(*it);
     if (header.is_valid()) {
       BMELOG(deparser_emit, *pkt, *it);
       BMLOG_DEBUG_PKT(*pkt, "Deparsing header '{}'", header.get_name());
       header.deparse(data + bytes_parsed);
       bytes_parsed += header.get_nbytes_packet();
-      // header.mark_invalid();
     }
   }
-  // phv->reset_header_stacks();
   BMELOG(deparser_done, *pkt, *this);
   DEBUGGER_NOTIFY_CTR(
       Debugger::PacketId::make(pkt->get_packet_id(), pkt->get_copy_id()),

src/bm_sim/parser.cpp

@@ -92,7 +92,7 @@ ParserOpSet<ParserLookAhead>::operator()(Packet *pkt, const char *data,
                                          size_t *bytes_parsed) const {
   static thread_local ByteContainer bc;
   auto phv = pkt->get_phv();
-  if (pkt->get_ingress_length() - *bytes_parsed < src.nbytes)
+  if (pkt->get_data_size() - *bytes_parsed < src.nbytes)
     throw parser_exception_core(ErrorCodeMap::Core::PacketTooShort);
   auto &f_dst = phv->get_field(dst.header, dst.offset);
   auto f_bits = f_dst.get_nbits();
@@ -235,7 +235,7 @@ namespace {
 
 void check_enough_data_for_extract(const Packet &pkt, size_t bytes_parsed,
                                    const Header &hdr) {
-  if (pkt.get_ingress_length() - bytes_parsed <
+  if (pkt.get_data_size() - bytes_parsed <
       static_cast<size_t>(hdr.get_nbytes_packet()))
     throw parser_exception_core(ErrorCodeMap::Core::PacketTooShort);
 }

targets/simple_switch/simple_switch.cpp

@@ -541,6 +541,9 @@ SimpleSwitch::egress_thread(size_t worker_id) {
         size_t packet_size = packet_copy->get_data_size();
         packet_copy->set_register(PACKET_LENGTH_REG_IDX, packet_size);
         phv_copy->get_field("standard_metadata.packet_length").set(packet_size);
+        // TODO(antonin): really it may be better to create a new packet here or
+        // to fold this functionality into the Packet class?
+        packet_copy->set_ingress_length(packet_size);
         input_buffer.push_front(std::move(packet_copy));
         continue;
       }

targets/simple_switch/tests/Makefile.am

@@ -19,7 +19,8 @@ common_source = main.cpp utils.cpp utils.h
 TESTS = test_packet_redirect \
 test_truncate \
 test_swap \
-test_queueing
+test_queueing \
+test_recirc
 
 check_PROGRAMS = $(TESTS) test_all
 
@@ -28,16 +29,19 @@ test_packet_redirect_SOURCES = $(common_source) test_packet_redirect.cpp
 test_truncate_SOURCES = $(common_source) test_truncate.cpp
 test_swap_SOURCES = $(common_source) test_swap.cpp
 test_queueing_SOURCES = $(common_source) test_queueing.cpp
+test_recirc_SOURCES = $(common_source) test_recirc.cpp
 
 test_all_SOURCES = $(common_source) \
 test_packet_redirect.cpp \
 test_truncate.cpp \
 test_swap.cpp \
-test_queueing.cpp
+test_queueing.cpp \
+test_recirc.cpp
 
 EXTRA_DIST = \
 testdata/packet_redirect.json \
 testdata/truncate.json \
 testdata/swap_1.json \
 testdata/swap_2.json \
-testdata/queueing.json
+testdata/queueing.json \
+testdata/recirc.json

targets/simple_switch/tests/test_recirc.cpp

@@ -0,0 +1,137 @@
+/* Copyright 2013-present Barefoot Networks, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include <bm/bm_apps/packet_pipe.h>
+
+#include <boost/filesystem.hpp>
+
+#include <string>
+#include <memory>
+#include <vector>
+#include <algorithm>  // for std::fill_n
+
+#include "simple_switch.h"
+
+#include "utils.h"
+
+namespace fs = boost::filesystem;
+
+using bm::MatchErrorCode;
+using bm::ActionData;
+using bm::MatchKeyParam;
+using bm::entry_handle_t;
+
+namespace {
+
+void
+packet_handler(int port_num, const char *buffer, int len, void *cookie) {
+  static_cast<SimpleSwitch *>(cookie)->receive(port_num, buffer, len);
+}
+
+}  // namespace
+
+class SimpleSwitch_RecircP4 : public ::testing::Test {
+ protected:
+  static constexpr size_t kMaxBufSize = 512;
+
+  static constexpr bm::device_id_t device_id{0};
+
+  SimpleSwitch_RecircP4()
+      : packet_inject(packet_in_addr) { }
+
+  // Per-test-case set-up.
+  // We make the switch a shared resource for all tests. This is mainly because
+  // the simple_switch target detaches threads
+  static void SetUpTestCase() {
+    // bm::Logger::set_logger_console();
+
+    test_switch = new SimpleSwitch(8);  // 8 ports
+
+    // load JSON
+    fs::path json_path = fs::path(testdata_dir) / fs::path(test_json);
+    test_switch->init_objects(json_path.string());
+
+    // packet in - packet out
+    test_switch->set_dev_mgr_packet_in(device_id, packet_in_addr, nullptr);
+    test_switch->Switch::start();  // there is a start member in SimpleSwitch
+    test_switch->set_packet_handler(packet_handler,
+                                    static_cast<void *>(test_switch));
+    test_switch->start_and_return();
+  }
+
+  // Per-test-case tear-down.
+  static void TearDownTestCase() {
+    delete test_switch;
+  }
+
+  virtual void SetUp() {
+    packet_inject.start();
+    auto cb = std::bind(&PacketInReceiver::receive, &receiver,
+                        std::placeholders::_1, std::placeholders::_2,
+                        std::placeholders::_3, std::placeholders::_4);
+    packet_inject.set_packet_receiver(cb, nullptr);
+
+    // default actions for all tables
+    test_switch->mt_set_default_action(0, "t_ingress", "_nop", ActionData());
+  }
+
+  virtual void TearDown() {
+    // kind of experimental, so reserved for testing
+    test_switch->reset_state();
+  }
+
+ protected:
+  static const std::string packet_in_addr;
+  static SimpleSwitch *test_switch;
+  bm_apps::PacketInject packet_inject;
+  PacketInReceiver receiver{};
+
+ private:
+  static const std::string testdata_dir;
+  static const std::string test_json;
+};
+
+const std::string SimpleSwitch_RecircP4::packet_in_addr =
+    "inproc://packets";
+
+SimpleSwitch *SimpleSwitch_RecircP4::test_switch = nullptr;
+
+const std::string SimpleSwitch_RecircP4::testdata_dir = TESTDATADIR;
+const std::string SimpleSwitch_RecircP4::test_json =
+    "recirc.json";
+
+// Test added for this issue:
+// https://github.com/p4lang/behavioral-model/issues/626
+// The parser used the ingress_length field of Packet to determine whether the
+// packet has enough available bytes for an extract, which may not be correct
+// (e.g. an architecture like PSA may have an egress parser...). In this
+// specific case, the ingress_length field wasn't updated properly for
+// recirculated packet, which exposed the issue (by raising a PacketTooShort
+// exception in the parser).
+TEST_F(SimpleSwitch_RecircP4, Recirc) {
+  static constexpr int port = 1;
+
+  const char pkt[] = {'\x00'};
+  packet_inject.send(port, pkt, sizeof(pkt));
+  char recv_buffer[] = {'\x00', '\x00'};
+  int recv_port = -1;
+  size_t recv_size = receiver.read(
+      recv_buffer, sizeof(recv_buffer), &recv_port);
+  EXPECT_EQ(port, recv_port);
+  EXPECT_EQ(2, recv_size);
+  EXPECT_EQ('\xab', recv_buffer[1]);
+}