Skip to content
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

sysrand leaks threadvar variables #48

Open
n5m opened this issue Nov 1, 2020 · 1 comment
Open

sysrand leaks threadvar variables #48

n5m opened this issue Nov 1, 2020 · 1 comment

Comments

@n5m
Copy link

n5m commented Nov 1, 2020

Calling sysrand.randomBytes causes Valgrind to mark memory as "possibly lost."

Example

# proof.nim
import nimcrypto/sysrand

var x: int
discard randomBytes(addr(x), sizeof(x))

Compile

$ nim c --gc:arc -d:useMalloc proof.nim 
Hint: used config file '/home/user/nim/config/nim.cfg' [Conf]
Hint: used config file '/home/user/nim/config/config.nims' [Conf]
....................
Hint:  [Link]
Hint: 41955 lines; 0.729s; 49.098MiB peakmem; Debug build; proj: /home/user/nimcrypto/proof.nim; out: /home/user/nimcrypto/proof [SuccessX]

Run

$ valgrind --error-exitcode=1 --leak-check=yes ./proof
[[Elided output]]
==989== LEAK SUMMARY:
==989==    definitely lost: 0 bytes in 0 blocks
==989==    indirectly lost: 0 bytes in 0 blocks
==989==      possibly lost: 24 bytes in 1 blocks
==989==    still reachable: 0 bytes in 0 blocks
==989==         suppressed: 0 bytes in 0 blocks
==989== 
==989== For counts of detected and suppressed errors, rerun with: -v
==989== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

This "leak" happens because the global RNG is marked as {.threadvar.}:

var gSystemRng {.threadvar.}: SystemRng ## System thread global RNG

but this seems like marking the variable as {.threadvar.} is avoidable entirely. After examining the way gSystemRng is initialized (when being compiled on Linux),

proc newSystemRNG(): SystemRng =
result = SystemRng()
if SYS_getrandom != 0:
var data: int
result.getRandomPresent = true
let res = syscall(SYS_getrandom, addr data, 1, GRND_NONBLOCK)
if res == -1:
let err = osLastError().int32
if err == ENOSYS or err == EPERM:
result.getRandomPresent = false
proc getSystemRNG(): SystemRng =
if gSystemRng.isNil: gSystemRng = newSystemRng()
result = gSystemRng

it seems that we could have

 var gSystemRng: SystemRng = newSystemRNG() ## System thread global RNG 

which does not have {.threadvar.}. Are there any issues I am overlooking with this approach?

I agree that this isn't a genuine leak since it is a global variable and the memory always still has a reference, but having Valgrind not complain would be a nice feature to have.

@n5m n5m changed the title sysrand leaks threadlocal variables sysrand leaks threadvar variables Nov 1, 2020
@tersec
Copy link
Contributor

tersec commented Feb 29, 2024

At this point, since Nimcrypto requires Nim 1.6 regardless, and https://nim-lang.org/docs/sysrand.html is available (different implementation, same name, same concept/intention) in the Nim standard library, is that an appropriate replacement for Nimcrypto's own sysrand?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants