Fix resource leak on error in GetDevURandom #10837

pull bytting wants to merge 1 commits into bitcoin:master from bytting:20170715-fix-leak-1 changing 1 files +1 −0

bytting commented at 7:38 PM on July 15, 2017: contributor

Fixes a potential file handle leak when size of entropy is invalid
Fix resource leak a8ae0b252a
in src/random.cpp:194 in a8ae0b252a
```
 190 | @@ -191,6 +191,7 @@ void GetDevURandom(unsigned char *ent32)
 191 |      do {
 192 |          ssize_t n = read(f, ent32 + have, NUM_OS_RANDOM_BYTES - have);
 193 |          if (n <= 0 || n + have > NUM_OS_RANDOM_BYTES) {
 194 | +            close(f);
```
paveljanik commented at 7:43 PM on July 15, 2017:

Is it needed at all as RandFailure() aborts?

bytting commented at 8:11 PM on July 15, 2017:

As I understand it, most OS's will close descriptors on exit, but it is not guaranteed by the C language, so best practice is to close properly.

Also, depending on the behavior of RandFailure, which could change in the future, seems sloppy

TheBlueMatt commented at 12:07 AM on July 16, 2017:

Heh, if we abort() there's gonna be a lot of things left open, but OK.

bytting commented at 6:31 PM on July 16, 2017:

I'm so used to try to bring everything down brick by brick, I can't remember ever resorting to abort, or even exit :worried:
practicalswift commented at 9:14 PM on July 15, 2017: contributor

utACK a8ae0b252a2007568e77f5aca1c7fa3ec5941b72

+1 for explicit closing. Reduces the number of false positives when hunting file descriptor leaks.

Thanks for a nice first-time contribution!
TheBlueMatt commented at 12:07 AM on July 16, 2017: member

utACK, I guess.
practicalswift commented at 1:10 PM on July 16, 2017: contributor

Follow-up PR: RandFailure() (which is called from GetDevURandom(…)) is marked as [[noreturn]] in #10843. Thanks for bringing this to my attention @corebob!
sipa commented at 6:57 PM on July 16, 2017: member

utACK a8ae0b252a2007568e77f5aca1c7fa3ec5941b72. Better to be explicit.
promag commented at 8:59 PM on July 16, 2017: member

utACK a8ae0b2.
paveljanik commented at 7:22 AM on July 17, 2017: contributor

ACK a8ae0b2
laanwj renamed this:
~~Fix resource leak~~
Fix resource leak on error in GetDevURandom
on Jul 17, 2017
laanwj merged this on Jul 17, 2017
laanwj closed this on Jul 17, 2017
laanwj referenced this in commit 8bc6d1f179 on Jul 17, 2017
PastaPastaPasta referenced this in commit 0ecd159637 on Jul 6, 2019
PastaPastaPasta referenced this in commit c42d5a63f7 on Jul 8, 2019
PastaPastaPasta referenced this in commit 28030e4928 on Jul 9, 2019
PastaPastaPasta referenced this in commit ef12c9e478 on Jul 11, 2019
PastaPastaPasta referenced this in commit 90e8a35862 on Jul 13, 2019
PastaPastaPasta referenced this in commit eefadfd051 on Jul 13, 2019
PastaPastaPasta referenced this in commit 83e086554a on Jul 17, 2019
PastaPastaPasta referenced this in commit 16b7769d22 on Jul 23, 2019
PastaPastaPasta referenced this in commit 083c49e1d2 on Jul 24, 2019
barrystyle referenced this in commit 85a11c9c21 on Jan 22, 2020
DrahtBot locked this on Sep 8, 2021

Contributors