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

randomPathName called twice in a row can return the same string on windows #17898

Closed
timotheecour opened this issue Apr 30, 2021 · 3 comments · Fixed by #18729 or #18744
Closed

randomPathName called twice in a row can return the same string on windows #17898

timotheecour opened this issue Apr 30, 2021 · 3 comments · Fixed by #18729 or #18744

Comments

@timotheecour
Copy link
Member

timotheecour commented Apr 30, 2021

this is the 2nd bug with std/tempfiles i've encountered while working on #17892 (i already sent a fix for the 1st one #17888 which affected it). This shows that dog-fooding is useful and we should do more of it (ie, promote using stdlib within nim repo).

Example

# see https://github.com/nim-lang/Nim/pull/17892 for `genTempPath`, which
# is based on `randomPathName`
proc diffStrings*(a, b: string): tuple[output: string, same: bool] =
  template tmpFileImpl(prefix, str): auto =
    let path = genTempPath(prefix, "")
    writeFile(path, str)
    path
  let patha = tmpFileImpl("diffStrings_a_", a)
  let pathb = tmpFileImpl("diffStrings_b_", b)
  defer:
    removeFile(patha)
    removeFile(pathb)
  result = diffFiles(patha, pathb)

Current Output

on windows, in CI, echo diffStrings(lhs, rhs) produced the following:

2021-04-29T20:00:01.1496001Z --- "a/C:\\Users\\VSSADM~1\\AppData\\Local\\Temp\\diffStrings_a_C5xLA9E2"
2021-04-29T20:00:01.1507701Z +++ "b/C:\\Users\\VSSADM~1\\AppData\\Local\\Temp\\diffStrings_b_C5xLA9E2"

(see also https://gist.github.com/timotheecour/515dde92cb29ce359a3f4b9546c247fd)

it looks like randomPathName produced the same string C5xLA9E2, and this is unlikely to be by accident.

Expected Output

randomPathName called twice in a row should return different strings with very high probability.

Additional Information

1.5.1 1640508

I'm not on windows, but windows users should be able to minimize this issue with a reproducible example based on this, eg by calling randomPathName in a loop and checking for nearest neighbor duplicates

/cc @xflywind can you reproduce this on your end?

@ringabout
Copy link
Member

ringabout commented Apr 30, 2021

let's add secure random state for initRand?

@ringabout
Copy link
Member

ringabout commented Apr 30, 2021

Using sysrand can mitigate this problem.

  import times, std/sysrand

  type
    RandomState = enum
      useTime, useSysrand

  proc initRand(opt = useTime): Rand =
    case opt
    of useTime:
      when defined(js):
        let time = int64(times.epochTime() * 1000) and 0x7fff_ffff
        result = initRand(time)
      else:
        let now = times.getTime()
        result = initRand(convert(Seconds, Nanoseconds, now.toUnix) + now.nanosecond)
    of useSysrand:
      let state = urandom(8)
      var num = 0'i64
      for idx in 0 .. 7:
        num += int64(state[idx]) shr (idx * 8'i64)
      when defined(js):
        result = initRand(num)
      else:
        result = initRand(num and 0x7fff_ffff)

Ref timotheecour#429 (comment)

@ringabout
Copy link
Member

ringabout commented Apr 30, 2021

Maybe std/times should use https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemtimepreciseasfiletime instead of GetSystemTimeAsFileTime of which resolution is
10-15 ms.

Function Return type Resolution Timezone
GetLocalTime SYSTEM_TIME (1ms) ~10-15 ms Local
GetSystemTime SYSTEM_TIME (1ms) ~10-15 ms UTC
GetSystemTimeAsFileTime FILE_TIME (0.1us) ~10-15 ms UTC
GetSystemTimePreciseAsFileTime FILE_TIME (0.1us) 0.1 us UTC
       
GetTickCount Int32 (1ms) ~10-15 ms n/a
GetTickCount64 Int64 (1ms) ~10-15 ms n/a
QueryPerformanceCounter Int64 (ticks) 0.1 us n/a

see https://stackoverflow.com/questions/34557407/queryperformancecounter-or-getsystemtimepreciseasfiletime-when-using-sntp

@ringabout ringabout self-assigned this Apr 30, 2021
ringabout added a commit to ringabout/Nim that referenced this issue Apr 30, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 2, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 2, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 2, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 2, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 2, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 2, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 3, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 3, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 6, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Jun 6, 2021
ringabout added a commit to ringabout/Nim that referenced this issue Jul 1, 2021
ringabout added a commit to ringabout/Nim that referenced this issue Aug 22, 2021
Araq pushed a commit that referenced this issue Aug 22, 2021
…tring on windows) (#18729)

* close #17898

* no need to consider js
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment