Skip to content

fix: reject invalid graphs in Import instead of panicking later - #25

Open
Tyagiquamar wants to merge 1 commit into
coder:mainfrom
Tyagiquamar:fix/import-validation
Open

fix: reject invalid graphs in Import instead of panicking later#25
Tyagiquamar wants to merge 1 commit into
coder:mainfrom
Tyagiquamar:fix/import-validation

Conversation

@Tyagiquamar

Copy link
Copy Markdown

Problem

Graph.Import trusted the encoded payload structurally. Three classes of malformed input decoded successfully and produced a graph that panics later, far from the cause:

1. Dangling neighbor reference -> nil-pointer panic on Search

A node whose neighbor list references a key absent from its layer got a nil *layerNode stored in its neighbor map (encode.go filled pointers with nodes[key], which silently yields nil on a miss). The first Search over the graph panics:

panic: runtime error: invalid memory address or nil pointer dereference
github.com/coder/hnsw.(*layerNode[...]).search(...)  graph.go:135
github.com/coder/hnsw.(*Graph[...]).search(...)      graph.go:477
github.com/coder/hnsw.(*Graph[...]).Search(...)      graph.go:433

Reproduced against current main by importing a hand-crafted payload (one layer, one node, one neighbor key not present) and then searching.

2. Inconsistent vector dimensionality -> panic inside the distance function

Import enforced no dimensionality constraint, although the doc comment carves out dimensionality as the one thing that must match, and assertDims enforces uniformity for Add. A file containing vectors of different lengths imports cleanly; afterwards Search panics inside vek32 depending on which node map iteration picks as entry point:

panic: slices must be of equal length
github.com/viterin/vek/vek32.CosineSimilarity(...)
github.com/coder/hnsw.CosineDistance(...)  distance.go:14
github.com/coder/hnsw.(*layerNode[...]).search(...)  graph.go:101

The same applies to importing a differently-dimensional file into a populated graph.

3. Negative counts -> panic during decoding

Negative layer/node/neighbor/vector-length varints reach make(...) (make([]*layer[K], -1), make([]K, -3), make([]float32, -4)) and panic inside Import instead of returning an error.

Fix

Validate while decoding and return errors, consistent with the existing version / distance-function checks:

  • neighbor keys must exist in their layer (node %v has neighbor %v, but it is not present in its layer)
  • every vector must match the dimensionality of the first decoded vector, or of the target graph when it is non-empty
  • negative counts are rejected before reaching make
  • negative string/vector lengths are rejected in binaryRead

Valid Export output never trips any of the new checks: neighbors are always intra-layer by construction, and assertDims keeps dimensions uniform across a graph.

Verification

New tests (encode_test.go), all passing locally on Go 1.26:

  • TestGraph_ImportRejectsDanglingNeighbor - failed with the nil-pointer panic above on unpatched main; now Import errors
  • TestGraph_ImportRejectsInconsistentDims/WithinFile - panicked in vek32 on unpatched main; now errors
  • TestGraph_ImportRejectsInconsistentDims/AgainstGraph - same for import into a populated graph
  • TestGraph_ImportRejectsInvalidCounts/{negativeLayers,negativeNodes,negativeNeighbors,negativeVectorLength} - panicked in make before; now error
  • Full existing suite (go test ./..., includes TestGraph_ExportImport roundtrip and TestSavedGraph) passes unchanged

Performance (BenchmarkGraph_Import, 100 nodes x 256 dims): allocations are byte-identical before/after (3016 allocs/op, ~491 KiB/op). Timings on my noisy Windows laptop were inconclusive run-to-run, but the change adds only an ok check to a map lookup that already existed plus integer comparisons per node - no new allocations or passes over the data.

Import trusted the encoded payload structurally:

- a neighbor key absent from its layer was stored as a nil *layerNode,
  so the first Search over the imported graph panicked with a nil
  pointer dereference
- vectors of mismatched dimensionality were accepted (within one file,
  or against vectors already in the graph), making Search panic inside
  the distance function depending on which node became the entry point
- negative layer/node/neighbor/vector counts reached make() and panicked
  during decoding

All four are rejected with errors now, matching the existing version and
distance-function validation. Valid Export output never triggers any of
the new checks: neighbors are always intra-layer and assertDims keeps
vector dimensions uniform.
@Tyagiquamar

Copy link
Copy Markdown
Author

Hi @ammario, gentle review ping when you have a chance. This is a focused Import validation fix with regression coverage, and the full Go test suite passes. Happy to adjust anything you'd prefer.

@Tyagiquamar

Copy link
Copy Markdown
Author

Hi @suykerbuyk, could you please review when you have a chance? Thanks!

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.

1 participant