Skip to content

Commit

Permalink
Fix Local Session ID allocation to be global (#13569)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mrjerryjohns @pull
mrjerryjohns authored and pull[bot] committed Apr 7, 2022
1 parent c04b5de commit 3644211
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 51 deletions.
13 changes: 12 additions & 1 deletion src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,12 @@ CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)

params.systemState->SessionMgr()->RegisterRecoveryDelegate(*this);

#if 0 //
// We cannot reinstantiate session ID allocator state from each fabric-scoped commissioner
// individually because the session ID allocator space is and must be shared for all users
// of the Session Manager. Disable persistence for now. #12821 tracks a proper fix this issue.
//

uint16_t nextKeyID = 0;
uint16_t size = sizeof(nextKeyID);
CHIP_ERROR error = mStorageDelegate->SyncGetKeyValue(kNextAvailableKeyID, &nextKeyID, size);
Expand All @@ -627,6 +633,8 @@ CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)
nextKeyID = 0;
}
ReturnErrorOnFailure(mIDAllocator.ReserveUpTo(nextKeyID));
#endif

mPairingDelegate = params.pairingDelegate;

#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
Expand Down Expand Up @@ -840,7 +848,10 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re

// Immediately persist the updated mNextKeyID value
// TODO maybe remove FreeRendezvousSession() since mNextKeyID is always persisted immediately
PersistNextKeyId();
//
// Disabling session ID persistence (see previous comment in Init() about persisting key ids)
//
// PersistNextKeyId();

exit:
if (err != CHIP_NO_ERROR)
Expand Down
30 changes: 16 additions & 14 deletions src/protocols/secure_channel/SessionIDAllocator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,36 @@

namespace chip {

uint16_t SessionIDAllocator::sNextAvailable = 1;

CHIP_ERROR SessionIDAllocator::Allocate(uint16_t & id)
{
VerifyOrReturnError(mNextAvailable < kMaxSessionID, CHIP_ERROR_NO_MEMORY);
VerifyOrReturnError(mNextAvailable > kUnsecuredSessionId, CHIP_ERROR_INTERNAL);
id = mNextAvailable;
VerifyOrReturnError(sNextAvailable < kMaxSessionID, CHIP_ERROR_NO_MEMORY);
VerifyOrReturnError(sNextAvailable > kUnsecuredSessionId, CHIP_ERROR_INTERNAL);
id = sNextAvailable;

// TODO - Update SessionID allocator to use freed session IDs
mNextAvailable++;
sNextAvailable++;

return CHIP_NO_ERROR;
}

void SessionIDAllocator::Free(uint16_t id)
{
// As per spec 4.4.1.3 Session ID of 0 is reserved for Unsecure communication
if (mNextAvailable > (kUnsecuredSessionId + 1) && (mNextAvailable - 1) == id)
if (sNextAvailable > (kUnsecuredSessionId + 1) && (sNextAvailable - 1) == id)
{
mNextAvailable--;
sNextAvailable--;
}
}

CHIP_ERROR SessionIDAllocator::Reserve(uint16_t id)
{
VerifyOrReturnError(id < kMaxSessionID, CHIP_ERROR_NO_MEMORY);
if (id >= mNextAvailable)
if (id >= sNextAvailable)
{
mNextAvailable = id;
mNextAvailable++;
sNextAvailable = id;
sNextAvailable++;
}

// TODO - Check if ID is already allocated in SessionIDAllocator::Reserve()
Expand All @@ -59,23 +61,23 @@ CHIP_ERROR SessionIDAllocator::Reserve(uint16_t id)
CHIP_ERROR SessionIDAllocator::ReserveUpTo(uint16_t id)
{
VerifyOrReturnError(id < kMaxSessionID, CHIP_ERROR_NO_MEMORY);
if (id >= mNextAvailable)
if (id >= sNextAvailable)
{
mNextAvailable = id;
mNextAvailable++;
sNextAvailable = id;
sNextAvailable++;
}

// TODO - Update ReserveUpTo to mark all IDs in use
// Current SessionIDAllocator only tracks the smallest unused session ID.
// If/when we change it to track all in use IDs, we should also update ReserveUpTo
// to reserve all individual session IDs, instead of just setting the mNextAvailable.
// to reserve all individual session IDs, instead of just setting the sNextAvailable.

return CHIP_NO_ERROR;
}

uint16_t SessionIDAllocator::Peek()
{
return mNextAvailable;
return sNextAvailable;
}

} // namespace chip
5 changes: 4 additions & 1 deletion src/protocols/secure_channel/SessionIDAllocator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
// available keys (either session or group), and the particular encryption/message
// integrity algorithm to use for the message.The Session ID field is always present.
// A Session ID of 0 SHALL indicate an unsecured session with no encryption or message integrity checking.
//
// The Session ID is allocated from a global numerical space shared across all fabrics and nodes on the resident process instance.
//

namespace chip {

Expand All @@ -46,7 +49,7 @@ class SessionIDAllocator
static constexpr uint16_t kMaxSessionID = UINT16_MAX;
static constexpr uint16_t kUnsecuredSessionId = 0;

uint16_t mNextAvailable = 1;
static uint16_t sNextAvailable;
};

} // namespace chip
54 changes: 19 additions & 35 deletions src/protocols/secure_channel/tests/TestSessionIDAllocator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,76 +24,61 @@

