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

iOS: open file on aarch64 breaks permissions #22862

Merged
merged 1 commit into from Mar 7, 2015

Conversation

vhbit
Copy link
Contributor

@vhbit vhbit commented Feb 27, 2015

According to Apple's arm64 calling convention varargs always are passed
through stack. Since open is actually a vararg function on Darwin,
it means that older declaration caused permissions to be taken from
stack, while passed through register => it set file permissions
to garbage and it was simply impossible to read/delete files after they
were created.

They way this commit handles it is to preserve compatibility with
existing code - it simply creates a shim unsafe function so all existing
callers continue work as nothing happened.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@vhbit
Copy link
Contributor Author

vhbit commented Feb 27, 2015

As it's actually *BSD open, cc'ing to @semarie (OpenBSD), @dhuseby (Bitrig) and @mneumann (DragonFly) - it's for your consideration whatever to join it or ignore if you're run only on a "safe" architectures.

@dhuseby
Copy link

dhuseby commented Feb 27, 2015

Is this only for AArch64? I haven't added anything to make Rust on Bitrig AArch64 work yet.

@vhbit
Copy link
Contributor Author

vhbit commented Feb 27, 2015

It's actually depends on how varargs are handled in calling convention of the system on a specific architecture. In case of iOS it bite only on aarch64, but your mileage may vary.

@semarie
Copy link
Contributor

semarie commented Feb 27, 2015

@vhbit first, thanks to share a potential problem !

Do you have a small test case we could try in order to help to check if we have the same problem ? From your description, it seems a open call with permissions set, would result to be as if permissions are 0 ?

@vhbit
Copy link
Contributor Author

vhbit commented Feb 27, 2015

@semarie

would result to be as if permissions are 0?

In my case - it actually will be any value.

Here are tests I've used (I had to call them from C code, x was used to have a value on stack, and written to file just to avoid removal as a result of optimization):

use libc;
use std::mem;

#[no_mangle]
pub extern fn rw_file_raw(path: *const libc::c_char) -> libc::c_int {
    extern {
        fn open(path: *const libc::c_char, flags: libc::c_int, mode: libc::mode_t) -> libc::c_int;
    }

    unsafe {
        let x = [0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff];
        let fd = open(path,
                        libc::O_RDWR | libc::O_TRUNC | libc::O_CREAT,
                        libc::S_IRUSR | libc::S_IWUSR);

        if fd < 0 {
            return fd;
        }

        let txt = "HelloWorld";
        libc::write(fd, mem::transmute(txt.as_ptr()), txt.len() as libc::size_t);
        libc::write(fd, mem::transmute(x.as_ptr()), x.len() as libc::size_t);
        libc::close(fd);
    }
    return 0;
}


#[no_mangle]
pub extern fn rw_file_raw_vararg(path: *const libc::c_char) -> libc::c_int {
    extern {
        fn open(path: *const libc::c_char, flags: libc::c_int, ...) -> libc::c_int;
    }

    unsafe {
        let x = [0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff];
        let fd = open(path,
                      libc::O_RDWR | libc::O_TRUNC | libc::O_CREAT,
                      (libc::S_IRUSR | libc::S_IWUSR) as libc::c_uint);

        if fd < 0 {
            return fd;
        }

        let txt = "HelloWorld";
        libc::write(fd, mem::transmute(txt.as_ptr()), txt.len() as libc::size_t);
        libc::write(fd, mem::transmute(x.as_ptr()), x.len() as libc::size_t);
        libc::close(fd);
    }
    return 0;
}

@alexcrichton
Copy link
Member

I might actually prefer to lose the shims entirely and just expose the raw open function on the relevant platforms. We may have to add some casts here and there in our usage, but it seems like the "most correct" thing to do?

@semarie
Copy link
Contributor

semarie commented Feb 27, 2015

In my case - it actually will be any value.

if it take the first value from stack, yes it should look like random...

I have tested under openbsd, and the two functions produce files with 0700 permissions. So it should be ok for openbsd (and bitrig too, I think).

@vhbit thanks again.

@vhbit
Copy link
Contributor Author

vhbit commented Feb 28, 2015

@alexcrichton,

just expose the raw open function on the relevant platforms. We may have to add some casts here and there in our usage

Unfortunately just adding some casts shouldn't work for all platforms - vararg is promoted to c_int/c_uint, while mode_t is usually u32 or u16. For example, if we compile for arm linux (mode_t = u16, c_uint = i32), we can't use open("foo", opts, mode as c_uint).

What I don't like in exposing raw open function is that there are 4 places now:

  • declaration in libc
  • librustdoc/flock.rs
  • libstd/sys/unix/fs.rs (this one will fade away soon)
  • libstd/sys/unix/std/fs2.rs

Removing shims from libc means that I have to duplicate #[cfg()] branches on all usage sites which is kind of weird.

What I suggest in this case - leave a shim to be in libc, but instead of silent override use explicit name like rust_open or rust_open_shim with documentation why it might be useful. In this case both direct platform specific call can be used (although I can't see when it'll be better than using shim) and "shimmed" call can be used without messing and tracking all corresponding #[cfg()] in libc.

@alexcrichton
Copy link
Member

Couldn't all usage sites look like open(ptr, opts, mode as mode_t)? I may be missing something, but why do usage sites require a #[cfg]?

@vhbit
Copy link
Contributor Author

vhbit commented Mar 1, 2015 via email

@alexcrichton
Copy link
Member

Ah right good point, I missed that part. In that case I think that this is fine by me, but I would request that these functions defined in Rust are also marked with extern to ensure that the ABI remains the same (in case you take the function by value). Other than that this looks great to me, thanks @vhbit!

@vhbit
Copy link
Contributor Author

vhbit commented Mar 4, 2015

@alexcrichton marked as extern and force pushed

@alexcrichton
Copy link
Member

@bors: r+ 349b1ca

Thanks!

@bors
Copy link
Contributor

bors commented Mar 5, 2015

⌛ Testing commit 349b1ca with merge 74ac507...

@bors
Copy link
Contributor

bors commented Mar 5, 2015

💔 Test failed - auto-linux-64-nopt-t

According to Apple arm64 calling convention varargs always are passed
through stack. Since `open` is actually a vararg function on Darwin's,
it means that older declaration caused permissions to be taken from
stack, while passed through register => it set file permissions
to garbage and it was simply impossible to read/delete files after they
were created.

They way this commit handles it is to preserve compatibility with
existing code - it simply creates a shim unsafe function so all existing
callers continue work as nothing happened.
@vhbit
Copy link
Contributor Author

vhbit commented Mar 5, 2015

Fixed unused import on Linux.

@alexcrichton
Copy link
Member

@bors: r+ 3f4181a

@bors
Copy link
Contributor

bors commented Mar 6, 2015

⌛ Testing commit 3f4181a with merge f7469df...

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 6, 2015
 According to Apple's [arm64 calling convention](https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW1) varargs always are passed
through stack. Since `open` is actually a vararg function on Darwin,
it means that older declaration caused permissions to be taken from
stack, while passed through register => it set file permissions
to garbage and it was simply impossible to read/delete files after they
were created.

They way this commit handles it is to preserve compatibility with
existing code - it simply creates a shim unsafe function so all existing
callers continue work as nothing happened.
@bors
Copy link
Contributor

bors commented Mar 6, 2015

💔 Test failed - auto-linux-64-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors bors merged commit 3f4181a into rust-lang:master Mar 7, 2015
@vhbit vhbit deleted the broken-open branch April 3, 2015 08:25
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.

None yet

6 participants