-
Notifications
You must be signed in to change notification settings - Fork 13.3k
ICE: std::os::getcwd fails too eagerly #16946
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
Comments
It would probably be better to return a result instead of failing. |
Naturally an IoResult would be best. For now I've fixed this locally: static BUF_BYTES: uint = 4096;
#[cfg(unix)]
pub fn getcwd() -> Option<Path> {
use std::c_str::CString;
use libc::{c_char, size_t};
let mut buf = [0 as c_char, ..BUF_BYTES];
unsafe {
if libc::getcwd(buf.as_mut_ptr(), buf.len() as size_t).is_null() {
None
} else {
Some(Path::new(CString::new(buf.as_ptr(), false)))
}
}
}
#[cfg(windows)]
pub fn getcwd() -> Option<Path> {
use libc::DWORD;
use libc::GetCurrentDirectoryW;
let mut buf = [0 as u16, ..BUF_BYTES];
unsafe {
if libc::GetCurrentDirectoryW(buf.len() as DWORD, buf.as_mut_ptr()) == 0 {
return None;
}
}
match String::from_utf16(std::str::truncate_utf16_at_nul(buf)) {
Some(s) => Some(Path::new(s)),
_ => None,
}
} And while we're at it: BUF_BYTES is 2048 in the stdlib which is way too small. |
Apparently there is no path length limit on linux. glibc has the following extension that could be used to handle this:
Not sure about jemalloc <-> malloc here. FreeBSD has the same extension. |
@huonw Not sure why you changed the title. You cannot use getcwd in its current form in a safe way if you don't want your program to crash. Even if the user just deleted the directory your program was started from, your program will crash. If your program was started with a very long cwd path? Crash. getcwd is literally useless in its current form. No need to sugarcoat the issue. Furthermore your title is misleading since this is not the only issue. getcwd also fails if the buffer is too small. That means that the documentation is incorrect. |
The original title was unnecessarily inflammatory and nonspecific; I changed it to accurately reflect the problem: it
That is included in "failing too eagerly": it doesn't handle this 'error' properly.
Your title didn't address this either. |
cc @aturon |
Make old-fashioned functions in the `std::os` module utilize `IoResult`. I'm still investigating the possibility to include more functions in this pull request. Currently, it covers `getcwd()`, `make_absolute()`, and `change_dir()`. The issues covered by this PR are #16946 and #16315. A few concerns: - Should we provide `OsError` in distinction from `IoError`? I'm saying this because in Python, those two are distinguished. One advantage that we keep using `IoError` is that we can make the error cascade down other functions whose return type also includes `IoError`. An example of such functions is `std::io::TempDir::new_in()`, which uses `os::make_absolute()` as well as returns `IoResult<TempDir>`. - `os::getcwd()` uses an internal buffer whose size is 2048 bytes, which is passed to `getcwd(3)`. There is no upper limitation of file paths in the POSIX standard, but typically it is set to 4096 bytes such as in Linux. Should we increase the buffer size? One thing that makes me nervous is that the size of 2048 bytes already seems a bit excessive, thinking that in normal cases, there would be no filenames that even exceeds 512 bytes. Fixes #16946. Fixes #16315. Any ideas are welcomed. Thanks!
This means that getcwd is unusable outside of toy programs. There is no way to see if this function might fail before calling it or to get the path in a safe way.
rustc uses getcwd and will ICE under the conditions described in this issue.
The text was updated successfully, but these errors were encountered: