Skip to content

Commit

Permalink
Do not crash when lookup yields empty group in indirect table
Browse files Browse the repository at this point in the history
Instead just log an error and treat it as an error.
  • Loading branch information
antoninbas committed Mar 14, 2018
1 parent fee0b24 commit 0da0b29
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 1 deletion.
2 changes: 1 addition & 1 deletion include/bm/bm_sim/match_tables.h
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ class MatchTableIndirect : public MatchTableAbstract {
std::unique_ptr<MatchUnitAbstract<IndirectIndex> > match_unit;
ActionProfile *action_profile{nullptr};
bool default_set{false};
ActionEntry empty_action{};
ActionEntry empty_action{}; // for lookups yielding empty groups
};

class MatchTableIndirectWS : public MatchTableIndirect {
Expand Down
6 changes: 6 additions & 0 deletions src/bm_sim/match_tables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,12 @@ MatchTableIndirect::lookup(const Packet &pkt,
if (!(*hit) && !default_set) return empty_action;

const IndirectIndex &index = (*hit) ? *res.value : default_index;
if (index.is_grp() && action_profile->group_is_empty(index.get_grp())) {
BMLOG_ERROR_PKT(
pkt, "Lookup in table '{}' yielded empty group", get_name());
return empty_action;
}

auto &entry = action_profile->lookup(pkt, index);
// TODO(antonin): unfortunately this has to be done at this stage and cannot
// be done when inserting a member because for 2 match tables sharing the same
Expand Down
21 changes: 21 additions & 0 deletions tests/test_tables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1828,6 +1828,27 @@ TEST_F(TableIndirectWS, DefaultGroup) {
EXPECT_EQ(rc, MatchErrorCode::SUCCESS);
}

// It can be quite complicated to prevent the control-plane from referencing
// empty groups (is it even desirable?). We must at least make sure that we do
// not crash. In the current implementation, I believe the table apply is a
// no-op.
TEST_F(TableIndirectWS, EmptyGroup) {
MatchErrorCode rc;
grp_hdl_t grp;
entry_handle_t lookup_handle;
bool hit;

auto pkt = get_pkt(64);

rc = action_profile.create_group(&grp);
EXPECT_EQ(MatchErrorCode::SUCCESS, rc);

rc = table->set_default_group(grp);
EXPECT_EQ(rc, MatchErrorCode::SUCCESS);

table->lookup(pkt, &hit, &lookup_handle);
}

TEST_F(TableIndirectWS, CustomGroupSelection) {
using GroupSelectionIface = ActionProfile::GroupSelectionIface;
using hash_t = ActionProfile::hash_t;
Expand Down

0 comments on commit 0da0b29

Please sign in to comment.