File instance dropped before leaving scope

[RandomInsano]

This is tested on 1.26.0-nightly, but may affect other versions.

I found that if I don't let the File instance returned from OpenOptions::open() the File object is drop()ed and the handle is closed before we go out of scope.

#[macro_use]
extern crate nix;
use std::fs::OpenOptions;
use std::os::unix::io::AsRawFd;

const I2C_SLAVE: u16 = 0x0703;
 
ioctl_write_int_bad!(set_i2c_slave_address, I2C_SLAVE);
 
fn main() {
    println!("Let's try some i2c");
    let file_result = OpenOptions::new()
        .read(true)
        .write(true)
        .open("/dev/i2c-0");
 
    assert!(file_result.is_ok());
 
    let fd = file_result.unwrap().as_raw_fd();
    println!("File descriptor: {}", fd);
 
    unsafe {
        set_i2c_slave_address(fd, 0x34).unwrap();
    }
}

Here's the strace for the above:

open("/dev/i2c-0", O_RDWR|O_LARGEFILE|O_CLOEXEC) = 3
ioctl(3, FIOCLEX)                       = 0
close(3)                                = 0
write(1, "File descriptor: 3\n", 19File descriptor: 3
)    = 19
ioctl(3, 0x703, 0x34)                   = -1 EBADF (Bad file descriptor)

Should objects be dropped before the scope ends if they aren't used? While it makes a certain amount of sense, this was a hard paper cut for me doing some low-level work.

[posborne]

Hmmm, this one is definitely interesting. I don't see it in this code, but is there any chance that the NLL is enabled. The last time I had checked, this still required #![feature(nll)] in order to be in play. Cases like this is one of the things that concerns me about NLL (I am not at all against NLL but it seems likely to break existing code if it is just enabled).

Does this same code work without issue on stable?

[Havvy]

From the as_raw_rd documentation:

This method does not pass ownership of the raw file descriptor to the caller. The descriptor is only guaranteed to be valid while the original object has not yet been destroyed.

And you destroy the original File object in the line you get the file descriptor from:

let fd = file_result.unwrap().as_raw_fd();

The file_result.unwrap() consumes the Result<File, _> returning a File as a value expression (a.k.a rvalue). You then use that value expression as a place expression (a.k.a. lvalue) by using it at the target of a method call. This puts the value expression into a temporary that lasts as long as the let statement. The method call returns a file descriptor and then drop the temporary, destroying the original File the file descriptor expects to be alive. It's equivalent to the following code:

let fd = {
    let temp;
    {
        temp = file_result.unwrap();
        temp.as_raw_fd();
    }
}

NLL isn't allowed to touch drop order (that's the domain of dropck, not borrowck), so if NLL make the original code work as the author thought it would, that itself would be a bug.

The fix to this is add another let statement.

let file = file_result.unwrap();
let fd = file.as_raw_fd();

I wonder if there's a way for clippy to catch this specific instance. cc @Manishearth

I don't think it's a bug, but I'm not sure if I should be closing issues, so I leave it for somebody else to actually close it if they agree with my analysis.

[RandomInsano]

I’ll try stable and report back. Thanks for the explanation folks!

I don’t remember much if of NLL (Non-lexical lifetimes) so I’ll go Googling in a bit. The point of this issue is: Should “This puts the value expression into a temporary that lasts as long as the let statement.” be a thing, and are there cases where we’ll run afoul?

By and large I think no, but when you’re running through an FFI boundary with handles or other values pointing to low-level OS primitives it can get gnarly to figure out why it’s gone wrong.

To gauge impact, I spent about 8 hours over about a week with an API that returns EBADF not only for bad file handles but also bad input data (when a device address is wrong). If I hadn’t thought to try strace I would likely still be stuck.

[RandomInsano]

Looks to still be a problem all the way back with "rustc 1.24.1 (d3ae9a9 2018-02-27)". rustup is being a little unhappy at the moment and can't pull down recent additions, but I'm not sure if NLL is the case as it was an experimental feature back in January.

[posborne]

And you destroy the original File object in the line you get the file descriptor from:

@Havvy I completely missed that in my reading and quickly biased it with other thoughts I had related to how NLL changes might impact drop rules. If your statement about the NLL rules only affecting borrowck and not affecting dropck rules, then my concerns there might very well be unfounded.

I agree that this issue should be closed.

[pnkfelix]

@RandomInsano wrote:

Should “This puts the value expression into a temporary that lasts as long as the let statement.” be a thing, and are there cases where we’ll run afoul?

The semantics of how long r-value temporaries live is something that is a source of confusion at times, and has been discussed on various issues.

You can see background discussion of this, as well as proposals for revising our semantics here, at links such as:

• http://smallcultfollowing.com/babysteps/blog/2014/01/09/rvalue-lifetimes-in-rust/#appendix-b-tail-expressions-in-block
• Issue #3511 - Rationalize temporary lifetimes. #11585
• document and justify temporary lifetime rules in the documentation #12032
• Better temporary lifetimes (tracking issue for RFC 66) #15023 (comment)
• Temporary lifetimes sometimes yield surprising errors #46413
• Lifetimes: Shouldn't the compiler figure out and prolong lifetimes of temporary values where it makes sense? #49118 (comment)

---

Having said that, the semantics is very unlikely to change from what it currently is. The rules weren't chosen randomly. The rules are an attempt to satisfy the needs of various use cases, while still being a relatively small set of uniform rules.

[pnkfelix]

I agree with the analysis by @Havvy above: #54494 (comment)

Closing as notabug.

[pnkfelix]

(Also, I do sympathize that the rules can lead to mistakes at times. It would probably be a good idea to try to improve this section of the reference documentation, along the same lines as proposed here: rust-lang/reference#452 )

[RandomInsano]

Thanks for closing. I forgot it was still around.