[BUG] Encoding embedded 0s in UTF8 strings

[andy31415]

Reproduction steps

Ran a commissioning locally to capture trace results. Something like ./out/linux-x64-chip-tool/chip-tool pairing onnetwork 1 20202021 --trace-to json:$HOME/tmp/pairing.json

Looking at the content, the read of timezones decodes in TLV as:

...
Record { tag: ContextSpecific { tag: 2 }, value: ContainerStart(Array) }
            Record { tag: Anonymous, value: ContainerStart(Structure) }
            Record { tag: ContextSpecific { tag: 0 }, value: Signed(0) }
            Record { tag: ContextSpecific { tag: 1 }, value: Unsigned(0) }
            Record { tag: ContextSpecific { tag: 2 }, value: Utf8([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) }
...

That UTF8 string (looks like tag2 == optional char_string<64> name = 2 ) looks very much invalid. SHould have been empty string (smaller payload and generally correct UTF8)

GitHub hash of the SDK that was being used

1813bfa

Platform

linux

[tcarmelveilleux]

Spec of Matter-TLV:

A.11.2. UTF-8 and Octet Strings
UTF-8 and octet strings are encoded as follows:

Control Octet Tag Length Value
1 octet

0 to 8 octets

1 to 8 octets

0 to 264-1 octets

The length field of a UTF-8 or octet string encodes the number of octets (not characters) present in the value field. The number of octets in the length field is implied by the type specified in the element type field (within the control octet).

For octet strings, the value can be any arbitrary sequence of octets. For UTF-8 strings, the value octets SHALL encode a valid UTF-8 character (code points) sequence. Senders SHALL NOT include a terminating null character to mark the end of a string.

Having 1 terminating null (let alone 10) is forbidden. Our TLVWriter must error-out on this.

[bzbarsky-apple]

void TimeSynchronizationServer::InitTimeZone()
{
    mTimeZoneObj.validSize    = 1; // one default time zone item is needed
    mTimeZoneObj.timeZoneList = Span<TimeSyncDataProvider::TimeZoneStore>(mTz);
    for (auto & tzStore : mTimeZoneObj.timeZoneList)
    {
        memset(tzStore.name, 0, sizeof(tzStore.name));
        tzStore.timeZone = { .offset = 0, .validAt = 0, .name = MakeOptional(CharSpan(tzStore.name, sizeof(tzStore.name))) };

This part is initializing the timezone name to "64 0 bytes". Why @fessehaeve @cecille ? Shouldn't it default to "no name" (i.e. NullOptional), just like it would if timezone gets set to something with no name via command?

[andy31415]

void TimeSynchronizationServer::InitTimeZone()
{
    mTimeZoneObj.validSize    = 1; // one default time zone item is needed
    mTimeZoneObj.timeZoneList = Span<TimeSyncDataProvider::TimeZoneStore>(mTz);
    for (auto & tzStore : mTimeZoneObj.timeZoneList)
    {
        memset(tzStore.name, 0, sizeof(tzStore.name));
        tzStore.timeZone = { .offset = 0, .validAt = 0, .name = MakeOptional(CharSpan(tzStore.name, sizeof(tzStore.name))) };

This part is initializing the timezone name to "64 0 bytes". Why @fessehaeve @cecille ? Shouldn't it default to "no name" (i.e. NullOptional), just like it would if timezone gets set to something with no name via command?

Not only that. This is also wrong:

 576   │     if (lastTzState != TimeState::kInvalid)
 577   │     {
 578   │         const auto & tzStore = GetTimeZone()[0];
 579   │         lastTz.offset        = tzStore.timeZone.offset;
 580   │         if (tzStore.timeZone.name.HasValue())
 581   │         {
 582   │             lastTz.name.SetValue(CharSpan(name));   /// HERE - this is sizeof as well
 583   │             memcpy(name, tzStore.name, sizeof(tzStore.name));
 584   │         }
 585   │     }

And because we serialized char-spans with embedded 0s (64 of them), they will be wrong on load as well and that has to be fixed too (so it auto-corrects old wrong data).

This code needs significant review and not sure how easy that even is.

[fessehaeve]

If this is not urgent, I can commit to fixing this (in timesync*) post the Nov 20 deadline for v1.3.