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

Use /dev/urandom and sysctl(RANDOM_UUID) on Linux. #743

Merged
merged 1 commit into from Feb 5, 2018

Conversation

Projects
None yet
2 participants
@bnoordhuis
Member

bnoordhuis commented Feb 4, 2018

Add fallback paths for when the getrandom(2) system call is not
available. Try /dev/urandom first and sysctl(RANDOM_UUID) second.

The sysctl issues a warning in the system logs with some kernels but
that seems like an acceptable tradeoff for the fallback of a fallback.

Use /dev/urandom and sysctl(RANDOM_UUID) on Linux.
Add fallback paths for when the getrandom(2) system call is not
available.  Try /dev/urandom first and sysctl(RANDOM_UUID) second.

The sysctl issues a warning in the system logs with some kernels but
that seems like an acceptable tradeoff for the fallback of a fallback.

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:linux-random branch from 6a8a9e6 to 73ee434 Feb 4, 2018

@andrewrk

This comment has been minimized.

Member

andrewrk commented Feb 4, 2018

Looks good to me. Did you just push an update to fix the tests?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 4, 2018

Yep, ~30 minutes ago. I was going to wait until appveyor was done before posting a "fixed!" comment.

@andrewrk andrewrk merged commit ec59f76 into ziglang:master Feb 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andrewrk

This comment has been minimized.

Member

andrewrk commented Feb 5, 2018

Thanks!

@andrewrk

This comment has been minimized.

Member

andrewrk commented Feb 5, 2018

I noticed some stuff like usize(-EIO) and realized that there was a missing compile error for that. Integer casts maintain the integer value and assert that no data was lost. The way to accomplish what you want in zig is @bitCast(usize, isize(-EIO)). So I added the compile error for this.

I read up on the docs for sysctl, and found this:

Do not use this system call
don't call it: use of this system call has
long been discouraged, and it is so unloved that it is likely to disap‐
pear in a future kernel version. Since Linux 2.6.24, uses of this sys‐
tem call result in warnings in the kernel log. Remove it from your
programs now; use the /proc/sys interface instead.

 This  system  call  is available only if the kernel was configured with
 the CONFIG_SYSCTL_SYSCALL option.

BUGS
The object names vary between kernel versions, making this system call
worthless for applications.

I think let's try getting away with not supporting this, unless you have an actual use case in mind?

Further, since we don't actually store method yet, let's just handle ENOSYS since the logic is way simpler. And let's use the posix abstraction functions already available (see the Darwin getRandomBytes implementation).

Good point about read only supporting 0x7ffff000 bytes at max. That should go into the posixRead function so that it gets fixed everywhere. However, upon further reading, it looks like it's ok to ask for more bytes to be transferred, because we check the return value and handle the case when fewer than requested are transferred.

I already pushed all the above stuff into master. I hope you don't mind, it restructured a lot of what you did.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 5, 2018

So I added the compile error for this.

Good idea!

I think let's try getting away with not supporting this, unless you have an actual use case in mind?

Yes, kernels < 3.17 and no /dev mounted. One somewhat common use case is people running applications in containers.

However, upon further reading, it looks like it's ok to ask for more bytes to be transferred

Apologies, I should have written a better comment. Linux returns EINVAL when you pass in a value > INT_MAX, at least some of the time. We have hit this bug in libuv.

@bnoordhuis bnoordhuis deleted the bnoordhuis:linux-random branch Feb 5, 2018

@andrewrk

This comment has been minimized.

Member

andrewrk commented Feb 5, 2018

One somewhat common use case is people running applications in containers.

I'm willing to support this use case, but isn't there kinda no excuse for using an old kernel in a container?

Apologies, I should have written a better comment. Linux returns EINVAL when you pass in a value > INT_MAX, at least some of the time. We have hit this bug in libuv.

Ok, good to know. I'll push a fix to std.os.posixRead right away. Does the same thing go for write?

andrewrk added a commit that referenced this pull request Feb 5, 2018

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 5, 2018

There's no excuse for lots of things but people still do them. If - I mean: when! - zig is going to get popular, you're going to get bug reports about it sooner or later.

Does the same thing go for write?

Yes. Now that I think about it, it's predominantly an issue with 32 bits kernels although you can probably still trigger it on 64 bits kernels if you try to read or write 2**63 bytes or more.

@andrewrk

This comment has been minimized.

Member

andrewrk commented Feb 5, 2018

you're going to get bug reports about it sooner or later.

I agree with this, and I don't want to be stubborn here, but, would it be reasonable to tell people, "your use case is invalid and is therefore not supported"?

Yes. Now that I think about it, it's predominantly an issue with 32 bits kernels

Makes sense. I'll push another fix.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 5, 2018

would it be reasonable to tell people, "your use case is invalid and is therefore not supported"?

Hey, it's your project; that's your call to make.

andrewrk added a commit that referenced this pull request Feb 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment