Follow-up minor changes to OpCertStore (#19828)

* Follow-up minor changes to OpCertStore

- After PR #19643 was merge, @bzbarsky-apple had several minor
  editorial comments.
- This PR applies the comments from
  #19643 (review)

Testing:
- Unit tests still pass
- No integration yet, so unit tests passing is enough

* Restyled by clang-format

* Fix grammar

Co-authored-by: Restyled.io <commits@restyled.io>

src/app/ClusterStateCache.cpp

@@ -220,7 +220,7 @@ CHIP_ERROR ClusterStateCache::Get(const ConcreteAttributePath & path, TLV::TLVRe
  }
 
  reader.Init(attributeState->Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().Get(),
- attributeState->Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().BufferByteSize());
+ attributeState->Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().AllocatedSize());
  return reader.Next();
 }
 
@@ -423,7 +423,7 @@ void ClusterStateCache::GetSortedFilters(std::vector<std::pair<DataVersionFilter
  {
  TLV::TLVReader bufReader;
  bufReader.Init(attributeIter.second.Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().Get(),
- attributeIter.second.Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().BufferByteSize());
+ attributeIter.second.Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().AllocatedSize());
  ReturnOnFailure(bufReader.Next());
  // Skip to the end of the element.
  ReturnOnFailure(bufReader.Skip());

src/credentials/OperationalCertificateStore.h

@@ -64,19 +64,19 @@ class OperationalCertificateStore
  virtual bool HasCertificateForFabric(FabricIndex fabricIndex, CertChainElement element) const = 0;
 
  /**
- * @brief Add and temporarily activate a new Trusted Root Certificate to storage for the given fabric
+ * @brief Add and temporarily activate a new Trusted Root Certificate for the given fabric
  *
  * The certificate is temporary until committed or reverted.
- * The certificate is committed to storage on `CommitOpCertsForFabric`.
+ * The certificate is committed to storage only on `CommitOpCertsForFabric`.
  * The certificate is destroyed if `RevertPendingOpCerts` is called before `CommitOpCertsForFabric`.
  *
  * Only one pending trusted root certificate is supported at a time and it is illegal
  * to call this method if there is already a persisted root certificate for the given
  * fabric.
  *
- * Uniqueness constraints for roots (see AddTrustedRootCertificate command in spec) is not
+ * Uniqueness constraints for roots (see AddTrustedRootCertificate command in spec) are not
  * enforced by this method and must be done as a more holistic check elsewhere. Cryptographic
- * signature verification or path validation is not enforced by this method.
+ * signature verification or path validation are not enforced by this method.
  *
  * If `UpdateOpCertsForFabric` had been called before this method, this method will return
  * CHIP_ERROR_INCORRECT_STATE since it is illegal to update trusted roots when updating an
@@ -106,11 +106,11 @@ class OperationalCertificateStore
  * to call this method if there is already a persisted certificate chain for the given
  * fabric.
  *
- * Cryptographic signature verification or path validation is not enforced by this method.
+ * Cryptographic signature verification or path validation are not enforced by this method.
  *
  * If `UpdateOpCertsForFabric` had been called before this method, this method will return
  * CHIP_ERROR_INCORRECT_STATE since it is illegal to add a certificate chain after
- * updating an existing NOC updating prior to a commit.
+ * updating an existing NOC and before committing or reverting the update.
  *
  * If `AddNewTrustedRootCertForFabric` had not been called before this method, this method will
  * return CHIP_ERROR_INCORRECT_STATE since it is illegal in this implementation to store an
@@ -129,7 +129,7 @@ class OperationalCertificateStore
  * @retval CHIP_NO_ERROR on success
  * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to maintain the temporary `noc` and `icac` cert copies
  * @retval CHIP_ERROR_INVALID_ARGUMENT if either the noc or icac are invalid sizes
- * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if `fabricIndex` mismatches the one from previous successful
+ * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if `fabricIndex` mismatches the one from a previous successful
  * `AddNewTrustedRootCertForFabric`.
  * @retval CHIP_ERROR_INCORRECT_STATE if the certificate store is not properly initialized, if this method
  * is called after `UpdateOpCertsForFabric`, or if there was
@@ -151,33 +151,34 @@ class OperationalCertificateStore
  * to call this method if there was not already a persisted certificate chain for the given
  * fabric.
  *
- * Cryptographic signature verification or path validation is not enforced by this method.
+ * Cryptographic signature verification or path validation are not enforced by this method.
  *
  * If `AddNewOpCertsForFabric` had been called before this method, this method will return
  * CHIP_ERROR_INCORRECT_STATE since it is illegal to update a certificate chain after
- * adding an existing NOC updating prior to a commit.
+ * adding an existing NOC and before committing or reverting the addition.
  *
- * If there is no existing persisted trusted root certificate for the given fabricIndex, this
- * method will method will eturn CHIP_ERROR_INCORRECT_STATE since it is illegal in this
- * implementation to store an NOC chain without associated root.
+ * If there is no existing persisted trusted root certificate and NOC chain for the given
+ * fabricIndex, this method will return CHIP_ERROR_INCORRECT_STATE since it is
+ * illegal in this implementation to store an NOC chain without associated root, and it is illegal
+ * to update an opcert for a fabric not already configured.
  *
- * @param fabricIndex - FabricIndex for which to add a new operational certificate chain
- * @param noc - Buffer containing the NOC certificate to update
- * @param icac - Buffer containing the ICAC certificate to update. If no ICAC is needed, `icac.empty()` must be true.
+ * @param fabricIndex - FabricIndex for which to update the operational certificate chain
+ * @param noc - Buffer containing the new NOC certificate to use
+ * @param icac - Buffer containing the ICAC certificate to use. If no ICAC is needed, `icac.empty()` must be true.
  *
  * @retval CHIP_NO_ERROR on success
  * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to maintain the temporary `noc` and `icac` cert copies
  * @retval CHIP_ERROR_INVALID_ARGUMENT if either the noc or icac are invalid sizes
  * @retval CHIP_ERROR_INCORRECT_STATE if the certificate store is not properly initialized, if this method
  * is called after `AddNewOpCertsForFabric`, or if there was
- * already a pending cert chain for the given `fabricIndex`,
- * or if there is no associated persisted root for for the given `fabricIndex`.
+ * already a pending cert chain for the given `fabricIndex`, or if there are
+ * no associated persisted root and NOC chain for for the given `fabricIndex`.
  * @retval other CHIP_ERROR value on internal errors
  */
  virtual CHIP_ERROR UpdateOpCertsForFabric(FabricIndex fabricIndex, const ByteSpan & noc, const ByteSpan & icac) = 0;
 
  /**
- * @brief Permanently commit the certificate chain last setup via successful calls to
+ * @brief Permanently commit the certificate chain last configured via successful calls to
  * legal combinations of `AddNewTrustedRootCertForFabric`, `AddNewOpCertsForFabric` or
  * `UpdateOpCertsForFabric`, replacing previously committed data, if any.
  *
@@ -187,24 +188,23 @@ class OperationalCertificateStore
  *
  * @retval CHIP_NO_ERROR on success
  * @retval CHIP_ERROR_INCORRECT_STATE if the certificate store is not properly initialized,
- * or if not valid pending state is available.
- * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if there are no pending certificate chain for `fabricIndex`
+ * or if no valid pending state is available.
+ * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if there is no pending certificate chain for `fabricIndex`
  * @retval other CHIP_ERROR value on internal storage errors
  */
  virtual CHIP_ERROR CommitOpCertsForFabric(FabricIndex fabricIndex) = 0;
 
  /**
  * @brief Permanently remove the certificate chain associated with a fabric.
  *
- * This is to be used for fail-safe handling and RemoveFabric. Removes both the
- * pending operational cert chain elements for the fabricIndex (if any) and the committed
- * ones (if any).
+ * This is to be used for RemoveFabric. Removes both the pending operational cert chain
+ * elements for the fabricIndex (if any) and the committed ones (if any).
  *
  * @param fabricIndex - FabricIndex for which to remove the operational cert chain
  *
  * @retval CHIP_NO_ERROR on success
  * @retval CHIP_ERROR_INCORRECT_STATE if the certificate store is not properly initialized.
- * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if there was not operational certificate data at all for `fabricIndex`
+ * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if there was no operational certificate data at all for `fabricIndex`
  * @retval other CHIP_ERROR value on internal storage errors
  */
  virtual CHIP_ERROR RemoveOpCertsForFabric(FabricIndex fabricIndex) = 0;
@@ -234,7 +234,7 @@ class OperationalCertificateStore
  * @param outCertificate - buffer to contain the certificate obtained from persistent or temporary storage
  *
  * @retval CHIP_NO_ERROR on success.
- * @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outCertificate` does not fit the certificate.
+ * @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outCertificate` is too small to fit the certificate found.
  * @retval CHIP_ERROR_INCORRECT_STATE if the certificate store is not properly initialized.
  * @retval CHIP_ERROR_NOT_FOUND if the element cannot be found.
  * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if the fabricIndex is invalid.

src/credentials/PersistentStorageOpCertStore.cpp

@@ -220,12 +220,11 @@ CHIP_ERROR PersistentStorageOpCertStore::AddNewTrustedRootCertForFabric(FabricIn
  CHIP_ERROR_INCORRECT_STATE);
  ReturnErrorCodeIf(StorageHasCertificate(mStorage, fabricIndex, CertChainElement::kRcac), CHIP_ERROR_INCORRECT_STATE);
 
- Platform::ScopedMemoryBuffer<uint8_t> rcacBuf;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> rcacBuf;
  ReturnErrorCodeIf(!rcacBuf.Alloc(rcac.size()), CHIP_ERROR_NO_MEMORY);
  memcpy(rcacBuf.Get(), rcac.data(), rcac.size());
 
- mPendingRcac = std::move(rcacBuf);
- mPendingRcacSize = static_cast<uint16_t>(rcac.size());
+ mPendingRcac = std::move(rcacBuf);
 
  mPendingFabricIndex = fabricIndex;
  mStateFlags.Set(StateFlags::kAddNewTrustedRootCalled);
@@ -255,24 +254,19 @@ CHIP_ERROR PersistentStorageOpCertStore::AddNewOpCertsForFabric(FabricIndex fabr
  ReturnErrorCodeIf(StorageHasCertificate(mStorage, fabricIndex, CertChainElement::kNoc), CHIP_ERROR_INCORRECT_STATE);
  ReturnErrorCodeIf(StorageHasCertificate(mStorage, fabricIndex, CertChainElement::kIcac), CHIP_ERROR_INCORRECT_STATE);
 
- Platform::ScopedMemoryBuffer<uint8_t> nocBuf;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> nocBuf;
  ReturnErrorCodeIf(!nocBuf.Alloc(noc.size()), CHIP_ERROR_NO_MEMORY);
  memcpy(nocBuf.Get(), noc.data(), noc.size());
 
- Platform::ScopedMemoryBuffer<uint8_t> icacBuf;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> icacBuf;
  if (icac.size() > 0)
  {
  ReturnErrorCodeIf(!icacBuf.Alloc(icac.size()), CHIP_ERROR_NO_MEMORY);
  memcpy(icacBuf.Get(), icac.data(), icac.size());
  }
 
- mPendingNoc = std::move(nocBuf);
- mPendingNocSize = static_cast<uint16_t>(noc.size());
-
- mPendingIcac = std::move(icacBuf);
- mPendingIcacSize = static_cast<uint16_t>(icac.size());
-
- mPendingFabricIndex = fabricIndex;
+ mPendingNoc = std::move(nocBuf);
+ mPendingIcac = std::move(icacBuf);
 
  mStateFlags.Set(StateFlags::kAddNewOpCertsCalled);
 
@@ -303,22 +297,19 @@ CHIP_ERROR PersistentStorageOpCertStore::UpdateOpCertsForFabric(FabricIndex fabr
  // Don't check for ICAC, we may not have had one before, but assume that if NOC is there, a
  // previous chain was at least partially there
 
- Platform::ScopedMemoryBuffer<uint8_t> nocBuf;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> nocBuf;
  ReturnErrorCodeIf(!nocBuf.Alloc(noc.size()), CHIP_ERROR_NO_MEMORY);
  memcpy(nocBuf.Get(), noc.data(), noc.size());
 
- Platform::ScopedMemoryBuffer<uint8_t> icacBuf;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> icacBuf;
  if (icac.size() > 0)
  {
  ReturnErrorCodeIf(!icacBuf.Alloc(icac.size()), CHIP_ERROR_NO_MEMORY);
  memcpy(icacBuf.Get(), icac.data(), icac.size());
  }
 
- mPendingNoc = std::move(nocBuf);
- mPendingNocSize = static_cast<uint16_t>(noc.size());
-
- mPendingIcac = std::move(icacBuf);
- mPendingIcacSize = static_cast<uint16_t>(icac.size());
+ mPendingNoc = std::move(nocBuf);
+ mPendingIcac = std::move(icacBuf);
 
  // For NOC update, UpdateOpCertsForFabric is what determines the pending fabric index,
  // not a previous AddNewTrustedRootCertForFabric call.
@@ -346,17 +337,17 @@ CHIP_ERROR PersistentStorageOpCertStore::CommitOpCertsForFabric(FabricIndex fabr
  // TODO: Handle transaction marking to revert partial certs at next boot if we get interrupted by reboot.
 
  // Start committing NOC first so we don't have dangling roots if one was added.
- ByteSpan pendingNocSpan{ mPendingNoc.Get(), mPendingNocSize };
+ ByteSpan pendingNocSpan{ mPendingNoc.Get(), mPendingNoc.AllocatedSize() };
  CHIP_ERROR nocErr = SaveCertToStorage(mStorage, mPendingFabricIndex, CertChainElement::kNoc, pendingNocSpan);
 
  // ICAC storage handles deleting on empty/missing
- ByteSpan pendingIcacSpan{ mPendingIcac.Get(), mPendingIcacSize };
+ ByteSpan pendingIcacSpan{ mPendingIcac.Get(), mPendingIcac.AllocatedSize() };
  CHIP_ERROR icacErr = SaveCertToStorage(mStorage, mPendingFabricIndex, CertChainElement::kIcac, pendingIcacSpan);
 
  CHIP_ERROR rcacErr = CHIP_NO_ERROR;
  if (HasPendingRootCert())
  {
- ByteSpan pendingRcacSpan{ mPendingRcac.Get(), mPendingRcacSize };
+ ByteSpan pendingRcacSpan{ mPendingRcac.Get(), mPendingRcac.AllocatedSize() };
  rcacErr = SaveCertToStorage(mStorage, mPendingFabricIndex, CertChainElement::kRcac, pendingRcacSpan);
  }
 
@@ -401,7 +392,6 @@ bool PersistentStorageOpCertStore::HasAnyCertificateForFabric(FabricIndex fabric
  bool nocMissing = !StorageHasCertificate(mStorage, fabricIndex, CertChainElement::kNoc);
  bool anyPending = (mPendingRcac.Get() != nullptr) || (mPendingIcac.Get() != nullptr) || (mPendingNoc.Get() != nullptr);
 
- // If there was *no* state, pending or persisted, we have an error
  if (rcacMissing && icacMissing && nocMissing && !anyPending)
  {
  return false;
@@ -453,21 +443,21 @@ CHIP_ERROR PersistentStorageOpCertStore::GetPendingCertificate(FabricIndex fabri
  case CertChainElement::kRcac:
  if (mPendingRcac.Get() != nullptr)
  {
- ByteSpan rcacSpan{ mPendingRcac.Get(), static_cast<size_t>(mPendingRcacSize) };
+ ByteSpan rcacSpan{ mPendingRcac.Get(), mPendingRcac.AllocatedSize() };
  return CopySpanToMutableSpan(rcacSpan, outCertificate);
  }
  break;
  case CertChainElement::kIcac:
  if (mPendingIcac.Get() != nullptr)
  {
- ByteSpan icacSpan{ mPendingIcac.Get(), static_cast<size_t>(mPendingIcacSize) };
+ ByteSpan icacSpan{ mPendingIcac.Get(), mPendingIcac.AllocatedSize() };
  return CopySpanToMutableSpan(icacSpan, outCertificate);
  }
  break;
  case CertChainElement::kNoc:
  if (mPendingNoc.Get() != nullptr)
  {
- ByteSpan nocSpan{ mPendingNoc.Get(), static_cast<size_t>(mPendingNocSize) };
+ ByteSpan nocSpan{ mPendingNoc.Get(), mPendingNoc.AllocatedSize() };
  return CopySpanToMutableSpan(nocSpan, outCertificate);
  }
  break;

src/credentials/PersistentStorageOpCertStore.h

@@ -86,10 +86,6 @@ class PersistentStorageOpCertStore : public OperationalCertificateStore
  mPendingIcac.Free();
  mPendingNoc.Free();
 
- mPendingRcacSize = 0;
- mPendingIcacSize = 0;
- mPendingNocSize = 0;
-
  mPendingFabricIndex = kUndefinedFabricIndex;
  mStateFlags.ClearAll();
  }
@@ -116,13 +112,9 @@ class PersistentStorageOpCertStore : public OperationalCertificateStore
  // This pending fabric index is `kUndefinedFabricIndex` if there are no pending certs at all for the fabric
  FabricIndex mPendingFabricIndex = kUndefinedFabricIndex;
 
- Platform::ScopedMemoryBuffer<uint8_t> mPendingRcac;
- Platform::ScopedMemoryBuffer<uint8_t> mPendingIcac;
- Platform::ScopedMemoryBuffer<uint8_t> mPendingNoc;
-
- uint16_t mPendingRcacSize = 0;
- uint16_t mPendingIcacSize = 0;
- uint16_t mPendingNocSize = 0;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> mPendingRcac;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> mPendingIcac;
+ Platform::ScopedMemoryBufferWithSize<uint8_t> mPendingNoc;
 
  BitFlags<StateFlags> mStateFlags;
 };

src/lib/support/ScopedBuffer.h

@@ -187,7 +187,13 @@ class ScopedMemoryBufferWithSize : public ScopedMemoryBuffer<T>
  ~ScopedMemoryBufferWithSize() { mSize = 0; }
 
  // return the size in bytes
- inline size_t BufferByteSize() const { return mSize; }
+ inline size_t AllocatedSize() const { return mSize; }
+
+ void Free()
+ {
+ mSize = 0;
+ ScopedMemoryBuffer<T>::Free();
+ }
 
  T * Release()
  {