using namespace chip;

void TestSessionIDAllocator_Allocate(nlTestSuite * inSuite, void * inContext)
{
SessionIDAllocator allocator;

NL_TEST_ASSERT(inSuite, allocator.Peek() == 1);

uint16_t id;

for (uint16_t i = 1; i < 16; i++)
{
CHIP_ERROR err = allocator.Allocate(id);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, id == i);
NL_TEST_ASSERT(inSuite, allocator.Peek() == i + 1);
}
}

void TestSessionIDAllocator_Free(nlTestSuite * inSuite, void * inContext)
{
SessionIDAllocator allocator;

NL_TEST_ASSERT(inSuite, allocator.Peek() == 1);
uint16_t i = allocator.Peek();

uint16_t id;

for (uint16_t i = 1; i < 17; i++)
for (uint16_t j = 0; j < 17; j++)
{
CHIP_ERROR err = allocator.Allocate(id);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, id == i);
NL_TEST_ASSERT(inSuite, allocator.Peek() == i + 1);
NL_TEST_ASSERT(inSuite, id == static_cast<uint16_t>(i + j));
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + j + 1));
}

// Free an intermediate ID
allocator.Free(10);
NL_TEST_ASSERT(inSuite, allocator.Peek() == 17);
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + 17));

// Free the last allocated ID
allocator.Free(16);
NL_TEST_ASSERT(inSuite, allocator.Peek() == 16);
allocator.Free(static_cast<uint16_t>(i + 16));
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + 16));

// Free some random unallocated ID
allocator.Free(100);
NL_TEST_ASSERT(inSuite, allocator.Peek() == 16);
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + 16));
}

void TestSessionIDAllocator_Reserve(nlTestSuite * inSuite, void * inContext)
{
SessionIDAllocator allocator;

uint16_t i = allocator.Peek();
uint16_t id;

for (uint16_t i = 1; i < 16; i++)
for (uint16_t j = 0; j < 17; j++)
{
CHIP_ERROR err = allocator.Allocate(id);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, id == i);
NL_TEST_ASSERT(inSuite, allocator.Peek() == i + 1);
NL_TEST_ASSERT(inSuite, id == static_cast<uint16_t>(i + j));
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + j + 1));
}

allocator.Reserve(100);
NL_TEST_ASSERT(inSuite, allocator.Peek() == 101);
i = allocator.Peek();
allocator.Reserve(static_cast<uint16_t>(i + 100));
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + 101));
}

void TestSessionIDAllocator_ReserveUpTo(nlTestSuite * inSuite, void * inContext)
{
SessionIDAllocator allocator;
uint16_t i = allocator.Peek();

allocator.ReserveUpTo(100);
NL_TEST_ASSERT(inSuite, allocator.Peek() == 101);
i = allocator.Peek();
allocator.Reserve(static_cast<uint16_t>(i + 100));
NL_TEST_ASSERT(inSuite, allocator.Peek() == static_cast<uint16_t>(i + 101));
}

// Test Suite
Expand All @@ -104,7 +89,6 @@ void TestSessionIDAllocator_ReserveUpTo(nlTestSuite * inSuite, void * inContext)
// clang-format off
static const nlTest sTests[] =
{
NL_TEST_DEF("SessionIDAllocator_Allocate", TestSessionIDAllocator_Allocate),
NL_TEST_DEF("SessionIDAllocator_Free", TestSessionIDAllocator_Free),
NL_TEST_DEF("SessionIDAllocator_Reserve", TestSessionIDAllocator_Reserve),
NL_TEST_DEF("SessionIDAllocator_ReserveUpTo", TestSessionIDAllocator_ReserveUpTo),
Expand Down

0 comments on commit 3644211

Please sign in to comment.