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

atty potential unaligned read #1457

Closed
niluxv opened this issue Nov 2, 2022 · 3 comments
Closed

atty potential unaligned read #1457

niluxv opened this issue Nov 2, 2022 · 3 comments

Comments

@niluxv
Copy link
Contributor

niluxv commented Nov 2, 2022

I found some discussions on a potential unaligned read in atty (10,564,450 recent downloads; 3 years since the last release). I think it legitimately looks like a soundness bug (though I guess it won't cause issues with the usual global allocators?). The issue was filed more than a year ago.

If I understand correctly, FILE_NAME_INFO boils down to a struct like

#[repr(C)]
struct FILE_NAME_INFO {
    FileNameLength: u32,
    FileName: [u16; 1],
}

which has align 4 (playground). The vector name_info_bytes in https://github.com/softprops/atty/blob/7b5df17888997d57c2c1c8f91da1db5691f49953/src/lib.rs#L131-L141 need not be 4 byte aligned (though with any sane allocator it will be?).

Links:

@pinkforest
Copy link
Contributor

Hey thanks :) Would you mind filing a PR with informational = "unsound" ?

To me it also looks like it's not maintained so we can go with that too 👍

@pinkforest
Copy link
Contributor

People also at Wg-unsafe-guidelines agree that this indeed is unsound
https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/.60.23.5Bglobal_allocator.5D.60.20academic.20dilemma/near/311530432

Thanks to @thomcc who had picked up when this was being ported to std and pointed to:
rust-lang/rust#98033 (comment)

Also thanks to @digama0 to also confirm this

@pinkforest
Copy link
Contributor

Resolved via #1462

bors bot added a commit to cross-rs/cross that referenced this issue Nov 22, 2022
1151: Replace atty with is-terminal. r=Emilgardis a=Alexhuszagh

Although this doesn't affect cross (since we do not use custom allocators), this address [rustsec-1457](rustsec/advisory-db#1457).

Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
etehtsea added a commit to etehtsea/spin that referenced this issue Dec 11, 2022
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

No branches or pull requests

2 participants