Some operations on System::Clock types are not safe #10062

kpschoedel · 2021-09-29T17:14:30Z

Problem

Clock::MonotonicMicroseconds and Clock::MonotonicMilliseconds are not distinguishable from each other or uint64_t. (See e.g. PR fix typo in GetCurrentMonotonicTimeMs #10012)
Some comparisons are subject to wraparound issues. (See e.g. comments on PR Indirect access for System::Clock functions #10056)

Proposed Solution

Make the counts type-safe and provide safe utility functions for the common operations (e.g. ‘is t₁ within δ ms of t₀?’)

The text was updated successfully, but these errors were encountered:

kpschoedel · 2021-10-01T13:41:42Z

After a bit of experimentation I think we can use std::chrono::milliseconds and microseconds as the representation types. There is a lot of expensive stuff in <chrono> that we wouldn't want to use, but duration readily optimizes down to the underlying integer.

#### Problem Issue project-chip#10062 Some operations on System::Clock types are not safe #### Change overview What's in this PR #### Testing How was this tested? (at least one bullet point required) * If unit tests were added, how do they cover this issue? * If unit tests existed, how were they fixed/modified to prevent this in future? * If new unit tests are not added, why not? * If integration tests were added, how do they verify this change? * If new integration tests are not added, why not? * If manually tested, what platforms controller and device platforms were manually tested, and how? * If no testing is required, why not?

#### Problem Issue project-chip#10062 Some operations on System::Clock types are not safe #### Change overview Introduce safe types based on `std::chrono::duration` to `System::Clock`. #### Testing Updated `src/system/tests/TestSystemClock.cpp`

#### Problem Issue project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Introduce safe types based on `std::chrono::duration` to `System::Clock`. Future changes will use these types, starting with system timers and proceeding upward through the stack. #### Testing Updated `src/system/tests/TestSystemClock.cpp`

#### Problem Timer functions took plain integer arguments and relied on callers to get the units correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change `System::Layer` and `PlatformManager` times to take `System::Clock::Timeout` instead of plain integers. #### Testing CI; no change to functionality intended.

* Type-safe System::Clock #### Problem Issue #10062 _Some operations on System::Clock types are not safe_ #### Change overview Introduce safe types based on `std::chrono::duration` to `System::Clock`. Future changes will use these types, starting with system timers and proceeding upward through the stack. #### Testing Updated `src/system/tests/TestSystemClock.cpp` * review

#### Problem Timer functions take plain integer arguments and rely on callers to get the units correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change `System::Layer` and `PlatformManager` timers to take `System::Clock::Timeout` instead of plain integers. This change primarily converts arguments at call sites by wrapping with a `Clock::Milliseconds32()` constructor or similar operation. Future changes will convert types further up the call stack. #### Testing CI; no change to functionality intended.

#### Problem Timer functions take plain integer arguments and rely on callers to get the units correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Change `System::Layer` and `PlatformManager` timers to take `System::Clock::Timeout` instead of plain integers. This change primarily converts arguments at call sites by wrapping with a `Clock::Milliseconds32()` constructor or similar operation. Future changes will convert types further up the call stack. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/platform`, and immediate callers, to use the safer `System::Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/protocols/bdx` to use the safer `Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change `ExchangeContext::Timeout` and affected code. #### Testing CI; no change to functionality intended.

#### Problem `Event::Timestamp` uses a plain integer for `Type::kSystem`. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Use `System::Clock::Timestamp` instead #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/platform`, and immediate callers, to use the safer `System::Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/protocols/bdx` to use the safer `Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/platform`, and immediate callers, to use the safer `System::Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/protocols/bdx` to use the safer `Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Change `ExchangeContext::Timeout` and affected code. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Change code under `src/platform`, and immediate callers, to use the safer `System::Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `examples/chip-tool` to use the safer `Clock` types. #### Testing CI; no change to functionality intended.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/transport` and `src/messaging` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `src/messaging/tests/echo/echo_requester.cpp` and several `src/transport/raw/tests/`.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/transport` and `src/messaging` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `src/messaging/tests/echo/echo_requester.cpp` and several `src/transport/raw/tests/`.

#### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/lib/dnssd` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `TestActiveResolveAttempts.cpp`.

* Use safer System::Clock types in dnssd #### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of #10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/lib/dnssd` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `TestActiveResolveAttempts.cpp`. * restyle

…p#10913) #### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/transport` and `src/messaging` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `src/messaging/tests/echo/echo_requester.cpp` and several `src/transport/raw/tests/`.

* Use safer System::Clock types in dnssd #### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/lib/dnssd` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `TestActiveResolveAttempts.cpp`. * restyle

#### Problem Previous PRs added strong types to `System::Clock` and converted many uses, but a few remain. #### Change overview Convert the remaining uses of the weak types (`MonotonicMilliseconds` and `MonotonicMicroseconds`), and remove them from `System::Clock`. Fixes project-chip#10062 Some operations on System::Clock types are not safe There are some remaining uses of `Clock`-derived values as plain integers; these can be found for later cleanup by the use of `.count()`. #### Testing CI; no changes to functionality. Converted `TestPlatformTime`.

* WIP * Replace and remove weakly-typed System::Clock code #### Problem Previous PRs added strong types to `System::Clock` and converted many uses, but a few remain. #### Change overview Convert the remaining uses of the weak types (`MonotonicMilliseconds` and `MonotonicMicroseconds`), and remove them from `System::Clock`. Fixes #10062 Some operations on System::Clock types are not safe There are some remaining uses of `Clock`-derived values as plain integers; these can be found for later cleanup by the use of `.count()`. #### Testing CI; no changes to functionality. Converted `TestPlatformTime`. * review * initialization consistency * restyle

#### Problem `System::TimeSource` still uses raw integer times rather than the safer `Syste::Clock` types based on `std::chrono::duration`. Followup to project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `TimeSource` and its uses. #### Testing CI; no changes to functionality. Converted `TestTimeSource`.

#### Problem `System::TimeSource` still uses raw integer times rather than the safer `Syste::Clock` types based on `std::chrono::duration`. Followup to #10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `TimeSource` and its uses. #### Testing CI; no changes to functionality. Converted `TestTimeSource`.

* Use safer System::Clock types in dnssd #### Problem Code uses plain integers to represent time values and relies on users to get the unit scale correct. Part of project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `src/lib/dnssd` to use the safer `Clock` types. #### Testing CI; no change to functionality intended. Conversion includes `TestActiveResolveAttempts.cpp`. * restyle

* WIP * Replace and remove weakly-typed System::Clock code #### Problem Previous PRs added strong types to `System::Clock` and converted many uses, but a few remain. #### Change overview Convert the remaining uses of the weak types (`MonotonicMilliseconds` and `MonotonicMicroseconds`), and remove them from `System::Clock`. Fixes project-chip#10062 Some operations on System::Clock types are not safe There are some remaining uses of `Clock`-derived values as plain integers; these can be found for later cleanup by the use of `.count()`. #### Testing CI; no changes to functionality. Converted `TestPlatformTime`. * review * initialization consistency * restyle

#### Problem `System::TimeSource` still uses raw integer times rather than the safer `Syste::Clock` types based on `std::chrono::duration`. Followup to project-chip#10062 _Some operations on System::Clock types are not safe_ #### Change overview Convert `TimeSource` and its uses. #### Testing CI; no changes to functionality. Converted `TestTimeSource`.

kpschoedel mentioned this issue Sep 29, 2021

Indirect access for System::Clock functions #10056

Merged

kpschoedel self-assigned this Oct 1, 2021

kpschoedel mentioned this issue Oct 15, 2021

Type-safe System::Clock #10589

Merged

kpschoedel mentioned this issue Oct 21, 2021

Use safe System::Clock types for system timers #10779

Merged

kpschoedel mentioned this issue Oct 22, 2021

Use System::Clock::Timeout for ExchangeContext::Timeout #10846

Merged

kpschoedel mentioned this issue Oct 25, 2021

Use safer System::Clock types in transport and messaging #10913

Merged

kpschoedel mentioned this issue Oct 27, 2021

Use safer System::Clock types in dnssd #11062

Merged

kpschoedel mentioned this issue Oct 29, 2021

Replace and remove weakly-typed System::Clock code #11238

Merged

andy31415 closed this as completed in #11238 Nov 2, 2021

kpschoedel mentioned this issue Nov 4, 2021

Convert TimeSource to safer Clock types #11449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some operations on System::Clock types are not safe #10062

Some operations on System::Clock types are not safe #10062

kpschoedel commented Sep 29, 2021

kpschoedel commented Oct 1, 2021

Some operations on System::Clock types are not safe #10062

Some operations on System::Clock types are not safe #10062

Comments

kpschoedel commented Sep 29, 2021

Problem

Proposed Solution

kpschoedel commented Oct 1, 2021