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

oids: switch from PRNG to random module #16203

Merged
merged 10 commits into from
Jan 7, 2021
Merged

oids: switch from PRNG to random module #16203

merged 10 commits into from
Jan 7, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 1, 2020

0x7fff is enough to use and is often used as RAND_MAX in PRNG.
http://www.cplusplus.com/reference/cstdlib/RAND_MAX

  • If this PR is acceptable, next PR will add a overload for genOid which accepts Rand as a parameter.

Benefits:

  • More pure(more possible to work in JS backend)
  • Benefits from the improvement from random module
  • Random module also provides better interface(initRand, next) and could be used to improve genOid

@planetis-m
Copy link
Contributor

Could it be easy to convert cstring usage to strings, or at least openarray[char]?

@ringabout
Copy link
Member Author

I don't like oidToString*(oid: Oid, str: cstring) because cstring slice doesn't work in JS backend. I will consider cleaning these procs in next PR(if possible). At least proc $*(oid: Oid): string shouldn't rely on cstring version.

lib/pure/oids.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

timotheecour commented Dec 2, 2020

use this (I checked, it works):

proc toStringImpl*[T: string | cstring](result: var T, oid: Oid) =
  ## Converts an oid to `str`.
  type T = typeof(result)
  const hex = "0123456789abcdef"
  const N = 24

  when T is string:
    result.setLen N

  var o = oid
  var bytes = cast[cstring](addr(o))
  var i = 0
  while i < 12:
    let b = bytes[i].ord
    result[2 * i] = hex[(b and 0xF0) shr 4]
    result[2 * i + 1] = hex[b and 0xF]
    inc(i)
  when T is cstring:
    result[N] = '\0'

proc oidToString*(oid: Oid, str: cstring) {.deprecated: "unsafe; use `$`".} =
  ## assumes str is space allocated for 25 elements.
  # work around a compiler bug:
  var str = str
  toStringImpl(str, oid)

proc `$`*(oid: Oid): string =
  ## Converts an oid to string.
  toStringImpl(result, oid)

(refs #16201 (comment))
EDIT: => #16704

@timotheecour
Copy link
Member

timotheecour commented Dec 2, 2020

@xflywind this PR introduces a regression:

when true:
  import std/oids
  import std/random
  import std/sugar

  proc main()=
    let s = collect(newSeq):
      for i in 0..<10: rand(int.high)
    echo s
  main()

nim c main

before PR:

  • commenting or not import std/oids gives same results
    after PR:
  • with import std/oids, running ./main gives same results for runs within 1 second apart (seeding via var t = getTime().toUnix.int32 is bad, 0-arg random.randomize() is better at least, but still this needs more thought)

I recommend splitting this PR in 2:

  • 1 for all the cleanups (no change to PRNG) => non controversial, easy to accept, after above proposed changes have been addressed
  • 1 for the PRNG => random change, which needs more care and should be done separately

@timotheecour timotheecour changed the title switch from PRNG to random module oids: switch from PRNG to random module Dec 2, 2020
@ringabout ringabout marked this pull request as draft December 3, 2020 00:20
@ringabout ringabout marked this pull request as ready for review December 3, 2020 06:45
@ringabout
Copy link
Member Author

The regression has been fixed.

lib/pure/oids.nim Outdated Show resolved Hide resolved
@ringabout ringabout requested a review from Araq December 7, 2020 11:36
@FedericoCeratto
Copy link
Member

The random module is not cryptographically secure.
Please put a warning in the documentation to remind users that future OIDs can become predictable to an attacker that can reverse the initial seed value.

@ringabout
Copy link
Member Author

ringabout commented Dec 7, 2020

Since original implementation using srand and rand is not secure(?) too, I could add these warnings in the following PR.

http://www.cplusplus.com/reference/cstdlib/srand/

lib/pure/oids.nim Outdated Show resolved Hide resolved
lib/pure/oids.nim Outdated Show resolved Hide resolved
@ringabout ringabout marked this pull request as draft December 8, 2020 10:17
@ringabout ringabout marked this pull request as ready for review December 25, 2020 08:32
@ringabout ringabout requested review from timotheecour and removed request for Araq December 28, 2020 13:28
@planetis-m
Copy link
Contributor

Can this be merged as is? It's already an improvement.

@Araq Araq merged commit 89a21e4 into nim-lang:devel Jan 7, 2021
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* switch from PRNG to random module
* fix the regression
* comments + tests
* runnableExamples
* make oids better
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* switch from PRNG to random module
* fix the regression
* comments + tests
* runnableExamples
* make oids better
narimiran pushed a commit that referenced this pull request Dec 13, 2021
* switch from PRNG to random module
* fix the regression
* comments + tests
* runnableExamples
* make oids better

(cherry picked from commit 89a21e4)
narimiran pushed a commit that referenced this pull request Dec 13, 2021
* switch from PRNG to random module
* fix the regression
* comments + tests
* runnableExamples
* make oids better

(cherry picked from commit 89a21e4)
@ringabout ringabout added the TODO: followup needed remove tag once fixed or tracked elsewhere label Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants