[HVAC] Added Enhanced Scheduling feature for TRV app #38839
Conversation
kliffwong
force-pushed
the
mr/add-trv-msch
branch
from
ad84990 to
e702a20
Compare
kliffwong
force-pushed
the
mr/add-trv-msch
branch
from
e702a20 to
0560f34
Compare
|
PR #38839: Size comparison from 8167c5f to 0560f34 Increases above 0.2%:
Full report (3 builds for cc32xx, stm32)
|
There was a problem hiding this comment.
Manual is insufficient detail about what was tested.
please provide detailed description: what did you test, why, what were the observed results, justify why automated testing is impossible in this case.
Cluster functionality seems like it should be automatable. Please try to automate this.
kliffwong
changed the title
| if (!newScheduleHandle.IsNull()) | ||
| { | ||
| size_t newScheduleHandleSize = newScheduleHandle.Value().size(); | ||
| if (newScheduleHandleSize > kScheduleHandleSize) |
There was a problem hiding this comment.
| if (newScheduleHandleSize > kScheduleHandleSize) | |
| if (newScheduleHandleSize > sizeof(scheduleHandleData)) |
and same in the logging would be clearer.
| if (newName.HasValue()) | ||
| { | ||
| size_t newNameSize = newName.Value().size(); | ||
| if (newNameSize > kScheduleNameSize) |
There was a problem hiding this comment.
Same as for the handle.
| if (newPresetHandle.HasValue()) | ||
| { | ||
| size_t newPresetHandleSize = newPresetHandle.Value().size(); | ||
| if (newPresetHandleSize > kSchedulePresetHandleSize) |
|
|
||
| for (size_t i = 0; i < scheduleTransitionSize; i++) | ||
| { | ||
| scheduleTransitionData[i] = newTransitions[i]; |
There was a problem hiding this comment.
This does not seem to be keeping a copy of the PresetHandle in the ScheduleTransitionStruct, so this looks like a likely use-after-free.
| builtIn = newBuiltIn; | ||
| } | ||
|
|
||
| Structs::ScheduleTransitionStruct::Type * ScheduleStructWithOwnedMembers::GetTransitionData() const |
There was a problem hiding this comment.
How would an API consumer use this?
And in fact, this seems to be unused....
| while (iter.Next()) | ||
| { | ||
| ScheduleStruct::DecodableType decodeSchedule = iter.GetValue(); | ||
| ScheduleTransitionStruct::Type transitions[5]; |
There was a problem hiding this comment.
Where is this 5 coming from?
|
|
||
| while (iter2.Next()) | ||
| { | ||
| transitions[i] = iter2.GetValue(); |
There was a problem hiding this comment.
This is a security bug.
| { | ||
| transitions[i] = iter2.GetValue(); | ||
| i++; | ||
| } |
There was a problem hiding this comment.
Where's the iterator status check?
|
|
||
| transitions[i] = iter.GetValue(); | ||
| i++; | ||
| } |
There was a problem hiding this comment.
Missing validity check.
| schedule.scheduleHandle = decodeSchedule.scheduleHandle; | ||
| schedule.name = decodeSchedule.name; | ||
| schedule.presetHandle = decodeSchedule.presetHandle; | ||
| schedule.transitions = DataModel::List<const Structs::ScheduleTransitionStruct::Type>(transitions.get(), i); |
There was a problem hiding this comment.
So why did we heap-allocate the transitions? Couldn't we have just put them on the stack?
Testing