fix readlink/readlinkat to return too long only when it is long

[sendilkumarn]

Currently readlink returns ENAMETOOLONG when the bufferSize is equal to the result size.

Consider the below case

int main(int argc, const char * argv[]) {
    
    std::string filename = "/tmp/test-dir/target";
    
    size_t bufferSize = 9;
    char* buffer = new char[bufferSize];
    size_t rc = readlink (filename.c_str(), buffer, bufferSize); 
   // Since `readlink's` output depends on the `bufferSize`. If the`bufferSize` is 9 or greater than 
   // 9 then it will return 9 or else it will return `bufferSize` as the value of `rc`. 

    return 0;
}

[asomers]

I'm afraid I don't understand the problem. Could you please be more clear? Give a complete example demonstrating the wrong behavior, and what you think the correct behavior should be.

[sendilkumarn]

I'm afraid I don't understand the problem.

Oops sorry! Let me try to clarify.

Consider the following folder structure

/tmp 
  |____ target
  |____ test-dir
       |____ target (symlinked folder)

With nix when we try to readLink this scenario.

let src = "/tmp/target"; 
let dest = "tmp/target/test-dir";

It works correctly when we pass in a buffer that is bigger than the src length

let mut buf = vec![0; src.len() + 1]; 
println!("{}", str::from_utf8(&buf).unwrap()); // prints "/tmp/target"

Actual: But when we pass in a buffer that is smaller than the src length, it returns ENAMETOOLONG exception.

let mut buf = vec![0; src.len()];  // returns `ENAMETOOLONG` 

Expected: Instead it should truncate the contents (to a length of buffer size), in case the buffer is too small to hold all of the contents. - from here

[asomers]

Truncating the result is what readlink(2) does. But Nix is supposed to be safe and easy to use; we don't want to truncate any results. Perhaps ENAMETOOLONG isn't the best error to return, but a truncated result would be even worse.

[sendilkumarn]

but a truncated result would be even worse.

That makes sense. I agree but this means we should always send in a buffer that is greater than the required size or if not it fails.

We should at least allow the buffer to have the same size as that of the required result. What do you think?

Perhaps ENAMETOOLONG isn't the best error to return

Can we return EINVAL instead of ENAMETOOLONG?

[asomers]

but a truncated result would be even worse.

That makes sense. I agree but this means we should always send in a buffer that is greater than the required size or if not it fails.

We should at least allow the buffer to have the same size as that of the required result. What do you think?

Perhaps ENAMETOOLONG isn't the best error to return

Can we return EINVAL instead of ENAMETOOLONG?

No, that would have the same problem. The problem with both of those error codes is that they overload errors used by the OS. Better to use our own error type. Better yet would be to never return an error. How about changing that function to allocate its storage internally and return an OsString, a little like this?

fn readlink<P: ...>(path: P) -> Result<OsString> {
    let v = Vec::with_capacity(libc::PATH_MAX);
    let r = libc::readlink(..., v.as_mut_ptr() as *mut c_char, v.capacity());
    if let Ok(len) = Errno::result(r) {
        unsafe { v.set_len(len) };
        OsString::from_vec(v)
    } else {
        /* error handling */
    }
}

[sendilkumarn]

@asomers That is a good idea. I updated the code to reflect your suggested changes.

[asomers]

The first letter of OsString should be capitalized. Also, since this change is backwards incompatible it would be courteous to our users to explain how to convert their existing code.

[asomers]

No need to zero initialize. You can use an uninitialized vec instead.

[asomers]

Trailing space here.

[asomers]

This example isn't going to mean much to a reader. I'm thinking something in English, like "the buffer argument of readlink and readlinkat has been removed, and the return value is now an owned type. Existing code can be updated by removing the buffer argument and removing any clone or similar operation on the output."

[asomers]

You seem to be unaware of the capacity method. You should write it like this:

let mut v = Vec::with_capacity(libc::PATH_MAX as usize);
let res = path.with_nix_path(|cstr| {
    unsafe { libc::readlink(cstr.as_ptr(), v.as_mut_ptr() as *mut c_char, v.capacity() as size_t) }

[asomers]

This line isn't important to readers. You can suppress it with a leading #, and delete the following blank line.

[asomers]

The code would be more self-documenting like this:

let _: OsString = readlink(&path);

[asomers]

LGTM. Thanks for your submission!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push
• FreeBSD 11.2