From 3efdc1cfa7f0b5bce2db8bbde2ccf59aa424e7ae Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 2 Aug 2026 06:16:26 +0300 Subject: [PATCH 1/5] fix: release the reference lock before resolving its pointer Reference.resolve held the reference's own cacheMutex write lock across the call to references.Resolve. That call navigates the document, and navigating into a reference calls GetObject, which takes a read lock on that reference. sync.RWMutex is not reentrant, so a $ref whose JSON pointer passes through the reference being resolved blocked forever on a lock its own goroutine held. A pointer reaches the in-flight reference either directly, when its own prefix names it: paths: /a: {$ref: '#/paths/~1a/t'} or through GetObject's cache delegation, when an already-resolved reference forwards to the one currently resolving: paths: /a: {$ref: '#/paths/~1b'} /b: {$ref: '#/paths/~1a/t'} Both hang the process. Neither is caught by resolveObjectWithTracking, whose reference chain is only extended once a hop completes, so a hop that re-enters itself is never compared against it. The same lock scope caused a second failure. When a reference resolved to itself -- reachable because GetJSONPointer trims the pointer, so '#/paths/~1a ' names /a -- the resolution cache was left pointing at its own reference, and GetObject's delegation to referenceResolutionCache.Object.GetObject() recursed until the goroutine stack was exhausted. Take the write lock only for the double-check, release it while resolving, and re-acquire to publish the result, re-checking the cache in case another goroutine published first. Concurrent resolution of the same reference may now duplicate work, which is wasted effort rather than a correctness problem: the published result is whichever completes first, and every caller returns it. With the lock released, both shapes resolve to the errors they should always have produced -- an unresolved reference, or "circular reference detected" from the existing tracker. --- openapi/reference.go | 20 +++++- openapi/reference_mutex_test.go | 119 ++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/openapi/reference.go b/openapi/reference.go index 4c307762..15cc348f 100644 --- a/openapi/reference.go +++ b/openapi/reference.go @@ -533,12 +533,16 @@ 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. Take the write lock only for the double-check, then + // release it: references.Resolve navigates the document and calls GetObject + // on references it traverses, which takes a read lock. sync.RWMutex is not + // reentrant, so holding the write lock across that call self-deadlocks when + // the traversal reaches this reference (directly or via a cache forward). r.cacheMutex.Lock() - defer r.cacheMutex.Unlock() // Double-check after acquiring write lock if r.referenceResolutionCache != nil { + defer r.cacheMutex.Unlock() if r.referenceResolutionCache.Object.IsReference() { return nil, r.referenceResolutionCache.Object, r.validationErrsCache, nil } else { @@ -546,6 +550,8 @@ func (r *Reference[T, V, C]) resolve(ctx context.Context, opts references.Resolv } } + r.cacheMutex.Unlock() + rootDoc, ok := opts.RootDocument.(*OpenAPI) if !ok { return nil, nil, nil, fmt.Errorf("root document must be *OpenAPI, got %T", opts.RootDocument) @@ -565,6 +571,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 r.validationErrsCache = validationErrs diff --git a/openapi/reference_mutex_test.go b/openapi/reference_mutex_test.go index 269960ae..97b6928d 100644 --- a/openapi/reference_mutex_test.go +++ b/openapi/reference_mutex_test.go @@ -1,11 +1,14 @@ package openapi import ( + "strings" "sync" "testing" + "time" "github.com/speakeasy-api/openapi/openapi/core" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestEnsureMutex_ConcurrentAccess verifies that ensureMutex is safe to call @@ -55,3 +58,119 @@ func TestEnsureMutex_CopiedReference(t *testing.T) { assert.NotNil(t, original.initMutex, "original initMutex should still be set") assert.NotNil(t, original.cacheMutex, "original cacheMutex should still be set") } + +// TestResolveAllReferences_PointerTraversingItsOwnReference verifies that a +// $ref whose JSON pointer passes through the reference being resolved does not +// deadlock. +// +// Reference.resolve used to hold the reference's own write lock across +// references.Resolve. That call navigates the document, and navigating into a +// reference calls GetObject, which takes a read lock. sync.RWMutex is not +// reentrant, so a pointer whose prefix named the reference being resolved +// blocked forever on a lock its own goroutine held. +// +// Each case below is a document that hung before the fix. The expected result +// is an unresolved-reference error, which is what every other pointer that +// names nothing already produced. +func TestResolveAllReferences_PointerTraversingItsOwnReference(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + spec string + }{ + { + name: "prefix names the reference being resolved", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: {$ref: '#/paths/~1a/t'} +`, + }, + { + name: "prefix names the reference and the pointer resolves", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: + $ref: '#/paths/~1a/get' + get: {operationId: a, responses: {"200": {description: ok}}} +`, + }, + { + name: "prefix reaches the in-flight reference through a resolved one", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: {$ref: '#/paths/~1b'} + /b: {$ref: '#/paths/~1a/t'} +`, + }, + { + name: "components spelling", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: {$ref: '#/components/pathItems/A'} +components: + pathItems: + A: {$ref: '#/components/pathItems/A/t'} +`, + }, + { + name: "webhooks spelling", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: {} +webhooks: + onA: {$ref: '#/webhooks/onA/t'} +`, + }, + { + // GetJSONPointer trims the pointer, so this names /a and the + // reference resolves to itself. Before the fix the resolution cache + // pointed at its own reference and GetObject's delegation at the end + // of the cache-hit branch recursed until the stack was exhausted, + // taking the process with it rather than failing the test. + name: "reference resolving to itself via a trimmed pointer", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: + $ref: '#/paths/~1a ' + get: {operationId: a, responses: {"200": {description: ok}}} +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + doc, _, err := Unmarshal(ctx, strings.NewReader(tt.spec)) + require.NoError(t, err) + + type result struct { + resolveErrs []error + err error + } + done := make(chan result, 1) + go func() { + resolveErrs, err := doc.ResolveAllReferences(ctx, ResolveAllOptions{ + OpenAPILocation: "test.yaml", + DisableExternalRefs: true, + }) + done <- result{resolveErrs: resolveErrs, err: err} + }() + + select { + case got := <-done: + assert.True(t, got.err != nil || len(got.resolveErrs) > 0, + "a pointer that names nothing should report an error") + case <-time.After(30 * time.Second): + t.Fatal("ResolveAllReferences deadlocked resolving a reference whose pointer traverses itself") + } + }) + } +} From d02ba76048555eaaa85636a5499cbbd2aac77fea Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 2 Aug 2026 10:21:24 +0300 Subject: [PATCH 2/5] fix: stop GetObject recursing through a cyclic resolution chain Releasing the reference lock keeps resolution from deadlocking, but it does not change what gets published. A $ref whose pointer names a reference already in the chain still leaves that reference holding a resolution cache that points back into the chain, and GetObject followed it by recursing into the next reference's GetObject. Walking a cycle that way exhausts the goroutine stack and aborts the process. Two shapes reach it. A reference can resolve to itself, and two references can resolve to each other -- the tracker only reports the cycle on the hop after both caches are published, so neither one is a self-reference. A pointer-identity check against the reference being resolved would catch the first and miss the second. Walk the chain iteratively instead, tracking what has been seen and returning nil once it repeats. That covers a cycle of any length, and it leaves the resolution errors and the published cache exactly as they were. Also skip the parent links when a reference resolves to itself, so GetParent and GetTopLevelParent do not loop for anyone walking them. The existing cases now assert the specific error they produce rather than just that one occurred, and every case checks GetObject afterwards, which is where the crash actually lived. --- openapi/reference.go | 45 +++++++++-- openapi/reference_mutex_test.go | 132 +++++++++++++++++++++++++++++--- 2 files changed, 158 insertions(+), 19 deletions(-) diff --git a/openapi/reference.go b/openapi/reference.go index 15cc348f..31dabbcf 100644 --- a/openapi/reference.go +++ b/openapi/reference.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "sync" @@ -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 (r.referenceResolutionCache != nil && r.referenceResolutionCache.Object != nil) || r.circularErrorFound { - if r.referenceResolutionCache != nil && r.referenceResolutionCache.Object != nil { - return r.referenceResolutionCache.Object.GetObject() + if slices.Contains(seen, current) { + return nil } + seen = append(seen, current) + + current.ensureMutex() + current.cacheMutex.RLock() + cache := current.referenceResolutionCache + current.cacheMutex.RUnlock() + + if cache == nil { + return nil + } + current = cache.Object } + return nil } @@ -649,8 +673,13 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co } else { topLevel = ref } - nextRef.SetParent(ref) - nextRef.SetTopLevelParent(topLevel) + // A reference can resolve to itself, in which case parenting it to + // itself would make GetParent/GetTopLevelParent loop for anyone walking + // the chain. The recursive call below reports it as circular. + if nextRef != ref { + 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 diff --git a/openapi/reference_mutex_test.go b/openapi/reference_mutex_test.go index 97b6928d..428fe347 100644 --- a/openapi/reference_mutex_test.go +++ b/openapi/reference_mutex_test.go @@ -69,15 +69,20 @@ func TestEnsureMutex_CopiedReference(t *testing.T) { // reentrant, so a pointer whose prefix named the reference being resolved // blocked forever on a lock its own goroutine held. // -// Each case below is a document that hung before the fix. The expected result -// is an unresolved-reference error, which is what every other pointer that -// names nothing already produced. +// Each case below is a document that hung before the fix. +// +// Resolution is only half of it. A pointer that names a reference already in +// the chain also leaves that reference's resolution cache pointing back into +// the chain, so every case asserts GetObject afterwards: walking a cycle there +// exhausts the goroutine stack, which aborts the test binary outright rather +// than failing a single case. func TestResolveAllReferences_PointerTraversingItsOwnReference(t *testing.T) { t.Parallel() tests := []struct { - name string - spec string + name string + spec string + expectedErr string }{ { name: "prefix names the reference being resolved", @@ -86,6 +91,7 @@ info: {title: t, version: "1"} paths: /a: {$ref: '#/paths/~1a/t'} `, + expectedErr: "unresolved reference", }, { name: "prefix names the reference and the pointer resolves", @@ -96,6 +102,7 @@ paths: $ref: '#/paths/~1a/get' get: {operationId: a, responses: {"200": {description: ok}}} `, + expectedErr: "unresolved reference", }, { name: "prefix reaches the in-flight reference through a resolved one", @@ -105,6 +112,7 @@ paths: /a: {$ref: '#/paths/~1b'} /b: {$ref: '#/paths/~1a/t'} `, + expectedErr: "unresolved reference", }, { name: "components spelling", @@ -116,6 +124,7 @@ components: pathItems: A: {$ref: '#/components/pathItems/A/t'} `, + expectedErr: "unresolved reference", }, { name: "webhooks spelling", @@ -125,13 +134,13 @@ paths: {} webhooks: onA: {$ref: '#/webhooks/onA/t'} `, + expectedErr: "unresolved reference", }, { // GetJSONPointer trims the pointer, so this names /a and the - // reference resolves to itself. Before the fix the resolution cache - // pointed at its own reference and GetObject's delegation at the end - // of the cache-hit branch recursed until the stack was exhausted, - // taking the process with it rather than failing the test. + // reference resolves to itself. The tracker reports that, but the + // reference is left holding a resolution cache that points at + // itself, so it is GetObject below that this case guards. name: "reference resolving to itself via a trimmed pointer", spec: `openapi: 3.1.0 info: {title: t, version: "1"} @@ -140,6 +149,25 @@ paths: $ref: '#/paths/~1a ' get: {operationId: a, responses: {"200": {description: ok}}} `, + expectedErr: "circular reference detected: test.yaml#/paths/~1a -> test.yaml#/paths/~1a", + }, + { + // Same shape one hop wider: /a's cache points at /b and /b's points + // back at /a. The tracker only notices on the third hop, by which + // point both caches are published, so neither reference is a + // self-reference and the cycle only shows up when walking them. + name: "two references resolving to each other via trimmed pointers", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: + $ref: '#/paths/~1b ' + get: {operationId: a, responses: {"200": {description: ok}}} + /b: + $ref: '#/paths/~1a ' + get: {operationId: b, responses: {"200": {description: ok}}} +`, + expectedErr: "circular reference detected: test.yaml#/paths/~1b -> test.yaml#/paths/~1a -> test.yaml#/paths/~1b", }, } @@ -166,11 +194,93 @@ paths: select { case got := <-done: - assert.True(t, got.err != nil || len(got.resolveErrs) > 0, - "a pointer that names nothing should report an error") + require.Error(t, got.err) + assert.Contains(t, got.err.Error(), tt.expectedErr) + assert.Empty(t, got.resolveErrs) case <-time.After(30 * time.Second): t.Fatal("ResolveAllReferences deadlocked resolving a reference whose pointer traverses itself") } + + // None of these resolved, so none of them have an object. Reaching + // that verdict must not walk a cycle. + for path, pathItem := range doc.Paths.All() { + assert.Nil(t, pathItem.GetObject(), "path %s should have no resolved object", path) + } + for name, webhook := range doc.Webhooks.All() { + assert.Nil(t, webhook.GetObject(), "webhook %s should have no resolved object", name) + } + }) + } +} + +// TestGetObject_ChainWalking covers the two ends of GetObject's chain walk: a +// chain of references that terminates has to be followed all the way to the +// object, and one that does not terminate has to give up. +func TestGetObject_ChainWalking(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + spec string + expectedOp string + expectedErr string + }{ + { + name: "multi-hop chain reaches the object", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: {$ref: '#/components/pathItems/A'} +components: + pathItems: + A: {$ref: '#/components/pathItems/B'} + B: {$ref: '#/components/pathItems/C'} + C: {get: {operationId: c, responses: {"200": {description: ok}}}} +`, + expectedOp: "c", + }, + { + name: "circular chain reports no object", + spec: `openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: {$ref: '#/components/pathItems/A'} +components: + pathItems: + A: {$ref: '#/components/pathItems/B'} + B: {$ref: '#/components/pathItems/A'} +`, + expectedErr: "circular reference detected", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + doc, _, err := Unmarshal(ctx, strings.NewReader(tt.spec)) + require.NoError(t, err) + + _, err = doc.ResolveAllReferences(ctx, ResolveAllOptions{ + OpenAPILocation: "test.yaml", + DisableExternalRefs: true, + }) + + pathItem, ok := doc.Paths.Get("/a") + require.True(t, ok) + + if tt.expectedErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedErr) + assert.Nil(t, pathItem.GetObject()) + return + } + + require.NoError(t, err) + obj := pathItem.GetObject() + require.NotNil(t, obj) + assert.Equal(t, tt.expectedOp, obj.Get().GetOperationID()) }) } } From 1fe8ffcc607bc754424a8a6cd8e158d83ec1fc07 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 3 Aug 2026 05:01:28 +0300 Subject: [PATCH 3/5] fix: keep parent links out of a cyclic resolution chain The previous guard only covered a reference that resolved to itself. With /a and /b resolving to each other, the second hop has ref == b and nextRef == a, so it passed the guard and published a.parent = b alongside b.parent = a. GetObject no longer walks that, but GetParent and GetTopLevelParent are public and a caller walking them still loops. The absolute references the tracker already collects cannot answer this: a pointer can name a reference that is already in the chain, so one reference value can appear under two different absolute references. Track the reference values alongside them and skip the links for any value the chain has already been through. The cycle is still reported by the recursive call, so nothing that resolves cleanly loses its links. The two-reference case now asserts that both the parent chain and the top-level parent terminate. --- openapi/reference.go | 29 +++++++++++++++++++++-------- openapi/reference_mutex_test.go | 22 +++++++++++++++++++++- openapi/reference_resolve_test.go | 2 +- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/openapi/reference.go b/openapi/reference.go index 31dabbcf..12e1718e 100644 --- a/openapi/reference.go +++ b/openapi/reference.go @@ -235,7 +235,7 @@ func (r *Reference[T, V, C]) Resolve(ctx context.Context, opts ResolveOptions) ( DisableExternalRefs: opts.DisableExternalRefs, VirtualFS: opts.VirtualFS, HTTPClient: opts.HTTPClient, - }, []string{}) + }, []string{}, nil) } // IsReference returns true if the reference is a reference (via $ref) to an object as opposed to an inline object. @@ -615,8 +615,15 @@ func (r *Reference[T, V, C]) resolve(ctx context.Context, opts references.Resolv } } -// resolveObjectWithTracking recursively resolves references while tracking visited references to detect cycles -func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.CoreModeler](ctx context.Context, ref *Reference[T, V, C], opts references.ResolveOptions, referenceChain []string) ([]error, error) { +// resolveObjectWithTracking recursively resolves references while tracking visited references to detect cycles. +// +// referenceChain holds the absolute reference of every hop so far and is what +// reports a circular reference. resolvedChain holds the reference values behind +// those hops, which the absolute references cannot stand in for: a pointer can +// name a reference that is already in the chain, so the same value can appear +// under two different absolute references. Parent links are only safe to set +// for a value that is not already in the chain. +func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.CoreModeler](ctx context.Context, ref *Reference[T, V, C], opts references.ResolveOptions, referenceChain []string, resolvedChain []*Reference[T, V, C]) ([]error, error) { // If this is not a reference, return the inline object if !ref.IsReference() { return nil, nil @@ -651,6 +658,9 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co newChain := referenceChain newChain = append(newChain, absRef) + newResolvedChain := resolvedChain + newResolvedChain = append(newResolvedChain, ref) + // Resolve the current reference obj, nextRef, validationErrs, err := ref.resolve(ctx, opts) if err != nil { @@ -673,10 +683,13 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co } else { topLevel = ref } - // A reference can resolve to itself, in which case parenting it to - // itself would make GetParent/GetTopLevelParent loop for anyone walking - // the chain. The recursive call below reports it as circular. - if nextRef != ref { + // Only link a reference the chain has not already been through. A + // pointer can name a reference that is already resolving (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. The recursive call below reports + // the cycle; leaving the links alone keeps them walkable meanwhile. + if !slices.Contains(newResolvedChain, nextRef) { nextRef.SetParent(ref) nextRef.SetTopLevelParent(topLevel) } @@ -691,7 +704,7 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co opts.TargetDocument = targetDoc opts.TargetLocation = targetLoc - return resolveObjectWithTracking(ctx, nextRef, opts, newChain) + return resolveObjectWithTracking(ctx, nextRef, opts, newChain, newResolvedChain) } return validationErrs, fmt.Errorf("unable to resolve reference: %s", ref.GetReference()) diff --git a/openapi/reference_mutex_test.go b/openapi/reference_mutex_test.go index 428fe347..06608692 100644 --- a/openapi/reference_mutex_test.go +++ b/openapi/reference_mutex_test.go @@ -202,17 +202,37 @@ paths: } // None of these resolved, so none of them have an object. Reaching - // that verdict must not walk a cycle. + // that verdict must not walk a cycle, and neither must walking the + // parent links the failed resolution left behind. for path, pathItem := range doc.Paths.All() { assert.Nil(t, pathItem.GetObject(), "path %s should have no resolved object", path) + assertParentLinksTerminate(t, path, pathItem) } for name, webhook := range doc.Webhooks.All() { assert.Nil(t, webhook.GetObject(), "webhook %s should have no resolved object", name) + assertParentLinksTerminate(t, name, webhook) } }) } } +// assertParentLinksTerminate walks the parent links a resolution attempt left +// on ref and fails if they lead back to a reference already walked. A failed +// resolution still publishes links, and callers reach them through the public +// GetParent and GetTopLevelParent. +func assertParentLinksTerminate(t *testing.T, label string, ref *ReferencedPathItem) { + t.Helper() + + seen := map[*ReferencedPathItem]bool{} + for current := ref; current != nil; current = current.GetParent() { + require.False(t, seen[current], "%s: parent links cycle", label) + seen[current] = true + } + + // The top-level parent is the head of the chain, never the reference itself. + assert.NotSame(t, ref, ref.GetTopLevelParent(), "%s: top-level parent points at itself", label) +} + // TestGetObject_ChainWalking covers the two ends of GetObject's chain walk: a // chain of references that terminates has to be followed all the way to the // object, and one that does not terminate has to give up. diff --git a/openapi/reference_resolve_test.go b/openapi/reference_resolve_test.go index 92f74f81..cd525597 100644 --- a/openapi/reference_resolve_test.go +++ b/openapi/reference_resolve_test.go @@ -408,7 +408,7 @@ func TestResolveObjectWithTracking_CircularReference(t *testing.T) { TargetLocation: "/test.yaml", RootDocument: &OpenAPI{}, // Empty document for this test TargetDocument: &OpenAPI{}, // Empty document for this test - }, referenceChain) + }, referenceChain, nil) require.Error(t, err) assert.Contains(t, err.Error(), "circular reference detected") From 9f9c8a6af497bc9fff19732b03298f4f8b96771f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 3 Aug 2026 05:23:58 +0300 Subject: [PATCH 4/5] refactor: flatten the double-check in Reference.resolve The deferred unlock inside the conditional, paired with a bare unlock on the branch below it, was correct but easy to break: every future edit to the block has to reason about which of the two paths releases the lock. Read the cache under the lock, release it, then branch on the copy. Same behaviour, one unlock, and it matches how the read-lock check above it already reads. --- openapi/reference.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/openapi/reference.go b/openapi/reference.go index 12e1718e..e5f96a3f 100644 --- a/openapi/reference.go +++ b/openapi/reference.go @@ -557,25 +557,26 @@ func (r *Reference[T, V, C]) resolve(ctx context.Context, opts references.Resolv } r.cacheMutex.RUnlock() - // Need to resolve. Take the write lock only for the double-check, then - // release it: references.Resolve navigates the document and calls GetObject - // on references it traverses, which takes a read lock. sync.RWMutex is not - // reentrant, so holding the write lock across that call self-deadlocks when - // the traversal reaches this reference (directly or via a cache forward). + // 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() + cache := r.referenceResolutionCache + cachedErrs := r.validationErrsCache + r.cacheMutex.Unlock() - // Double-check after acquiring write lock - if r.referenceResolutionCache != nil { - defer r.cacheMutex.Unlock() - 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 } - r.cacheMutex.Unlock() - rootDoc, ok := opts.RootDocument.(*OpenAPI) if !ok { return nil, nil, nil, fmt.Errorf("root document must be *OpenAPI, got %T", opts.RootDocument) From f58f208b58a4e286b91d3948d7359c207cba28c3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 3 Aug 2026 06:25:38 +0300 Subject: [PATCH 5/5] fix: derive parent-link safety from the links, not the current call The per-call chain only knew about hops made by one Resolve. Each member of a cycle can be resolved by its own call: resolving /a leaves b.parent = a, and resolving /b afterwards starts a fresh chain, sees /a as new, and adds a.parent = b on top of it. ResolveAllReferences never hit this because it skips references already marked resolved, but Reference.Resolve has no such guard. Ask the links instead. A reference is unsafe to parent to another when it is already reachable from it, which holds across calls because the links are the record that survives them. That subsumes what the chain covered, so the extra parameter goes away and the signature returns to what it was. TestResolve_SeparateCallsOverCycle resolves both members of a cycle in separate calls and walks their links; without the guard it fails on /a: parent links cycle. --- openapi/reference.go | 61 ++++++++++++++++++++----------- openapi/reference_mutex_test.go | 53 +++++++++++++++++++++++++++ openapi/reference_resolve_test.go | 2 +- 3 files changed, 94 insertions(+), 22 deletions(-) diff --git a/openapi/reference.go b/openapi/reference.go index e5f96a3f..cd1891a5 100644 --- a/openapi/reference.go +++ b/openapi/reference.go @@ -235,7 +235,7 @@ func (r *Reference[T, V, C]) Resolve(ctx context.Context, opts ResolveOptions) ( DisableExternalRefs: opts.DisableExternalRefs, VirtualFS: opts.VirtualFS, HTTPClient: opts.HTTPClient, - }, []string{}, nil) + }, []string{}) } // IsReference returns true if the reference is a reference (via $ref) to an object as opposed to an inline object. @@ -616,15 +616,8 @@ func (r *Reference[T, V, C]) resolve(ctx context.Context, opts references.Resolv } } -// resolveObjectWithTracking recursively resolves references while tracking visited references to detect cycles. -// -// referenceChain holds the absolute reference of every hop so far and is what -// reports a circular reference. resolvedChain holds the reference values behind -// those hops, which the absolute references cannot stand in for: a pointer can -// name a reference that is already in the chain, so the same value can appear -// under two different absolute references. Parent links are only safe to set -// for a value that is not already in the chain. -func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.CoreModeler](ctx context.Context, ref *Reference[T, V, C], opts references.ResolveOptions, referenceChain []string, resolvedChain []*Reference[T, V, C]) ([]error, error) { +// resolveObjectWithTracking recursively resolves references while tracking visited references to detect cycles +func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.CoreModeler](ctx context.Context, ref *Reference[T, V, C], opts references.ResolveOptions, referenceChain []string) ([]error, error) { // If this is not a reference, return the inline object if !ref.IsReference() { return nil, nil @@ -659,9 +652,6 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co newChain := referenceChain newChain = append(newChain, absRef) - newResolvedChain := resolvedChain - newResolvedChain = append(newResolvedChain, ref) - // Resolve the current reference obj, nextRef, validationErrs, err := ref.resolve(ctx, opts) if err != nil { @@ -684,13 +674,19 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co } else { topLevel = ref } - // Only link a reference the chain has not already been through. A - // pointer can name a reference that is already resolving (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. The recursive call below reports - // the cycle; leaving the links alone keeps them walkable meanwhile. - if !slices.Contains(newResolvedChain, nextRef) { + // 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) } @@ -705,12 +701,35 @@ func resolveObjectWithTracking[T any, V interfaces.Validator[T], C marshaller.Co opts.TargetDocument = targetDoc opts.TargetLocation = targetLoc - return resolveObjectWithTracking(ctx, nextRef, opts, newChain, newResolvedChain) + return resolveObjectWithTracking(ctx, nextRef, opts, newChain) } 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 { diff --git a/openapi/reference_mutex_test.go b/openapi/reference_mutex_test.go index 06608692..8dddf99e 100644 --- a/openapi/reference_mutex_test.go +++ b/openapi/reference_mutex_test.go @@ -233,6 +233,59 @@ func assertParentLinksTerminate(t *testing.T, label string, ref *ReferencedPathI assert.NotSame(t, ref, ref.GetTopLevelParent(), "%s: top-level parent points at itself", label) } +// TestResolve_SeparateCallsOverCycle covers the parent links when each member of +// a cycle is resolved by its own call to the public Resolve. +// +// ResolveAllReferences walks the document once and skips references already +// marked resolved, so it never revisits the second member. Resolve has no such +// guard: it starts a fresh chain every time, and the links from the earlier call +// are the only record that the two references already descend from each other. +func TestResolve_SeparateCallsOverCycle(t *testing.T) { + t.Parallel() + + ctx := t.Context() + doc, _, err := Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0 +info: {title: t, version: "1"} +paths: + /a: + $ref: '#/paths/~1b ' + get: {operationId: a, responses: {"200": {description: ok}}} + /b: + $ref: '#/paths/~1a ' + get: {operationId: b, responses: {"200": {description: ok}}} +`)) + require.NoError(t, err) + + pathA, ok := doc.Paths.Get("/a") + require.True(t, ok) + pathB, ok := doc.Paths.Get("/b") + require.True(t, ok) + + opts := ResolveOptions{ + RootDocument: doc, + TargetDocument: doc, + TargetLocation: "test.yaml", + DisableExternalRefs: true, + } + + // Both report the cycle; it is what they leave behind that matters. + _, err = pathA.Resolve(ctx, opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "circular reference detected") + + _, err = pathB.Resolve(ctx, opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "circular reference detected") + + assertParentLinksTerminate(t, "/a", pathA) + assertParentLinksTerminate(t, "/b", pathB) + + // The second call must not parent /a under its own descendant. + assert.NotSame(t, pathB, pathA.GetParent(), "/a should not be parented under /b") + assert.Nil(t, pathA.GetObject(), "/a should have no resolved object") + assert.Nil(t, pathB.GetObject(), "/b should have no resolved object") +} + // TestGetObject_ChainWalking covers the two ends of GetObject's chain walk: a // chain of references that terminates has to be followed all the way to the // object, and one that does not terminate has to give up. diff --git a/openapi/reference_resolve_test.go b/openapi/reference_resolve_test.go index cd525597..92f74f81 100644 --- a/openapi/reference_resolve_test.go +++ b/openapi/reference_resolve_test.go @@ -408,7 +408,7 @@ func TestResolveObjectWithTracking_CircularReference(t *testing.T) { TargetLocation: "/test.yaml", RootDocument: &OpenAPI{}, // Empty document for this test TargetDocument: &OpenAPI{}, // Empty document for this test - }, referenceChain, nil) + }, referenceChain) require.Error(t, err) assert.Contains(t, err.Error(), "circular reference detected")