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

path_symlink panics when old_path is relative #3233

Closed
Yesterday17 opened this issue Oct 15, 2022 · 2 comments
Closed

path_symlink panics when old_path is relative #3233

Yesterday17 opened this issue Oct 15, 2022 · 2 comments
Assignees
Labels
bug Something isn't working priority-medium Medium priority issue
Milestone

Comments

@Yesterday17
Copy link

Yesterday17 commented Oct 15, 2022

Assume we're calling path_symlink("testing", 1234, "/test-target").

let (source_inode, _) =
wasi_try!(state
.fs
.get_parent_inode_at_path(inodes.deref_mut(), fd, old_path_path, true));

source_inode is got from get_parent_inode_at_path:

pub(crate) fn get_parent_inode_at_path(
&self,
inodes: &mut WasiInodes,
base: WasiFd,
path: &Path,
follow_symlinks: bool,
) -> Result<(Inode, String), Errno> {
let mut parent_dir = std::path::PathBuf::new();
let mut components = path.components().rev();
let new_entity_name = components
.next()
.ok_or(Errno::Inval)?
.as_os_str()
.to_string_lossy()
.to_string();
for comp in components.rev() {
parent_dir.push(comp);
}
self.get_inode_at_path(inodes, base, &parent_dir.to_string_lossy(), follow_symlinks)
.map(|v| (v, new_entity_name))
}

Here path is testing, so we got components = Components([Normal("testing")]), So:

  • new_entity_name = testing
  • parent_dir = ""

Since parent_dir is empty, get_inode_at_path returns base directly.

let depth = wasi_try!(state
.fs
.path_depth_from_fd(inodes.deref(), fd, source_inode))
- 1;

So source_inode and fd here are actually referring the same thing. path_depth_from_fd return 0. The minus opration causes overflow.

let mut source_path = std::path::Path::new(&old_path_str);
let mut relative_path = std::path::PathBuf::new();
for _ in 0..depth {
relative_path.push("..");
}

The overflow would not be panic in release build, but results in another panic where the push operation exceeds capacity of relative_path with depth = 2**32 - 1.

Also, I found the use of Path and str very confusing. Maybe it needs to be unified?

@Yesterday17 Yesterday17 changed the title path_symlink panics when old_path is relative and does not contain ./ prefix path_symlink panics when old_path is relative Oct 15, 2022
@ptitSeb ptitSeb self-assigned this Oct 15, 2022
@ptitSeb ptitSeb added the priority-medium Medium priority issue label Oct 17, 2022
@ptitSeb ptitSeb added this to the v3.0 milestone Oct 17, 2022
@syrusakbary
Copy link
Member

Note: this issue seems related to #3228

1 similar comment
@syrusakbary
Copy link
Member

Note: this issue seems related to #3228

@syrusakbary syrusakbary added the bug Something isn't working label Oct 18, 2022
ptitSeb added a commit that referenced this issue Oct 18, 2022
ptitSeb added a commit that referenced this issue Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

3 participants