Remove libstd dependancy for Opening and Reading files

[josephlr]

Depends on #54 so only look at the last commit if you want to just review this PR.

This PR removes the last part of libstd we were depending on (when not using the std feature). Specifically:

• We now manually open files with libc::open(path.as_ptr(), libc::O_RDONLY | libc::O_CLOEXEC)
  • We don't care about Linux kernels too old to support libc::O_CLOEXEC (kernel < 2.6.22)
• All the filenames now have to be null-terminated
• Instead of reading one byte from /dev/random we now poll(2) on the /dev/random file descriptor
  • This still lets us know if /dev/urandom is safe
  • This avoids ever actually reading from /dev/random
  • This is what libsodium does
  • See existing discussion in Verify /dev/urandom has been seeded before using it. briansmith/ring#558

[briansmith]

• Suggested by @briansmith in briansmith/ring#558

As you can read in the linked ring issue, I made so such suggestion.

[josephlr]

• Suggested by @briansmith in briansmith/ring#558

As you can read in the linked ring issue, I made so such suggestion.

Whoops sorry about that, meant to say you pointed out the libsodium implementation, changed to be more accurate.

[dhardy]

I guess it's time for a rebase — however I believe the only motivation is rust-lang/rust#62082, thus ideally this should wait on a decision whether we should go ahead with that (except that decision may depend on the existence of a viable PR...)

[josephlr]

@dhardy I rebased this PR, and the CI is passing. I agree that if we don't do rust-lang/rust#62082, we should not merge this PR.

[newpavlov]

You also have to disable default features for libc, otherwise getrandom will not be std-independent.

[josephlr]

You also have to disable default features for libc, otherwise getrandom will not be std-independent.

Done

[josephlr]

As rust-lang/rust#62516 might take awhile to reslove. I just added in the RHEL 5 compat code. It only takes 2 lines.

[newpavlov]

So I think we can merge this without waiting for decision regarding rust-lang/rust#62082, if getrandom will not be used as part of std we may revert this change in a later release. But first I think we should decide what to do with the MSRV issue raised in #69.

[josephlr]

I slightly changed the use_file implementation to use cfg-if to determine if we should poll /dev/random. That seemed less hacky than checking the value of FILE_PATH. I also rebased this onto master to make sure there were no merge issues.

So I think we can merge this without waiting for decision regarding rust-lang/rust#62082.

Sounds reasonable to me.

[newpavlov]

Can you also update the table in lib.rs and replace "after reading from /dev/random once" to something like "after polling /dev/random once"?

[josephlr]

Can you also update the table in lib.rs and replace "after reading from /dev/random once" to something like "after polling /dev/random once"?

Done, I also fixed up the file_init loop where we poll, hopefully it's more clear now.

[newpavlov]

I think you can simplify the loop even further like this:

let ret = loop {
    // A negative timeout means an infinite timeout.
    let res = unsafe { libc::poll(&mut pfd, 1, -1) };
    if res == 1 {
        break unsafe { open_readonly("/dev/urandom\0") };
    } else if res < 0 {
        let e = last_os_error().raw_os_error();
        if e == Some(libc::EINTR) || e == Some(libc::EAGAIN) {
            continue;
        }
    }
    break None;
};
unsafe { libc::close(pfd.fd) };
ret

[josephlr]

I think you can simplify the loop even further like this:

Done. I totally forgot that rust loops can return a value.

[dhardy]

What happens in this case when the getrandom crate is used both by std and by other crates wanting std features?

[newpavlov]

IIUC right now getrandom will be compiled into std, so enabling features for it during usual builds will not influence the copy of the crate used by std. Although I am not sure how it will work with rust-lang/rfcs#2663.

[dhardy]

So I think we can merge this without waiting for decision regarding rust-lang/rust#62082, if getrandom will not be used as part of std we may revert this change in a later release. But first I think we should decide what to do with the MSRV issue raised in #69.

There may not be another way to go about this, so fair enough.

[josephlr]

@newpavlov if we're going with the build script approach in #69, do we still want to wait on that PR before merging this one?

[newpavlov]

No, I think we can merge it right away.

[RalfJung]

We don't care about Linux kernels too old to support libc::O_CLOEXEC (kernel < 2.6.22)

Wasn't the plan to use this crate in libstd? Rust officially supports Linux 2.6.18+.

[mati865]

Description was not updated to reflect #58 (comment)