Skip to content

fix: release the reference lock before resolving its pointer #230

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.

Sign up for GitHub

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

Open
OmarAlJarrah wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: release the reference lock before resolving its pointer #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3efdc1c
fix: release the reference lock before resolving its pointer
OmarAlJarrah Aug 2, 2026
d02ba76
fix: stop GetObject recursing through a cyclic resolution chain
OmarAlJarrah Aug 2, 2026
1fe8ffc
fix: keep parent links out of a cyclic resolution chain
OmarAlJarrah Aug 3, 2026
9f9c8a6
refactor: flatten the double-check in Reference.resolve
OmarAlJarrah Aug 3, 2026
f58f208
fix: derive parent-link safety from the links, not the current call
OmarAlJarrah Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 94 additions & 16 deletions openapi/reference.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"slices"
"strings"
"sync"

Expand Down Expand Up @@ -290,15 +291,38 @@ func (r *Reference[T, V, C]) GetObject() *T {
return r.Object
}

r.ensureMutex()
r.cacheMutex.RLock()
defer r.cacheMutex.RUnlock()
// Walk the resolution chain rather than recursing through it. A reference
// whose pointer names a reference already in the chain publishes a cache
// entry that closes the loop: a reference can resolve to itself, and two
// references can resolve to each other. Recursing through that exhausts the
// goroutine stack, so track what has been seen and report the reference as
// unresolved instead.
//
// The array sizes the common case; deeper chains grow onto the heap.
var backing [8]*Reference[T, V, C]
seen := backing[:0]

for current := r; current != nil; {
if !current.IsReference() {
return current.Object
}

if slices.Contains(seen, current) {
return nil
}
seen = append(seen, current)

current.ensureMutex()
current.cacheMutex.RLock()
cache := current.referenceResolutionCache
current.cacheMutex.RUnlock()

if (r.referenceResolutionCache != nil && r.referenceResolutionCache.Object != nil) || r.circularErrorFound {
if r.referenceResolutionCache != nil && r.referenceResolutionCache.Object != nil {
return r.referenceResolutionCache.Object.GetObject()
if cache == nil {
return nil
}
current = cache.Object
}

return nil
}

Expand Down Expand Up @@ -533,17 +557,24 @@ func (r *Reference[T, V, C]) resolve(ctx context.Context, opts references.Resolv
}
r.cacheMutex.RUnlock()

// Need to resolve (with write lock)
// Need to resolve, and that has to happen with the lock released:
// references.Resolve navigates the document and calls GetObject on
// references it traverses, which takes a read lock. sync.RWMutex is not
// reentrant, so holding this reference's lock across that call
// self-deadlocks when the traversal reaches it, directly or via a cache
// forward.
r.cacheMutex.Lock()
defer r.cacheMutex.Unlock()
cache := r.referenceResolutionCache
cachedErrs := r.validationErrsCache
r.cacheMutex.Unlock()

// Double-check after acquiring write lock
if r.referenceResolutionCache != nil {
if r.referenceResolutionCache.Object.IsReference() {
return nil, r.referenceResolutionCache.Object, r.validationErrsCache, nil
} else {
return r.referenceResolutionCache.Object.Object, nil, r.validationErrsCache, nil
// Double-check: another goroutine may have published between the read above
// and here.
if cache != nil {
if cache.Object.IsReference() {
return nil, cache.Object, cachedErrs, nil
}
return cache.Object.Object, nil, cachedErrs, nil
}

rootDoc, ok := opts.RootDocument.(*OpenAPI)
Expand All @@ -565,6 +596,16 @@ func (r *Reference[T, V, C]) resolve(ctx context.Context, opts references.Resolv
return nil, nil, validationErrs, err
}

// Re-acquire to publish the result.
r.cacheMutex.Lock()
defer r.cacheMutex.Unlock()
if r.referenceResolutionCache != nil {
if r.referenceResolutionCache.Object.IsReference() {
return nil, r.referenceResolutionCache.Object, r.validationErrsCache, nil
}
return r.referenceResolutionCache.Object.Object, nil, r.validationErrsCache, nil
}

r.referenceResolutionCache = result

@TristanSpeakEasy TristanSpeakEasy Aug 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still publishes a self-reference for the trimmed-pointer case. I verified after ResolveAllReferences returns circular reference detected that ref.referenceResolutionCache.Object == ref. The tracker sets circularErrorFound later, but it does not clear this cache entry, so any subsequent ref.GetObject() reaches line 299 and delegates straight back to ref.GetObject() until the process exhausts its goroutine stack. The new test only checks the resolution error and therefore misses the corrupt post-resolution state. Please avoid publishing a result whose object is this reference (or otherwise represent the circular state without a self-delegating cache), and add a regression that safely verifies GetObject() after the circular resolution attempt.

@OmarAlJarrah OmarAlJarrah Aug 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in d02ba76, though not by suppressing the publish.

A pointer-identity check against the reference being resolved turned out to catch only the one-node case. Two references can close the loop between them without either being a self-reference:

paths:
  /a:
    $ref: '#/paths/~1b '
    get: {operationId: a, responses: {"200": {description: ok}}}
  /b:
    $ref: '#/paths/~1a '
    get: {operationId: b, responses: {"200": {description: ok}}}

The tracker reports circular reference detected: test.yaml#/paths/~1b -> test.yaml#/paths/~1a -> test.yaml#/paths/~1b, but only on the hop after both caches are published, so a.cache.Object == b and b.cache.Object == a and neither == r. GetObject then alternates between them until the stack goes.

Not publishing is also awkward here: resolveObjectWithTracking reads ref.referenceResolutionCache.ResolvedDocument right after resolve returns a next reference, so an empty cache turns the overflow into a nil dereference.

So GetObject now walks the chain iteratively with a seen set and returns nil once it repeats, which covers a cycle of any length and leaves the resolution errors and the published cache as they were. Legitimate circular references were already safe because the tracker stops before the last hop's cache is published, and there is now a test pinning that alongside a multi-hop chain that still resolves.

Both cycle shapes are regression tested through GetObject after resolution: with the walk reverted they abort the binary with fatal error: stack overflow. go test ./..., -race on ./openapi/... ./references/... ./jsonpointer/..., and golangci-lint are clean.

r.validationErrsCache = validationErrs

Expand Down Expand Up @@ -633,8 +674,22 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co
} else {
topLevel = ref
}
nextRef.SetParent(ref)
nextRef.SetTopLevelParent(topLevel)
// Only link a reference that is not already an ancestor of this one.
// A pointer can name a reference the chain has been through (its own,
// or one an earlier hop went through), and parenting that reference to
// its own descendant closes a loop in the links GetParent and
// GetTopLevelParent expose to callers.
//
// Ancestry is read from the links rather than from this call's chain,
// because each member of a cycle can be resolved by a separate call:
// resolving /a leaves b.parent = a, and resolving /b afterwards starts
// a fresh chain that would otherwise add a.parent = b on top of it.
// The recursive call below reports the cycle either way; leaving the
// links alone keeps them walkable meanwhile.
if !isAncestor(ref, nextRef) && topLevel != nextRef {
nextRef.SetParent(ref)
nextRef.SetTopLevelParent(topLevel)
}

// For chained resolutions, we need to use the resolved document from the previous step
// The ResolveResult.ResolvedDocument should be used as the new TargetDocument
Expand All @@ -652,6 +707,29 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co
return validationErrs, fmt.Errorf("unable to resolve reference: %s", ref.GetReference())
}

// isAncestor reports whether candidate is ref itself or is reachable from ref by
// following parent links, which is what makes candidate unsafe to parent to ref.
//
// The links are already acyclic, so the walk terminates; the seen set is there
// so that a graph left cyclic by an older version stops the walk rather than
// hanging the caller.
func isAncestor[T any, V interfaces.Validator[T], C marshaller.CoreModeler](ref, candidate *Reference[T, V, C]) bool {
var backing [8]*Reference[T, V, C]
seen := backing[:0]

for current := ref; current != nil; current = current.GetParent() {
if current == candidate {
return true
}
if slices.Contains(seen, current) {
return false
}
seen = append(seen, current)
}

return false
}

// joinReferenceChain joins the reference chain with arrows to show the circular path
func joinReferenceChain(chain []string) string {
if len(chain) == 0 {
Expand Down
Loading