Add support for reading symlinks longer than `PATH_MAX` to `readlink` and `readlinkat` by SolraBizna · Pull Request #1231 · nix-rust/nix

SolraBizna · 2020-05-03T19:41:09Z

This is in response to issue #1178.

The new logic uses the following approach.

At any time, if readlink returns an error, or a value ≥ 0 and < (not ≤!) the buffer size, we're done.
Attempt to readlink into a PATH_MAX sized buffer. (This will almost always succeed, and saves a system call over calling lstat first.)
Try to lstat the link. If it succeeds and returns a sane value, allocate the buffer to be that large plus one byte. Otherwise, allocate the buffer to be PATH_MAX.max(128) << 1 bytes.
Repeatedly attempt to readlink. Any time its result is ≥ (not >!) the buffer size, double the buffer size and try again.

While testing this, I discovered that ext4 doesn't allow creation of a symlink > 4095 (Linux's PATH_MAX minus one) bytes long. This is in spite of Linux happily allowing paths in other contexts to be longer than this—including on ext4! This was probably instated to avoid breaking programs that assume PATH_MAX will always be enough, but ironically hindered my attempt to test support for not assuming. I tested the code using an artificially small PATH_MAX and (separately) a wired-to-fail lstat. strace showed the code behaving precisely as expected. Unfortunately, I can't add an automatic test for this.

Other changes made by this PR:

wrap_readlink_result now calls shrink_to_fit on the buffer before returning, potentially reclaiming kilobytes of memory per call. This could be very important if the returned buffer is long-lived.
readlink and readlink_at now both call an inner_readlink function that contains the bulk of the logic, avoiding copy-pasting of code. (This is much more important now that the logic is more than a few lines long.)

Notably, this PR does not add support for systems that don't define PATH_MAX at all. As far as I know, I don't have access to any POSIX-ish OS that doesn't have PATH_MAX, and I suspect it would have other compatibility issues with nix anyway.

asomers

I just checked, and I can't create a too-long symlink on FreeBSD, NetBSD, OpenBSD, or Solaris. Is there any operating system that does allow it?

