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.

Sign up for GitHub

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

Jump to bottom

Prevent future potential UB in unix wrapper for getpwuid() #104

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Prevent future potential UB in unix wrapper for getpwuid() #104

merged 2 commits into from
Mar 9, 2024

Conversation

martinvonz
Copy link
Contributor

No description provided.

@martinvonz
getpwuid: reduce scope of _passwd variable
e926f42 
The `_passwd` variable is only used in one of the code blocks, so
let's declare it in that block.
@martinvonz martinvonz changed the title Safety fixes and in unix getpwuid Safety fixes and cleanups in unix getpwuid Mar 9, 2024
@martinvonz martinvonz force-pushed the push-lmkzyuywzywx branch 4 times, most recently from bc51e20 to 34c9b06 Compare March 9, 2024 22:15
@martinvonz
Copy link
Contributor Author

The docs tests are already failing on the v1 branch

AldaronLau
AldaronLau requested changes Mar 9, 2024
View reviewed changes
Copy link
Member

@AldaronLau AldaronLau left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR!

The cleanups make sense, although as-is I think this potentially introduces unsoundness with a potential null dereference in getpwuid_r(). I don't believe there are any existing soundness bugs with using uninitialized memory here, as the API is used with passing a pointer to uninitialized memory in the man page example (although for the related function getpwnam_r()).

I'll work on getting the doc tests passing.

src/os/unix.rs Outdated Show resolved Hide resolved
src/os/unix.rs Outdated Show resolved Hide resolved
@AldaronLau
Copy link
Member

The docs tests are already failing on the v1 branch

While the warnings do exist, the tests are passing. The failure is due to a segmentation fault caused by your changes.

@martinvonz
getpwuid: don't return uninitialized data on unexpected OS
f617aee 
This commit makes it so the function doesn't compile on unexpected OSs
instead of returning uninitialized data.
@martinvonz
Copy link
Contributor Author

While the warnings do exist, the tests are passing. The failure is due to a segmentation fault caused by your changes.

Heh, funny that only the doc tests broke. I've simply dropped one of the commits.

@AldaronLau AldaronLau added this pull request to the merge queue Mar 9, 2024
@AldaronLau AldaronLau removed this pull request from the merge queue due to a manual request Mar 9, 2024
@AldaronLau AldaronLau changed the title Safety fixes and cleanups in unix getpwuid Code cleanup in unix wrapper for getpwuid() Mar 9, 2024
@AldaronLau AldaronLau changed the title Code cleanup in unix wrapper for getpwuid() Prevent future potential UB in unix wrapper for getpwuid() Mar 9, 2024
@AldaronLau AldaronLau added this pull request to the merge queue Mar 9, 2024
Merged via the queue into ardaku:v1 with commit 4bbaf52 Mar 9, 2024
67 checks passed
AldaronLau pushed a commit that referenced this pull request Mar 10, 2024
@martinvonz @AldaronLau
Prevent future potential UB in unix wrapper for getpwuid() (#104)
9e7a5c8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AldaronLau AldaronLau AldaronLau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martinvonz @AldaronLau