Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential integer overflow in the usleep implementation of ESP IDF (IDFGH-13493) #14390

Closed
3 tasks done
ivmarkov opened this issue Aug 18, 2024 · 1 comment
Closed
3 tasks done
Assignees
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Type: Bug bugs in IDF

Comments

@ivmarkov
Copy link
Contributor

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

master and any earlier

Espressif SoC revision.

all

Operating System used.

Linux

How did you build your project?

Command line with Make

If you are using Windows, please specify command line type.

None

Development Kit.

all

Power Supply used.

USB

What is the expected behavior?

Passing 0xFFFFFFFF to the current ESP IDF usleep implementation should make the system sleep for 0xFFFFFFFF microseconds, OR return EINVAL to indicate that values larger than 1_000_000 are not supported (as per the usleep syscall spec).

What is the actual behavior?

Passing 0xFFFFFFFF to the current ESP IDF usleep implementation will overflow and sleep for a smaller amount of time.

Steps to reproduce.

  1. Generate an ESP-IDF template with cargo generate https://github.com/esp-rs/esp-idf-template and accept all defaults
  2. Replace the main.rs of the generated template with
use std::time::Instant;

use esp_idf_svc::sys::usleep;

fn main() {
    // It is necessary to call this function once. Otherwise some patches to the runtime
    // implemented by esp-idf-sys might not link properly. See https://github.com/esp-rs/esp-idf-template/issues/71
    esp_idf_svc::sys::link_patches();

    // Bind the log crate to the ESP Logging facilities
    esp_idf_svc::log::EspLogger::initialize_default();

    for sleep_us in [
        1_000_000, // Works
        0xffff_ffffu32, // Will not work due to a silent integer overflow in `usleep`
        0xffff_ffffu32 - 10 /*ms - the default tick-rate*/ * 1000 - 1 // Works as we avoid the integer overflow
    ] {
        log::info!("About to sleep for ~{sleep_us}us");

        let start_sleep = Instant::now();

        let error_code = unsafe {
            usleep(sleep_us)
        };

        log::info!("Actual sleep: ~{}us, error_code: {error_code}", start_sleep.elapsed().as_micros());
    }
}

... or translate to C.

Build, flash and run.

Debug Logs.

No response

More Information.

Related to ivmarkov/rust@1faccba#diff-2b173511d53f2c01a70eac2f753b998948ca6cbe3d013b6119fbe4631770ec60R279 which fixes rust-lang/rust#129212 (comment)

@ivmarkov ivmarkov added the Type: Bug bugs in IDF label Aug 18, 2024
@github-actions github-actions bot changed the title Potential integer overflow in the usleep implementation of ESP IDF Potential integer overflow in the usleep implementation of ESP IDF (IDFGH-13493) Aug 18, 2024
@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 18, 2024
@ESP-Marius
Copy link
Collaborator

Thanks for pointing out this one, we'll fix it!

@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed and removed Status: Opened Issue is new labels Aug 19, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 21, 2024
Fix `thread::sleep` Duration-handling for ESP-IDF

Addresses the ESP-IDF specific aspect of rust-lang#129212

#### A short summary of the problems addressed by this PR:
================================================

1. **Problem 1** - the current implementation of `std::thread::sleep` does not properly round up the passed `Duration`

As per the documentation of `std::thread::sleep`, the implementation should sleep _at least_ for the provided duration, but not less. Since the minimum supported resolution of the `usleep` syscall which is used with ESP-IDF is one microsecond, this means that we need to round-up any sub-microsecond nanos to one microsecond. Moreover, in the edge case where the user had passed a duration of < 1000 nanos (i.e. less than one microsecond), the current implementation will _not_ sleep _at all_.

This is addressed by this PR.

2. **Problem 2** - the implementation of `usleep` on the ESP-IDF can overflow if the passed number of microseconds is >= `u32::MAX - 1_000_000`

This is also addressed by this PR.

Extra details for Problem 2:

`u32::MAX - 1_000_000` is chosen to accommodate for the longest possible systick on the ESP IDF which is 1000ms.