asomers · 2020-05-03T20:59:38Z

        Ok(len) => {
            unsafe { v.set_len(len as usize) }
+            v.shrink_to_fit();
            Ok(OsString::from_vec(v.to_vec()))


Not your fault, but I notice that this line is doing a data copy. Could you eliminate the data copy by taking the v argument by value instead of by reference?

asomers · 2020-05-03T21:02:25Z

-        unsafe { libc::readlink(cstr.as_ptr(), v.as_mut_ptr() as *mut c_char, v.capacity() as size_t) }
+        unsafe {
+            match dirfd  {
+                Some(dirfd) => libc::readlinkat(dirfd, cstr.as_ptr(), v.as_mut_ptr() as *mut c_char, v.capacity() as size_t),


Wrap the long lines, please.

What is this project's wrap width? (I normally wrap to 80 columns, but this project clearly has a wider convention.)

Edit: Apparently the rest of the file is almost all wrapped to 80 columns, and only the code I was editing wasn't. Wrapped all the code I touched to 80 columns.

asomers · 2020-05-03T21:04:46Z

+    // Uh oh, the result is too long...
+    // Let's try to ask lstat how many bytes to allocate.
+    let reported_size = super::sys::stat::lstat(path).and_then(|x| Ok(x.st_size)).unwrap_or(0);
+    let mut try_size = if reported_size > 0 { reported_size as usize + 1 }


As a matter of style, I prefer

if condition { something } else { something_else }

rather than cramming everything onto two lines

asomers · 2020-05-03T21:06:37Z

+    // If lstat doesn't cooperate, or reports an error, be a little less precise.
+    else { (libc::PATH_MAX as usize).max(128) << 1 };
+    loop {
+        v.reserve_exact(try_size);


Interesting. I think this may be the first good use of reserve_exact I've seen.

asomers · 2020-05-03T21:13:27Z

+        }
+        else {
+            // Ugh! Still not big enough!
+            let next_size = try_size << 1;


I don't think we need to gracefully handle symlinks with paths greater than 32 bits. A panic would suffice. You can do

try_size = try_size.checked_mul(2).unwrap();

I think it's better to return an error in this case (as below), but if you ask me to I'll replace that logic with a checked multiply.

In fact, I think this code will still panic if the size overflows 32 bits. I think << , like most operators, will panic on overflow in debug builds (but wrap on release builds). If you really want to handle the overflow without panicing, you need to use checked_shl.

asomers · 2020-05-03T21:14:35Z

+    else { (libc::PATH_MAX as usize).max(128) << 1 };
+    loop {
+        v.reserve_exact(try_size);
+        let res = path.with_nix_path(|cstr| {


These 13 lines are duped. Do you think you could deduplicate them?

I think the wrong lines got selected. Do you mean L197-L204 and L218-L225?

asomers · 2020-05-03T21:15:47Z

+    // Uh oh, the result is too long...
+    // Let's try to ask lstat how many bytes to allocate.
+    let reported_size = super::sys::stat::lstat(path).and_then(|x| Ok(x.st_size)).unwrap_or(0);
+    let mut try_size = if reported_size > 0 { reported_size as usize + 1 }


Instead of comparing reported_size to 0, you may as well compare it to PATH_MAX. After all, lstat reports less than what we already tried, then we shouldn't trust lstat's output.

Trusting lstat after a failed readlink handles a race condition where the link size changes between the first readlink and the ~~first~~ lstat.

SolraBizna · 2020-05-03T23:14:19Z

I just checked, and I can't create a too-long symlink on FreeBSD, NetBSD, OpenBSD, or Solaris. Is there any operating system that does allow it?

On Linux, as far as I can tell, it's up to the particular filesystem. None of [ext*, XFS, btrfs] support very long symlinks, though.

SolraBizna · 2020-05-03T23:26:14Z

Thank you for the thorough review. I have implemented every requested change except the panic on absurdly long symlinks. I'll implement that too while I'm "in the code" if you like.

(I do agree that a >4GiB symlink is absolutely, without a doubt useless, but I also subscribe to the GNU convention of avoiding arbitrary limits where possible, even reasonable ones.)

asomers · 2020-05-07T03:13:04Z

-fn wrap_readlink_result(v: &mut Vec<u8>, res: ssize_t) -> Result<OsString> {
+fn wrap_readlink_result(mut v: Vec<u8>, res: ssize_t) -> Result<OsString> {
    match Errno::result(res) {
        Err(err) => Err(err),


This line is dead code. We only ever call wrap_readlink_result when res is >= 0, so you may as well eliminate the Errno::result part. I think that issued predated your changes, but since you're here would you please fix it?

asomers · 2020-05-07T03:16:17Z

+    // Uh oh, the result is too long...
+    // Let's try to ask lstat how many bytes to allocate.
+    let reported_size = super::sys::stat::lstat(path)
+        .and_then(|x| Ok(x.st_size)).unwrap_or(0);


You could simply this line into .map_or(0, |x| x.st_size)

asomers · 2020-05-07T03:21:29Z

+        }
+        else {
+            // Ugh! Still not big enough!
+            let next_size = try_size << 1;


In fact, I think this code will still panic if the size overflows 32 bits. I think << , like most operators, will panic on overflow in debug builds (but wrap on release builds). If you really want to handle the overflow without panicing, you need to use checked_shl.

SolraBizna · 2020-05-07T09:23:05Z

In fact, I think this code will still panic if the size overflows 32 bits. I think << , like most operators, will panic on overflow in debug builds (but wrap on release builds).

It seems that, barring obviously foolish cases caught by the compiler, bit shifting wraps in both debug mode and release mode. Guess Rust's designers figured that overflow in bit shifting is likely to be intentional. Regardless, I replaced that code with checked_shl just because it's clearer and more idiomatic that way.

Thanks again for the feedback. I've pushed the requested changes. If you think of any other tweaks you'd like me to make, let me know.

SolraBizna · 2020-05-07T09:25:53Z

Huh. Using map_or broke the build on FreeBSD. Is it using an old rustc or something? (map_or seems to have been added in 1.41, which was released this last January.)

asomers · 2020-05-08T00:11:50Z

Huh. Using map_or broke the build on FreeBSD. Is it using an old rustc or something? (map_or seems to have been added in 1.41, which was released this last January.)

Yes. Nix's MSRV is currently 1.36.0. We're pretty conservative with MSRV because we're consumed by platforms that are slow to update, like OpenBSD and embedded systems. Could you please remove map_or? Fie on me for suggesting it.

SolraBizna · 2020-05-08T00:59:02Z

Done, and now all the checks pass again.

asomers · 2020-05-08T01:11:22Z

Great! Now all it needs is a squash.

SolraBizna · 2020-05-11T02:31:54Z

Should I be squashing it on my end? I assumed this would be a case for the "Squash and merge" button.

asomers · 2020-05-11T03:04:18Z

Yes you should squash it. Because we use the bors merge bot, we cannot squash on our end.

… and `readlinkat`

SolraBizna · 2020-05-11T06:22:03Z

There. Hopefully I did that properly.

asomers

bors r+

bors · 2020-05-12T05:12:42Z

Build succeeded:

asomers requested changes May 3, 2020

View reviewed changes

SolraBizna added a commit to SolraBizna/nix that referenced this pull request May 3, 2020

Changes requested by @asomers on nix-rust#1231

6e00c73

asomers requested changes May 7, 2020

View reviewed changes

Add support for reading symlinks longer than PATH_MAX to readlink…

dfee424

… and `readlinkat`

SolraBizna force-pushed the improved_readlink branch from 6fdd54d to dfee424 Compare May 11, 2020 04:49

asomers approved these changes May 12, 2020

View reviewed changes

bors bot merged commit 33f4efe into nix-rust:master May 12, 2020

SUPERCILEX mentioned this pull request Nov 6, 2022

readlink uses PATH_MAX-sized buffer, but PATH_MAX can't be relied upon #1178

Closed

Conversation

SolraBizna commented May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolraBizna May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolraBizna May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolraBizna commented May 3, 2020

Uh oh!

SolraBizna commented May 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolraBizna commented May 7, 2020

Uh oh!

SolraBizna commented May 7, 2020

Uh oh!

asomers commented May 8, 2020

Uh oh!

SolraBizna commented May 8, 2020

Uh oh!

asomers commented May 8, 2020

Uh oh!

SolraBizna commented May 11, 2020

Uh oh!

asomers commented May 11, 2020

Uh oh!

SolraBizna commented May 11, 2020

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SolraBizna commented May 3, 2020 •

edited

Loading

SolraBizna May 3, 2020 •

edited

Loading

SolraBizna May 3, 2020 •

edited

Loading