Skip to content

Commit

Permalink
ZCLWrite should return "INVALID_VALUE" instead of success & truncatio…
Browse files Browse the repository at this point in the history 
…n in case of invalid payloads. (#9039)

* [python] Add check for value size for attribute write

* Update test case

* Run Codegen
  • Loading branch information
@erjiaqing
erjiaqing authored Aug 18, 2021
1 parent 26c0e35 commit 303c02c
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 58 deletions.
22 changes: 11 additions & 11 deletions examples/chip-tool/commands/tests/Commands.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8681,7 +8681,7 @@ class TestCluster : public TestCommand
chip::Callback::Callback<DefaultFailureCallback> mOnFailureCallback_91{
OnTestSendClusterTestClusterCommandWriteAttribute_91_FailureResponse, this
};
bool mIsFailureExpected_91 = 0;
bool mIsFailureExpected_91 = true;

CHIP_ERROR TestSendClusterTestClusterCommandWriteAttribute_91()
{
Expand Down Expand Up @@ -9198,7 +9198,7 @@ class TestCluster : public TestCommand

CHIP_ERROR err = CHIP_NO_ERROR;

chip::ByteSpan charStringArgument = chip::ByteSpan(chip::Uint8::from_const_char("☉Test☉"), strlen("☉Test☉"));
chip::ByteSpan charStringArgument = chip::ByteSpan(chip::Uint8::from_const_char("☉T☉"), strlen("☉T☉"));
err = cluster.WriteAttributeCharString(mOnSuccessCallback_99.Cancel(), mOnFailureCallback_99.Cancel(), charStringArgument);

return err;
Expand Down Expand Up @@ -9236,19 +9236,19 @@ class TestCluster : public TestCommand
runner->NextTest();
}

// Test Write attribute CHAR_STRING
// Test Write attribute CHAR_STRING - Value too long
using SuccessCallback_100 = void (*)(void * context, chip::ByteSpan charString);
chip::Callback::Callback<SuccessCallback_100> mOnSuccessCallback_100{
OnTestSendClusterTestClusterCommandWriteAttribute_100_SuccessResponse, this
};
chip::Callback::Callback<DefaultFailureCallback> mOnFailureCallback_100{
OnTestSendClusterTestClusterCommandWriteAttribute_100_FailureResponse, this
};
bool mIsFailureExpected_100 = 0;
bool mIsFailureExpected_100 = true;

CHIP_ERROR TestSendClusterTestClusterCommandWriteAttribute_100()
{
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING: Sending command...");
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING - Value too long: Sending command...");

chip::Controller::TestClusterCluster cluster;
cluster.Associate(mDevice, 1);
Expand All @@ -9265,7 +9265,7 @@ class TestCluster : public TestCommand

static void OnTestSendClusterTestClusterCommandWriteAttribute_100_FailureResponse(void * context, uint8_t status)
{
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING: Failure Response");
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING - Value too long: Failure Response");

TestCluster * runner = reinterpret_cast<TestCluster *>(context);

Expand All @@ -9281,7 +9281,7 @@ class TestCluster : public TestCommand

static void OnTestSendClusterTestClusterCommandWriteAttribute_100_SuccessResponse(void * context, chip::ByteSpan charString)
{
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING: Success Response");
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING - Value too long: Success Response");

TestCluster * runner = reinterpret_cast<TestCluster *>(context);

Expand All @@ -9295,7 +9295,7 @@ class TestCluster : public TestCommand
runner->NextTest();
}

// Test Write attribute CHAR_STRING
// Test Write attribute CHAR_STRING - Empty
using SuccessCallback_101 = void (*)(void * context, chip::ByteSpan charString);
chip::Callback::Callback<SuccessCallback_101> mOnSuccessCallback_101{
OnTestSendClusterTestClusterCommandWriteAttribute_101_SuccessResponse, this
Expand All @@ -9307,7 +9307,7 @@ class TestCluster : public TestCommand

CHIP_ERROR TestSendClusterTestClusterCommandWriteAttribute_101()
{
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING: Sending command...");
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING - Empty: Sending command...");

chip::Controller::TestClusterCluster cluster;
cluster.Associate(mDevice, 1);
Expand All @@ -9323,7 +9323,7 @@ class TestCluster : public TestCommand

static void OnTestSendClusterTestClusterCommandWriteAttribute_101_FailureResponse(void * context, uint8_t status)
{
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING: Failure Response");
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING - Empty: Failure Response");

TestCluster * runner = reinterpret_cast<TestCluster *>(context);

Expand All @@ -9339,7 +9339,7 @@ class TestCluster : public TestCommand

static void OnTestSendClusterTestClusterCommandWriteAttribute_101_SuccessResponse(void * context, chip::ByteSpan charString)
{
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING: Success Response");
ChipLogProgress(chipTool, "Test Cluster - Write attribute CHAR_STRING - Empty: Success Response");

TestCluster * runner = reinterpret_cast<TestCluster *>(context);

Expand Down
12 changes: 8 additions & 4 deletions src/app/tests/suites/TestCluster.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,8 @@ tests:
attribute: "octet_string"
arguments:
value: "TestValueLongerThan10"
response:
error: true

- label: "Read attribute OCTET_STRING"
command: "readAttribute"
Expand Down Expand Up @@ -662,20 +664,22 @@ tests:
command: "writeAttribute"
attribute: "char_string"
arguments:
value: "☉Test☉"
value: "☉T☉"

- label: "Read attribute CHAR_STRING"
command: "readAttribute"
attribute: "char_string"
disabled: true
response:
value: "☉Test☉"
value: "☉T☉"

- label: "Write attribute CHAR_STRING"
- label: "Write attribute CHAR_STRING - Value too long"
command: "writeAttribute"
attribute: "char_string"
arguments:
value: "☉TestValueLongerThan10☉"
response:
error: true

- label: "Read attribute CHAR_STRING"
command: "readAttribute"
Expand All @@ -684,7 +688,7 @@ tests:
response:
value: "☉TestVal"

- label: "Write attribute CHAR_STRING"
- label: "Write attribute CHAR_STRING - Empty"
command: "writeAttribute"
attribute: "char_string"
arguments:
Expand Down
62 changes: 33 additions & 29 deletions src/app/util/ember-compatibility-functions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ EmberAfAttributeType BaseType(EmberAfAttributeType type)
"chip::Cluster is expected to be uint32_t, change this when necessary");
static_assert(std::is_same<chip::AttributeId, uint32_t>::value,
"chip::AttributeId is expected to be uint32_t, change this when necessary");
static_assert(std::is_same<chip::FieldId, uint32_t>::value,
"chip::FieldId is expected to be uint32_t, change this when necessary");
static_assert(std::is_same<chip::AttributeId, uint32_t>::value,
"chip::AttributeId is expected to be uint32_t, change this when necessary");
static_assert(std::is_same<chip::EventId, uint32_t>::value,
"chip::EventId is expected to be uint32_t, change this when necessary");
static_assert(std::is_same<chip::CommandId, uint32_t>::value,
Expand Down Expand Up @@ -195,7 +195,7 @@ CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * ap
{
ChipLogDetail(DataManagement,
"Received Cluster Command: Cluster=" ChipLogFormatMEI " NodeId=0x" ChipLogFormatX64 " Endpoint=%" PRIx16
" FieldId=%" PRIx32 " ListIndex=%" PRIx16,
" AttributeId=%" PRIx32 " ListIndex=%" PRIx16,
ChipLogValueMEI(aClusterInfo.mClusterId), ChipLogValueX64(aClusterInfo.mNodeId), aClusterInfo.mEndpointId,
aClusterInfo.mFieldId, aClusterInfo.mListIndex);

Expand Down Expand Up @@ -410,46 +410,50 @@ CHIP_ERROR prepareWriteData(EmberAfAttributeType expectedType, TLV::TLVReader &
}
} // namespace

CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
static Protocols::InteractionModel::ProtocolCode
WriteSingleClusterDataInternal(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
{
AttributePathParams attributePathParams;
attributePathParams.mNodeId = aClusterInfo.mNodeId;
attributePathParams.mEndpointId = aClusterInfo.mEndpointId;
attributePathParams.mClusterId = aClusterInfo.mClusterId;
attributePathParams.mFieldId = aClusterInfo.mFieldId;
attributePathParams.mFlags.Set(AttributePathParams::Flags::kFieldIdValid);

EmberAfAttributeType attributeType = ZCL_UNKNOWN_ATTRIBUTE_TYPE;

// Passing nullptr as buf to emberAfReadAttribute means we only need attribute type here, and ember will not do data read &
// copy in this case.
EmberAfStatus status = emberAfReadAttribute(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mFieldId,
CLUSTER_MASK_SERVER, nullptr, 0, &attributeType);
Protocols::InteractionModel::ProtocolCode imCode = Protocols::InteractionModel::ProtocolCode::Success;
const EmberAfAttributeMetadata * attributeMetadata = emberAfLocateAttributeMetadata(
aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mFieldId, CLUSTER_MASK_SERVER, 0);

if (EMBER_ZCL_STATUS_SUCCESS != status)
if (attributeMetadata == nullptr)
{
return apWriteHandler->AddAttributeStatusCode(attributePathParams, Protocols::SecureChannel::GeneralStatusCode::kFailure,
Protocols::SecureChannel::Id,
Protocols::InteractionModel::ProtocolCode::UnsupportedAttribute);
return Protocols::InteractionModel::ProtocolCode::UnsupportedAttribute;
}

CHIP_ERROR preparationError = CHIP_NO_ERROR;
uint16_t dataLen = 0;
if ((preparationError = prepareWriteData(attributeType, aReader, dataLen)) == CHIP_NO_ERROR)
if ((preparationError = prepareWriteData(attributeMetadata->attributeType, aReader, dataLen)) != CHIP_NO_ERROR)
{
// TODO (#8442): emberAfWriteAttributeExternal is doing additional ACL check, however true ACL support is missing in ember /
// IM. Should invesgate this function and integrate ACL support with related interactions.
imCode = ToInteractionModelProtocolCode(emberAfWriteAttributeExternal(aClusterInfo.mEndpointId, aClusterInfo.mClusterId,
aClusterInfo.mFieldId, CLUSTER_MASK_SERVER, 0,
attributeData, attributeType));
ChipLogDetail(Zcl, "Failed to preapre data to write: %s", ErrorStr(preparationError));
return Protocols::InteractionModel::ProtocolCode::InvalidValue;
}
else

if (dataLen > attributeMetadata->size)
{
ChipLogError(Zcl, "Failed to preapre data to write: %s", ErrorStr(preparationError));
imCode = Protocols::InteractionModel::ProtocolCode::InvalidValue;
ChipLogDetail(Zcl, "Data to write exceedes the attribute size claimed.");
return Protocols::InteractionModel::ProtocolCode::InvalidValue;
}

// TODO (#8442): emberAfWriteAttributeExternal is doing additional ACL check, however true ACL support is missing in ember /
// IM. Should invesgate this function and integrate ACL support with related interactions.
return ToInteractionModelProtocolCode(emberAfWriteAttributeExternal(aClusterInfo.mEndpointId, aClusterInfo.mClusterId,
aClusterInfo.mFieldId, CLUSTER_MASK_SERVER, 0,
attributeData, attributeMetadata->attributeType));
}

CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
{
AttributePathParams attributePathParams;
attributePathParams.mNodeId = aClusterInfo.mNodeId;
attributePathParams.mEndpointId = aClusterInfo.mEndpointId;
attributePathParams.mClusterId = aClusterInfo.mClusterId;
attributePathParams.mFieldId = aClusterInfo.mFieldId;
attributePathParams.mFlags.Set(AttributePathParams::Flags::kFieldIdValid);

auto imCode = WriteSingleClusterDataInternal(aClusterInfo, aReader, apWriteHandler);
return apWriteHandler->AddAttributeStatusCode(attributePathParams,
imCode == Protocols::InteractionModel::ProtocolCode::Success
? Protocols::SecureChannel::GeneralStatusCode::kSuccess
Expand Down
1 change: 1 addition & 0 deletions src/controller/python/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ pw_python_action("python") {
"chip/discovery/library_handle.py",
"chip/discovery/types.py",
"chip/exceptions/__init__.py",
"chip/interaction_model/__init__.py",
"chip/interaction_model/delegate.py",
"chip/internal/__init__.py",
"chip/internal/commissioner.py",
Expand Down
46 changes: 45 additions & 1 deletion src/controller/python/chip/interaction_model/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,49 @@
#

"""Provides Python APIs for CHIP."""
from construct.core import EnumInteger
__all__ = ["IMDelegate", "ProtocolCode"]

__all__ = ["IMDelegate"]

class ProtocolCode(EnumInteger):
Success = 0x0
Failure = 0x01
InvalidSubscription = 0x7d
UnsupportedAccess = 0x7e
UnsupportedEndpoint = 0x7f
InvalidAction = 0x80
UnsupportedCommand = 0x81
Deprecated82 = 0x82
Deprecated83 = 0x83
Deprecated84 = 0x84
InvalidCommand = 0x85
UnsupportedAttribute = 0x86
InvalidValue = 0x87
UnsupportedWrite = 0x88
ResourceExhausted = 0x89
Deprecated8a = 0x8a
NotFound = 0x8b
UnreportableAttribute = 0x8c
InvalidDataType = 0x8d
Deprecated8e = 0x8e
UnsupportedRead = 0x8f
Deprecated90 = 0x90
Deprecated91 = 0x91
Reserved92 = 0x92
Deprecated93 = 0x93
Timeout = 0x94
Reserved95 = 0x95
Reserved96 = 0x96
Reserved97 = 0x97
Reserved98 = 0x98
Reserved99 = 0x99
Reserved9a = 0x9a
ConstraintError = 0x9b
Busy = 0x9c
Deprecatedc0 = 0xc0
Deprecatedc1 = 0xc1
Deprecatedc2 = 0xc2
UnsupportedCluster = 0xc3
Deprecatedc4 = 0xc4
NoUpstreamSubscription = 0xc5
InvalidArgument = 0xc6
17 changes: 11 additions & 6 deletions src/controller/python/test/test_scripts/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from dataclasses import dataclass
from typing import Any
from chip import ChipDeviceCtrl
import chip.interaction_model as IM
import threading
import os
import sys
Expand Down Expand Up @@ -172,7 +173,6 @@ def TestReadBasicAttribiutes(self, nodeid: int, endpoint: int, group: int):
elif res.value != expected_value:
raise Exception("Read {} attribute: expect {} got {}".format(
basic_attr, repr(expected_value), repr(res.value)))
time.sleep(2)
except Exception as ex:
failed_zcl[basic_attr] = str(ex)
if failed_zcl:
Expand All @@ -186,9 +186,12 @@ class AttributeWriteRequest:
cluster: str
attribute: str
value: Any
expected_status: IM.ProtocolCode = IM.ProtocolCode.Success

requests = [
AttributeWriteRequest("Basic", "UserLabel", "Test"),
AttributeWriteRequest("Basic", "Location",
"a pretty loooooooooooooog string", IM.ProtocolCode.InvalidValue),
]
failed_zcl = []
for req in requests:
Expand All @@ -202,10 +205,12 @@ class AttributeWriteRequest:
if res is None:
raise Exception(
f"Write {req.cluster}.{req.attribute} attribute: no value get")
elif res.status != 0:
elif res.status != req.expected_status:
raise Exception(
f"Write {req.cluster}.{req.attribute} attribute: non-zero status code {res.status}")
time.sleep(2)
f"Write {req.cluster}.{req.attribute} attribute: expected status is {req.expected_status} got {res.status}")
if req.expected_status != IM.ProtocolCode.Success:
# If the write interaction is expected to success, proceed to verify it.
continue
res = self.devCtrl.ZCLReadAttribute(
cluster=req.cluster, attribute=req.attribute, nodeid=nodeid, endpoint=endpoint, groupid=group)
if res is None:
Expand All @@ -217,8 +222,8 @@ class AttributeWriteRequest:
elif res.value != req.value:
raise Exception(
f"Read written {req.cluster}.{req.attribute} attribute: expected {req.value} got {res.value}")
except Exception:
failed_zcl.append(req)
except Exception as ex:
failed_zcl.append(str(ex))
if failed_zcl:
self.logger.exception(f"Following attributes failed: {failed_zcl}")
return False
Expand Down
Loading

0 comments on commit 303c02c

Please sign in to comment.