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

fix: unaligned read panic on macos #2311

Conversation

anonkey
Copy link
Contributor

@anonkey anonkey commented Feb 7, 2024

What does this PR do

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch 2 times, most recently from c2ce2fd to 27a24d4 Compare February 7, 2024 14:40
@anonkey anonkey closed this Feb 7, 2024
@anonkey anonkey reopened this Feb 7, 2024
@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch 2 times, most recently from 607b0f3 to 4315c10 Compare February 7, 2024 14:53
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR only contains changes to the doc test. Where is the actual code change?

changelog/2311.fixed.md Outdated Show resolved Hide resolved
@anonkey anonkey changed the title fix: unaligned read panic on macos Draft: fix: unaligned read panic on macos Feb 7, 2024
@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch from 4315c10 to 5f85f02 Compare February 7, 2024 15:39
@anonkey
Copy link
Contributor Author

anonkey commented Feb 7, 2024

This PR only contains changes to the doc test. Where is the actual code change?

Wanted to check if misalignment was triggered in CI but wasn't the case so I factorize BSD and Apple test

The separate test case was relevant only locally on my computer

@anonkey anonkey changed the title Draft: fix: unaligned read panic on macos fix: unaligned read panic on macos Feb 7, 2024
@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch from 5f85f02 to 77e1305 Compare February 7, 2024 16:31
src/unistd.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member

Wanted to check if misalignment was triggered in CI but wasn't the case so I factorize BSD and Apple test

Did you successfully reproduce it in CI? Currently, there is no macOS aarch64 CI in Nix, but since GitHub Action has introduced this, we can enable it if needed.

I am not sure if macOS x86_64 would allow that misaligned read so...

src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
@anonkey
Copy link
Contributor Author

anonkey commented Feb 13, 2024

Wanted to check if misalignment was triggered in CI but wasn't the case so I factorize BSD and Apple test

Did you successfully reproduce it in CI? Currently, there is no macOS aarch64 CI in Nix, but since GitHub Action has introduced this, we can enable it if needed.

I am not sure if macOS x86_64 would allow that misaligned read so...

No i didn't success, maybe because of that, but maybe because memory alignment depends of the OS and lot other random variables, locally it only happens on some groups and i'm not sure if it will be same groups in CI.
I added a apple aarch64 test to check it
I'll disable the fix to check if error happens in aarch64-darwin CI when the runner would be enabled

@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch 3 times, most recently from 0516a69 to 246948f Compare February 14, 2024 01:08
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
test/test_unistd.rs Outdated Show resolved Hide resolved
@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch 4 times, most recently from 1faf396 to d3543e7 Compare February 15, 2024 19:46
@anonkey anonkey force-pushed the fix-misaligned-read-crash-on-group-members-macos branch from d3543e7 to 7141269 Compare February 15, 2024 20:06
@anonkey
Copy link
Contributor Author

anonkey commented Feb 15, 2024

don't trigger in CI, i tried 13 groups but none trigger while lot of them crash on my Mac, maybe because of the virtualization of macos?

@SteveLauC
Copy link
Member

don't trigger in CI, i tried 13 groups but none trigger while lot of them crash on my Mac, maybe because of the virtualization of macos?

That's possible. BTW, does this patch fix the issue on your host?

@SteveLauC SteveLauC linked an issue Feb 16, 2024 that may be closed by this pull request
@anonkey
Copy link
Contributor Author

anonkey commented Feb 16, 2024

don't trigger in CI, i tried 13 groups but none trigger while lot of them crash on my Mac, maybe because of the virtualization of macos?

That's possible. BTW, does this patch fix the issue on your host?

Definitely, yes
In repro example and in real life product

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SteveLauC SteveLauC added this pull request to the merge queue Feb 17, 2024
Merged via the queue into nix-rust:master with commit 197f55b Feb 17, 2024
35 checks passed
@aawsome
Copy link

aawsome commented Feb 21, 2024

Can you say when you are able to release a new version including this fix? We are having users who get this error and would like to use the fix in the dependencies.
Thanks for fixing!

@SteveLauC
Copy link
Member

Can you say when you are able to release a new version including this fix? We are having users who get this error and would like to use the fix in the dependencies. Thanks for fixing!

Hi, I think I will release 0.28.0 in the near future (possibly within this week)

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 this pull request may close these issues.

Group::from_name misaligned pointer dereference
4 participants