urandom is never closed

[marco-palumbi]

Hi Daan, I found a little issue here:

randombytes/randombytes.c

Line 221 in a29d979

fd = open("/dev/urandom", O_RDONLY);

/dev/urandom is opened at each call of the randombytes_linux_randombytes_urandom() but never closed.
I would suggest ether:

• close at the exit of the function
• use for fd a static variable initialized to -1 test it and if equal to -1 open the device and assign the descriptor to fd

the last could save some time if the function is called repeatedly

[thomwiggers]

• use for fd a static variable initialized to -1 test it and if equal to -1 open the device and assign the descriptor to fd

the last could save some time if the function is called repeatedly

I don't think that's thread-safe; and using thread_local probably imposes requirements that you don't want to include everywhere...

[marco-palumbi]

probably you are right @thomwiggers.
for a thread-safe safe implementation remain the option of closing the descriptor at each call or use a sort of init_random() to be called at the very beginning from the first thread..

[thomwiggers]

I highly doubt that's worth the complexity for what is essentially a fallback used only if more modern methods (getentropy) aren't available.

[marco-palumbi]

well.. if the code is there it can be used... and if you use it for any reason if you call the function for a while, you will get an error because you reach the maximum for the open file descriptors.
I got the bug because I used this code and after a while I start getting the error "Too many open files"

please note that, with the current implementation, each time you call the randombytes() function a new file descriptor is opened

[thomwiggers]

To clarify: closing the file descriptor is worth it; thread-local storage or init functions aren't.

[marco-palumbi]

We are on the same page then..
I had interpreted your previous comment in a more general way, sorry for that

[dsprenkels]

Hrm. This is an interesting point. This is a bug and should be fixed.

I see two options:

1. We make no guarantees about the performance, and we are only using /dev/urandom on older linux kernels, so close the file after every call.
2. When we are using /dev/urandom we are always using linux (right?), so we will always have access to some kind of linux synchronization primitives (maybe futex?). So in this case we can open the file once, and use the linux synchronization primitives to ensure we only open it once.

I have no clue how we would implement 2., so my vote would go to 1.

[dsprenkels]

@marco-palumbi PS Thanks for the bug report!