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

Ensure Nimbus doesn't overflow converting uint64 to int64 #2366

Closed
6 tasks done
tersec opened this issue Mar 2, 2021 · 6 comments
Closed
6 tasks done

Ensure Nimbus doesn't overflow converting uint64 to int64 #2366

tersec opened this issue Mar 2, 2021 · 6 comments

Comments

@tersec
Copy link
Contributor

tersec commented Mar 2, 2021

nim-lang/Nim#15210 renders this more acute in order to update to Nim 1.2.10 or later.

  • libp2p handlePrune (handlePrune() potentially overflows int64 vacp2p/nim-libp2p#538)
  • Slashing protection v2 (if target.int64 <= minTargetEpoch, if int64(slot) <= minSlot, SlashingProtectionDB_v2 type using int64 rather than uint64 for its prepared SQL statements' types, etc)
  • unixGenesis = fromUnix(genesis_time.int64) in time.nim (low-priority, because it's effectively statically verifiable data made at chain creation time, so nbc can just not support chains made by people who insist on silly timestamps in the far future; it's not an attack vector the way the rest of these could be)
  • Sync Manager (SyncInfo(head_slot: headSlot, sync_distance: int64(wallSlot - headSlot)))
  • Beacon chain database (let queryRes = s.selectStmt.exec(int64(idx) + 1) do (recordBytes: openArray[byte]): where idx is a uint64)
  • SSZ (there is a nextPow2Int64 function, but also elsewhere layer(nextPow2(a.maxChunks.uint64).int64))
@tersec tersec mentioned this issue Mar 2, 2021
@sinkingsugar
Copy link
Contributor

sinkingsugar commented Mar 2, 2021

as I was mentioning in the libp2p issue, using int64 for nanosecond time is not a good idea. Also quite unusual.
uint64_t is the right choice ideally, e.g. http://www.manpagez.com/man/3/clock_gettime_nsec_np/
Of course maybe the refactor is too big.. dunno , but still scary. (The issue is likely Duration needs to be negative aware)

@sinkingsugar
Copy link
Contributor

sinkingsugar commented Mar 2, 2021

https://news.ycombinator.com/item?id=14174958

580/2 years.. maybe it's ok after all.

@tersec
Copy link
Contributor Author

tersec commented Mar 2, 2021

Yeah, none of these are really immediate, practical issues.

Some are probably fine (e.g., the SSZ one likely is, because those HashLists/Arrays only get bounds up to 2^40 in Eth2) in principle. Others are fine for hundreds or thousands of years. Others have pretty benign failure modes (the genesis time one). The libp2p thing is a real edge case.

The point is to collect all of them, so they can be prioritized and addressed.

As regards whether unsigned or signed is a better representation, this comes up in https://github.com/status-im/nim-chronos/blob/master/chronos/timer.nim where fastEpochTimeNano(), which is the system-dependent backend of proc now*(t: typedesc[Moment]): Moment, returns uint64, but all of the functions it uses to get that uint64 (getSystemTimeAsFileTime(), QueryPerformanceFrequency(), and posix_gettimeofday()) return a signed 64-bit integer.

Then,
https://github.com/status-im/nim-chronos/blob/03707426e43d03cccc1de2e7284de168b79f7bf6/chronos/timer.nim#L430-L432

converts that back to a signed 64-bit integer. So the underlying source of Moments appears to be signed and already goes through some conversions, but can't exceed a signed integer's range.

@tersec
Copy link
Contributor Author

tersec commented Mar 10, 2021

status-im/nim-eth#332 isn't part of nbc or its dependencies, so doesn't block updating to 1.2.10. Removing from this issue.

@tersec
Copy link
Contributor Author

tersec commented Mar 10, 2021

#2392 and #2395 address the slashing protection database.

@tersec
Copy link
Contributor Author

tersec commented Mar 10, 2021

#2398 addresses the DbSeq one.

At this point, what remains either I didn't discover for this bug or is the libp2p one, which shouldn't block the 1.2.10 update, and if people want to keep track of it, can track vacp2p/nim-libp2p#538 directly, so there's little remaining use for this issue.

@tersec tersec closed this as completed Mar 10, 2021
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