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

AtomicUsize has an unsound Sync impl #534

Closed
djkoloski opened this issue Nov 16, 2022 · 5 comments · Fixed by #544
Closed

AtomicUsize has an unsound Sync impl #534

djkoloski opened this issue Nov 16, 2022 · 5 comments · Fixed by #544

Comments

@djkoloski
Copy link

The comment on the Sync impl for AtomicUsize is:

// Any platform without atomics is unlikely to have multiple cores, so
// writing via Cell will not be a race condition.

While this may be true in practice, it's still technically incorrect and could open the door for bad codegen. Is there some background on why this impl is required even for platforms that don't support atomics?

@Thomasdezeeuw
Copy link
Collaborator

I believe this was added to support embedded system which don't support atomics (or threads). I don't even think it's possible to support threads without some of atomics, but I could be wrong here.

@notgull
Copy link

notgull commented Nov 17, 2022

While embedded systems may not have threads, many of them still have interrupts. It is possible to start writing to an AtomicUsize, then have an interrupt, and then read that value inside of the interrupt handler, which leads to undefined behavior. The usual solution is to disable interrupts while you are writing to the AtomicUsize.

@Thomasdezeeuw
Copy link
Collaborator

This is documented in set_logger_racy.

@notgull
Copy link

notgull commented Nov 17, 2022

I see, I missed that. In that case it should be sound since it is only used when safety can be enforced.

@djkoloski
Copy link
Author

djkoloski commented Nov 17, 2022

Since MAX_LOG_LEVEL_FILTER is also an AtomicUsize, doesn't that mean that set_max_level should be set up the same way? i.e. set_max_level should be behind a #[cfg(atomic_cas)] and an unsafe set_max_level_racy should be available everywhere.

The potential for UB here is a similar setup where you start writing with set_max_level, have an interrupt, then read the value with max_level which transmutes the memory and may produce an invalid LevelFilter.

EFanZh pushed a commit to EFanZh/log that referenced this issue Jul 23, 2023
Bumps [clap](https://github.com/clap-rs/clap) from 4.0.24 to 4.0.25.
- [Release notes](https://github.com/clap-rs/clap/releases)
- [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md)
- [Commits](clap-rs/clap@v4.0.24...v4.0.25)

---
updated-dependencies:
- dependency-name: clap
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants