Skip to content

Commit 74768a8

Browse files
ksperling-appleksperling-apple
authored
ThreadOperationalDataset: various bug fixes (#34331)
* ThreadOperationalDataset: various bug fixes - Ensure TLVs read have the correct length - Default construct as empty (mLength == 0) - Change MakeRoom() size argument to size_t to avoid chance of truncation - Check for null in SetNetworkName - Use Encoding::BigEndian instead of hand-rolled math - Use ReturnErrorOnFailure / VerifyOrReturnError consistently - Make tests independent from each other (non-static dataset member) - Add more tests * Fix includes
1 parent 0d740d0 commit 74768a8

File tree

3 files changed

+191
-250
lines changed

3 files changed

+191
-250
lines changed

‎src/lib/support/ThreadOperationalDataset.cpp

+65-171
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@
1515
* limitations under the License.
1616
*/
1717

18-
#include <assert.h>
19-
#include <string.h>
20-
2118
#include <lib/support/ThreadOperationalDataset.h>
2219

20+
#include <lib/core/CHIPEncoding.h>
21+
22+
#include <cassert>
23+
#include <cstring>
24+
2325
namespace chip {
2426
namespace Thread {
2527

@@ -70,7 +72,7 @@ class ThreadTLV final
7072
mLength = aLength;
7173
}
7274

73-
const void * GetValue() const
75+
const uint8_t * GetValue() const
7476
{
7577
assert(mLength != kLengthEscape);
7678

@@ -79,75 +81,44 @@ class ThreadTLV final
7981
return reinterpret_cast<const uint8_t *>(this) + sizeof(*this);
8082
}
8183

82-
void * GetValue() { return const_cast<void *>(const_cast<const ThreadTLV *>(this)->GetValue()); }
84+
uint8_t * GetValue() { return const_cast<uint8_t *>(const_cast<const ThreadTLV *>(this)->GetValue()); }
85+
86+
ByteSpan GetValueAsSpan() const { return ByteSpan(static_cast<const uint8_t *>(GetValue()), GetLength()); }
8387

8488
void Get64(uint64_t & aValue) const
8589
{
8690
assert(GetLength() >= sizeof(aValue));
87-
88-
const uint8_t * p = reinterpret_cast<const uint8_t *>(GetValue());
89-
aValue = //
90-
(static_cast<uint64_t>(p[0]) << 56) | //
91-
(static_cast<uint64_t>(p[1]) << 48) | //
92-
(static_cast<uint64_t>(p[2]) << 40) | //
93-
(static_cast<uint64_t>(p[3]) << 32) | //
94-
(static_cast<uint64_t>(p[4]) << 24) | //
95-
(static_cast<uint64_t>(p[5]) << 16) | //
96-
(static_cast<uint64_t>(p[6]) << 8) | //
97-
(static_cast<uint64_t>(p[7]));
91+
aValue = Encoding::BigEndian::Get64(GetValue());
9892
}
9993

100-
void Get16(uint16_t & aValue) const
94+
void Get32(uint32_t & aValue) const
10195
{
10296
assert(GetLength() >= sizeof(aValue));
103-
104-
const uint8_t * p = static_cast<const uint8_t *>(GetValue());
105-
106-
aValue = static_cast<uint16_t>(p[0] << 8 | p[1]);
97+
aValue = Encoding::BigEndian::Get32(GetValue());
10798
}
10899

109-
void Get8(uint8_t & aValue) const
100+
void Get16(uint16_t & aValue) const
110101
{
111102
assert(GetLength() >= sizeof(aValue));
112-
aValue = *static_cast<const uint8_t *>(GetValue());
103+
aValue = Encoding::BigEndian::Get16(GetValue());
113104
}
114105

115106
void Set64(uint64_t aValue)
116107
{
117-
uint8_t * value = static_cast<uint8_t *>(GetValue());
118-
119108
SetLength(sizeof(aValue));
120-
121-
value[0] = static_cast<uint8_t>((aValue >> 56) & 0xff);
122-
value[1] = static_cast<uint8_t>((aValue >> 48) & 0xff);
123-
value[2] = static_cast<uint8_t>((aValue >> 40) & 0xff);
124-
value[3] = static_cast<uint8_t>((aValue >> 32) & 0xff);
125-
value[4] = static_cast<uint8_t>((aValue >> 24) & 0xff);
126-
value[5] = static_cast<uint8_t>((aValue >> 16) & 0xff);
127-
value[6] = static_cast<uint8_t>((aValue >> 8) & 0xff);
128-
value[7] = static_cast<uint8_t>(aValue & 0xff);
109+
Encoding::BigEndian::Put64(GetValue(), aValue);
129110
}
130111

131-
void Set16(uint16_t aValue)
112+
void Set32(uint32_t aValue)
132113
{
133-
uint8_t * value = static_cast<uint8_t *>(GetValue());
134-
135114
SetLength(sizeof(aValue));
136-
137-
value[0] = static_cast<uint8_t>(aValue >> 8);
138-
value[1] = static_cast<uint8_t>(aValue & 0xff);
139-
}
140-
141-
void Set8(uint8_t aValue)
142-
{
143-
SetLength(sizeof(aValue));
144-
*static_cast<uint8_t *>(GetValue()) = aValue;
115+
Encoding::BigEndian::Put32(GetValue(), aValue);
145116
}
146117

147-
void Set8(int8_t aValue)
118+
void Set16(uint16_t aValue)
148119
{
149120
SetLength(sizeof(aValue));
150-
*static_cast<int8_t *>(GetValue()) = aValue;
121+
Encoding::BigEndian::Put16(GetValue(), aValue);
151122
}
152123

153124
void SetValue(const void * aValue, uint8_t aLength)
@@ -218,24 +189,16 @@ CHIP_ERROR OperationalDataset::Init(ByteSpan aData)
218189
CHIP_ERROR OperationalDataset::GetActiveTimestamp(uint64_t & aActiveTimestamp) const
219190
{
220191
const ThreadTLV * tlv = Locate(ThreadTLV::kActiveTimestamp);
221-
222-
if (tlv != nullptr)
223-
{
224-
tlv->Get64(aActiveTimestamp);
225-
return CHIP_NO_ERROR;
226-
}
227-
228-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
192+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
193+
VerifyOrReturnError(tlv->GetLength() == sizeof(aActiveTimestamp), CHIP_ERROR_INVALID_TLV_ELEMENT);
194+
tlv->Get64(aActiveTimestamp);
195+
return CHIP_NO_ERROR;
229196
}
230197

231198
CHIP_ERROR OperationalDataset::SetActiveTimestamp(uint64_t aActiveTimestamp)
232199
{
233200
ThreadTLV * tlv = MakeRoom(ThreadTLV::kActiveTimestamp, sizeof(*tlv) + sizeof(aActiveTimestamp));
234-
235-
if (tlv == nullptr)
236-
{
237-
return CHIP_ERROR_NO_MEMORY;
238-
}
201+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
239202

240203
tlv->Set64(aActiveTimestamp);
241204

@@ -247,26 +210,19 @@ CHIP_ERROR OperationalDataset::SetActiveTimestamp(uint64_t aActiveTimestamp)
247210
CHIP_ERROR OperationalDataset::GetChannel(uint16_t & aChannel) const
248211
{
249212
const ThreadTLV * tlv = Locate(ThreadTLV::kChannel);
250-
251-
if (tlv != nullptr)
252-
{
253-
const uint8_t * value = reinterpret_cast<const uint8_t *>(tlv->GetValue());
254-
aChannel = static_cast<uint16_t>((value[1] << 8) | value[2]);
255-
return CHIP_NO_ERROR;
256-
}
257-
258-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
213+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
214+
VerifyOrReturnError(tlv->GetLength() == 3, CHIP_ERROR_INVALID_TLV_ELEMENT);
215+
// Note: The channel page (byte 0) is not returned
216+
const uint8_t * value = tlv->GetValue();
217+
aChannel = static_cast<uint16_t>((value[1] << 8) | value[2]);
218+
return CHIP_NO_ERROR;
259219
}
260220

261221
CHIP_ERROR OperationalDataset::SetChannel(uint16_t aChannel)
262222
{
263223
uint8_t value[] = { 0, static_cast<uint8_t>(aChannel >> 8), static_cast<uint8_t>(aChannel & 0xff) };
264224
ThreadTLV * tlv = MakeRoom(ThreadTLV::kChannel, sizeof(*tlv) + sizeof(value));
265-
266-
if (tlv == nullptr)
267-
{
268-
return CHIP_ERROR_NO_MEMORY;
269-
}
225+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
270226

271227
tlv->SetValue(value, sizeof(value));
272228

@@ -278,43 +234,24 @@ CHIP_ERROR OperationalDataset::SetChannel(uint16_t aChannel)
278234
CHIP_ERROR OperationalDataset::GetExtendedPanId(uint8_t (&aExtendedPanId)[kSizeExtendedPanId]) const
279235
{
280236
ByteSpan extPanIdSpan;
281-
CHIP_ERROR error = GetExtendedPanIdAsByteSpan(extPanIdSpan);
282-
283-
if (error != CHIP_NO_ERROR)
284-
{
285-
return error;
286-
}
287-
237+
ReturnErrorOnFailure(GetExtendedPanIdAsByteSpan(extPanIdSpan));
288238
memcpy(aExtendedPanId, extPanIdSpan.data(), extPanIdSpan.size());
289239
return CHIP_NO_ERROR;
290240
}
291241

292242
CHIP_ERROR OperationalDataset::GetExtendedPanIdAsByteSpan(ByteSpan & span) const
293243
{
294244
const ThreadTLV * tlv = Locate(ThreadTLV::kExtendedPanId);
295-
296-
if (tlv == nullptr)
297-
{
298-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
299-
}
300-
301-
if (tlv->GetLength() != kSizeExtendedPanId)
302-
{
303-
return CHIP_ERROR_INVALID_TLV_ELEMENT;
304-
}
305-
245+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
246+
VerifyOrReturnError(tlv->GetLength() == kSizeExtendedPanId, CHIP_ERROR_INVALID_TLV_ELEMENT);
306247
span = ByteSpan(static_cast<const uint8_t *>(tlv->GetValue()), tlv->GetLength());
307248
return CHIP_NO_ERROR;
308249
}
309250

310251
CHIP_ERROR OperationalDataset::SetExtendedPanId(const uint8_t (&aExtendedPanId)[kSizeExtendedPanId])
311252
{
312253
ThreadTLV * tlv = MakeRoom(ThreadTLV::kExtendedPanId, sizeof(*tlv) + sizeof(aExtendedPanId));
313-
314-
if (tlv == nullptr)
315-
{
316-
return CHIP_ERROR_NO_MEMORY;
317-
}
254+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
318255

319256
tlv->SetValue(aExtendedPanId, sizeof(aExtendedPanId));
320257

@@ -328,24 +265,16 @@ CHIP_ERROR OperationalDataset::SetExtendedPanId(const uint8_t (&aExtendedPanId)[
328265
CHIP_ERROR OperationalDataset::GetMasterKey(uint8_t (&aMasterKey)[kSizeMasterKey]) const
329266
{
330267
const ThreadTLV * tlv = Locate(ThreadTLV::kMasterKey);
331-
332-
if (tlv != nullptr)
333-
{
334-
memcpy(aMasterKey, tlv->GetValue(), sizeof(aMasterKey));
335-
return CHIP_NO_ERROR;
336-
}
337-
338-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
268+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
269+
VerifyOrReturnError(tlv->GetLength() == sizeof(aMasterKey), CHIP_ERROR_INVALID_TLV_ELEMENT);
270+
memcpy(aMasterKey, tlv->GetValue(), sizeof(aMasterKey));
271+
return CHIP_NO_ERROR;
339272
}
340273

341274
CHIP_ERROR OperationalDataset::SetMasterKey(const uint8_t (&aMasterKey)[kSizeMasterKey])
342275
{
343276
ThreadTLV * tlv = MakeRoom(ThreadTLV::kMasterKey, sizeof(*tlv) + sizeof(aMasterKey));
344-
345-
if (tlv == nullptr)
346-
{
347-
return CHIP_ERROR_NO_MEMORY;
348-
}
277+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
349278

350279
tlv->SetValue(aMasterKey, sizeof(aMasterKey));
351280

@@ -359,24 +288,16 @@ CHIP_ERROR OperationalDataset::SetMasterKey(const uint8_t (&aMasterKey)[kSizeMas
359288
CHIP_ERROR OperationalDataset::GetMeshLocalPrefix(uint8_t (&aMeshLocalPrefix)[kSizeMeshLocalPrefix]) const
360289
{
361290
const ThreadTLV * tlv = Locate(ThreadTLV::kMeshLocalPrefix);
362-
363-
if (tlv != nullptr)
364-
{
365-
memcpy(aMeshLocalPrefix, tlv->GetValue(), sizeof(aMeshLocalPrefix));
366-
return CHIP_NO_ERROR;
367-
}
368-
369-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
291+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
292+
VerifyOrReturnError(tlv->GetLength() == sizeof(aMeshLocalPrefix), CHIP_ERROR_INVALID_TLV_ELEMENT);
293+
memcpy(aMeshLocalPrefix, tlv->GetValue(), sizeof(aMeshLocalPrefix));
294+
return CHIP_NO_ERROR;
370295
}
371296

372297
CHIP_ERROR OperationalDataset::SetMeshLocalPrefix(const uint8_t (&aMeshLocalPrefix)[kSizeMeshLocalPrefix])
373298
{
374299
ThreadTLV * tlv = MakeRoom(ThreadTLV::kMeshLocalPrefix, sizeof(*tlv) + sizeof(aMeshLocalPrefix));
375-
376-
if (tlv == nullptr)
377-
{
378-
return CHIP_ERROR_NO_MEMORY;
379-
}
300+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
380301

381302
tlv->SetValue(aMeshLocalPrefix, sizeof(aMeshLocalPrefix));
382303

@@ -388,32 +309,21 @@ CHIP_ERROR OperationalDataset::SetMeshLocalPrefix(const uint8_t (&aMeshLocalPref
388309
CHIP_ERROR OperationalDataset::GetNetworkName(char (&aNetworkName)[kSizeNetworkName + 1]) const
389310
{
390311
const ThreadTLV * tlv = Locate(ThreadTLV::kNetworkName);
391-
392-
if (tlv != nullptr)
393-
{
394-
memcpy(aNetworkName, tlv->GetValue(), tlv->GetLength());
395-
aNetworkName[tlv->GetLength()] = '\0';
396-
return CHIP_NO_ERROR;
397-
}
398-
399-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
312+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
313+
VerifyOrReturnError(tlv->GetLength() <= kSizeNetworkName, CHIP_ERROR_INVALID_TLV_ELEMENT);
314+
memcpy(aNetworkName, tlv->GetValue(), tlv->GetLength());
315+
aNetworkName[tlv->GetLength()] = '\0';
316+
return CHIP_NO_ERROR;
400317
}
401318

402319
CHIP_ERROR OperationalDataset::SetNetworkName(const char * aNetworkName)
403320
{
321+
VerifyOrReturnError(aNetworkName != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
404322
size_t len = strlen(aNetworkName);
323+
VerifyOrReturnError(0 < len && len <= kSizeNetworkName, CHIP_ERROR_INVALID_STRING_LENGTH);
405324

406-
if (len > kSizeNetworkName || len == 0)
407-
{
408-
return CHIP_ERROR_INVALID_STRING_LENGTH;
409-
}
410-
411-
ThreadTLV * tlv = MakeRoom(ThreadTLV::kNetworkName, static_cast<uint8_t>(sizeof(*tlv) + static_cast<uint8_t>(len)));
412-
413-
if (tlv == nullptr)
414-
{
415-
return CHIP_ERROR_NO_MEMORY;
416-
}
325+
ThreadTLV * tlv = MakeRoom(ThreadTLV::kNetworkName, sizeof(*tlv) + len);
326+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
417327

418328
tlv->SetValue(aNetworkName, static_cast<uint8_t>(len));
419329

@@ -425,24 +335,16 @@ CHIP_ERROR OperationalDataset::SetNetworkName(const char * aNetworkName)
425335
CHIP_ERROR OperationalDataset::GetPanId(uint16_t & aPanId) const
426336
{
427337
const ThreadTLV * tlv = Locate(ThreadTLV::kPanId);
428-
429-
if (tlv != nullptr)
430-
{
431-
tlv->Get16(aPanId);
432-
return CHIP_NO_ERROR;
433-
}
434-
435-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
338+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
339+
VerifyOrReturnError(tlv->GetLength() == sizeof(aPanId), CHIP_ERROR_INVALID_TLV_ELEMENT);
340+
tlv->Get16(aPanId);
341+
return CHIP_NO_ERROR;
436342
}
437343

438344
CHIP_ERROR OperationalDataset::SetPanId(uint16_t aPanId)
439345
{
440346
ThreadTLV * tlv = MakeRoom(ThreadTLV::kPanId, sizeof(*tlv) + sizeof(aPanId));
441-
442-
if (tlv == nullptr)
443-
{
444-
return CHIP_ERROR_NO_MEMORY;
445-
}
347+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
446348

447349
tlv->Set16(aPanId);
448350

@@ -454,24 +356,16 @@ CHIP_ERROR OperationalDataset::SetPanId(uint16_t aPanId)
454356
CHIP_ERROR OperationalDataset::GetPSKc(uint8_t (&aPSKc)[kSizePSKc]) const
455357
{
456358
const ThreadTLV * tlv = Locate(ThreadTLV::kPSKc);
457-
458-
if (tlv != nullptr)
459-
{
460-
memcpy(aPSKc, tlv->GetValue(), sizeof(aPSKc));
461-
return CHIP_NO_ERROR;
462-
}
463-
464-
return CHIP_ERROR_TLV_TAG_NOT_FOUND;
359+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_TLV_TAG_NOT_FOUND);
360+
VerifyOrReturnError(tlv->GetLength() == sizeof(aPSKc), CHIP_ERROR_INVALID_TLV_ELEMENT);
361+
memcpy(aPSKc, tlv->GetValue(), sizeof(aPSKc));
362+
return CHIP_NO_ERROR;
465363
}
466364

467365
CHIP_ERROR OperationalDataset::SetPSKc(const uint8_t (&aPSKc)[kSizePSKc])
468366
{
469367
ThreadTLV * tlv = MakeRoom(ThreadTLV::kPSKc, sizeof(*tlv) + sizeof(aPSKc));
470-
471-
if (tlv == nullptr)
472-
{
473-
return CHIP_ERROR_NO_MEMORY;
474-
}
368+
VerifyOrReturnError(tlv != nullptr, CHIP_ERROR_NO_MEMORY);
475369

476370
tlv->SetValue(aPSKc, sizeof(aPSKc));
477371

@@ -533,7 +427,7 @@ void OperationalDataset::Remove(uint8_t aType)
533427
}
534428
}
535429

536-
ThreadTLV * OperationalDataset::MakeRoom(uint8_t aType, uint8_t aSize)
430+
ThreadTLV * OperationalDataset::MakeRoom(uint8_t aType, size_t aSize)
537431
{
538432
ThreadTLV * tlv = Locate(aType);
539433

0 commit comments

Comments
 (0)