The systick duration is selected when compiling the ESP IDF FreeRTOS task scheduler itself, so we can't know it from within `STD`. The default systick duration is 10ms, and might be lowered down to 1ms. (Making it longer I have never seen, but in theory it can go up to a 1000ms max, even if obviously a one second systick is unrealistic - but we are paranoid in the PR.)

While the overflow is reported upstream in the ESP IDF repo[^1], I still believe we should workaround it in the Rust wrappers as well, because it might take time until it is fixed, and they might not fix it for all released ESP IDF versions.

For big durations, rather than calling `usleep` repeatedly on the ESP-IDF in chunks of `u32::MAX - 1_000_000`us, it might make sense to call instead with 1_000_000us (one second) as this is the max period that seems to be agreed upon as a safe max period in the `usleep` POSIX spec. On the other hand, that might introduce less precision (as we need to call more times `usleep` in a loop) and, we would be fighting a theoretical problem only, as I have big doubts the ESP IDF will stop supporting durations higher than 1_000_000us - ever - because of backwards compatibility with code which already calls `usleep` on the ESP IDF with bigger durations.

[^1]: espressif/esp-idf#14390
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 21, 2024
Rollup merge of rust-lang#129232 - ivmarkov:master, r=workingjubilee

Fix `thread::sleep` Duration-handling for ESP-IDF

Addresses the ESP-IDF specific aspect of rust-lang#129212

#### A short summary of the problems addressed by this PR:
================================================

1. **Problem 1** - the current implementation of `std::thread::sleep` does not properly round up the passed `Duration`

As per the documentation of `std::thread::sleep`, the implementation should sleep _at least_ for the provided duration, but not less. Since the minimum supported resolution of the `usleep` syscall which is used with ESP-IDF is one microsecond, this means that we need to round-up any sub-microsecond nanos to one microsecond. Moreover, in the edge case where the user had passed a duration of < 1000 nanos (i.e. less than one microsecond), the current implementation will _not_ sleep _at all_.

This is addressed by this PR.

2. **Problem 2** - the implementation of `usleep` on the ESP-IDF can overflow if the passed number of microseconds is >= `u32::MAX - 1_000_000`

This is also addressed by this PR.

Extra details for Problem 2:

`u32::MAX - 1_000_000` is chosen to accommodate for the longest possible systick on the ESP IDF which is 1000ms.

The systick duration is selected when compiling the ESP IDF FreeRTOS task scheduler itself, so we can't know it from within `STD`. The default systick duration is 10ms, and might be lowered down to 1ms. (Making it longer I have never seen, but in theory it can go up to a 1000ms max, even if obviously a one second systick is unrealistic - but we are paranoid in the PR.)

While the overflow is reported upstream in the ESP IDF repo[^1], I still believe we should workaround it in the Rust wrappers as well, because it might take time until it is fixed, and they might not fix it for all released ESP IDF versions.

For big durations, rather than calling `usleep` repeatedly on the ESP-IDF in chunks of `u32::MAX - 1_000_000`us, it might make sense to call instead with 1_000_000us (one second) as this is the max period that seems to be agreed upon as a safe max period in the `usleep` POSIX spec. On the other hand, that might introduce less precision (as we need to call more times `usleep` in a loop) and, we would be fighting a theoretical problem only, as I have big doubts the ESP IDF will stop supporting durations higher than 1_000_000us - ever - because of backwards compatibility with code which already calls `usleep` on the ESP IDF with bigger durations.

[^1]: espressif/esp-idf#14390
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels Aug 22, 2024
espressif-bot pushed a commit that referenced this issue Aug 24, 2024
If trying to usleep for 0xFFFF FFFF us the calculation of delay ticks would overflow
resulting in the system not sleeping at all.

Closes #14390
espressif-bot pushed a commit that referenced this issue Sep 11, 2024
If trying to usleep for 0xFFFF FFFF us the calculation of delay ticks would overflow
resulting in the system not sleeping at all.

Closes #14390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Type: Bug bugs in IDF
Projects
None yet
Development

No branches or pull requests

3 participants