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

Use fstatat to check long path sizes in fcntl::readlinkat #1433

Merged
merged 1 commit into from May 23, 2021
Merged

Use fstatat to check long path sizes in fcntl::readlinkat #1433

merged 1 commit into from May 23, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 11, 2021

Without this, the lstat call ignores the dirfd and checks the wrong file. I wanted to write a unit test for this but I'm not sure how to do it reliably, AFAIK the maximum path size can vary between filesystems.

@asomers
Copy link
Member

asomers commented May 16, 2021

Can you explain in a little more detail what the problem is? Or better yet, show that unit test you came up with, even if it doesn't work everywhere.

@ghost
Copy link
Author

ghost commented May 22, 2021

The readlink and readlinkat syscalls read out a symlink to a fixed size buffer. If the buffer size is too small, then the syscall has to be performed again with a larger buffer. As a first step to mitigate this, inner_readlink will try to call lstat to read the size of the symlink from the filesystem. If the lstat call fails then it will fall back to enlarging the buffer in a loop and retrying until it succeeds.

This will work fine if we called it as fcntl::readlink, but the problem comes in if this is a call to fcntl::readlinkat. The readlinkat syscall expects a dirfd and a relative path. If we only call lstat then the dirfd is ignored and lstat will be trying to access the wrong file. On second thought, it's not really feasible to write a unit test for this since this can never cause the whole function to fail, it just causes it to be slower in some circumstances, since it will always enter the loop if the lstat call fails. The solution to this is: When we have a dirfd, use the corresponding stat-family syscall that takes a dirfd, which is fstatat.

There is another special case on Linux, where readlinkat will read the symlink straight from the fd if it's passed an empty string as a path. fstatat has similar behavior, but only if it's passed an AT_EMPTY_PATH flag, so we have to reconcile that here too.

Reference:

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 looks good. I just need to figure out why CI is hanging.

@asomers
Copy link
Member

asomers commented May 23, 2021

CI is hanging due to a regression in rustup. rust-lang/rustup#2774

@asomers
Copy link
Member

asomers commented May 23, 2021

If you rebase on main, CI should be fixed.

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.

bors r+

@bors bors bot merged commit dc07f61 into nix-rust:master May 23, 2021
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.

1 participant