Skip to content

Conversation

@777genius
Copy link

Summary

This PR adds missing error checking for binary.Varint and binary.Uvarint calls in nodedb.go to ensure data integrity and prevent incorrect behavior when reading corrupted database entries.

Problem

Missing validation in two locations leads to inconsistency and potential issues:

  1. Line 223 (fetchUint64): Ignores n return value - inconsistent with fetchInt64 which properly validates
  2. Line 341 (expireNodes): Ignores n return value when reading pong timestamps

When binary.Varint/Uvarint returns n <= 0, it indicates:

  • n == 0: Buffer too small (incomplete varint)
  • n < 0: Value overflow (> 64 bits)

Without checks:

  • fetchUint64() can return garbage values from corrupted data
  • expireNodes() can use invalid timestamps leading to incorrect node expiration decisions
  • Inconsistency between fetchInt64() (which validates) and fetchUint64() (which doesn't)

Solution

Added validation following the existing fetchInt64() pattern (lines 203-206):

val, n := binary.Uvarint(blob)
if n <= 0 {
    return 0
}
return val

For expireNodes(), added validation with silent skip:

time, n := binary.Varint(it.Value())
if n <= 0 {
    // Skip corrupted timestamp entry
    atEnd = !it.Next()
    continue
}

Testing

Added comprehensive tests:

  • TestDBFetchUint64Corrupted: Tests truncated varint, overflow varint, empty data, and valid data cases
  • TestDBExpireNodesCorrupted: Tests that expireNodes handles corrupted pong timestamps gracefully
  • All existing tests pass

Context

This fixes an inconsistency where fetchInt64() at lines 203-206 properly validates the return value but fetchUint64() does not.

This commit adds missing error checking for binary.Varint and binary.Uvarint
calls in nodedb.go to ensure data integrity and prevent incorrect behavior
when reading corrupted database entries.

Missing validation could lead to:
- fetchUint64() returning garbage values from corrupted data
- expireNodes() using invalid timestamps for node expiration decisions
- Incorrect node lifecycle management

Changes:
- Add n <= 0 validation in fetchUint64() (line 224) matching fetchInt64() pattern
- Add n <= 0 validation in expireNodes() (line 345) to skip corrupted timestamps
- Add comprehensive tests for corrupted varint handling
  - TestDBFetchUint64Corrupted: Tests truncated, overflow, and empty varint cases
  - TestDBExpireNodesCorrupted: Tests expireNodes with corrupted pong timestamps

Fixes inconsistency where fetchInt64() properly validates but fetchUint64() does not.
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

Successfully merging this pull request may close these issues.

2 participants