Skip to content

a malformed BaseURI can lead to unrecoverable panics #578

@jhump

Description

@jhump

Bug source: normalizeRemoteURL unconditionally overwrites the ref's scheme with the BaseURL's, even when BaseURL has an empty scheme. The downstream early-return at rolodex_remote_loader.go:588-591 should either fall back to the RemoteURLHandler or return an error rather than (nil, nil). Because of this (nil, nil) return, Rolodex.openRemoteLocation returns a nil reader to consumeAdaptedFile, which crashes at io.ReadAll(nil).

In some call chains, this can happen synchronously, from the goroutine that calls document.BuildV3Model. This is not ideal, but at least the caller could work-around potential issues like this by recovering from a panic.

However, there are other call chains where it happens from a separate goroutine related to the use of singleflight.Group.Do to de-duplicate fetches. In this case, there is nothing the calling program can do to recover from the panic.

While a malformed BaseURL could be viewed as user-error, it's bad when it causes unrecoverable panics in the calling program. The BaseURL could be eagerly validated and have the operation fail fast with a reasonable error when it is malformed. But having normalizeRemoteURL do something different is also appropriate IMO (such as only conditionally overwrite the ref's scheme if the BaseURL has a schema, or have it call the RemoteURLHandler even if the scheme is empty, or have it produce an error due to the missing scheme).

Here are example call chains for both paths:

# Synchronous/single goroutine
document.BuildV3Model
    └─ v3.CreateDocumentFromConfig
        └─ v3.createDocument                                 (low/v3/create_document.go:287)
            └─ Rolodex.CheckForCircularReferences            (index/rolodex.go:561)
                └─ Resolver.CheckForCircularReferences       (index/resolver_entry.go:199)
                    └─ visitIndexWithoutDamagingIt           (index/resolver_visit.go:22)
                        └─ Resolver.VisitReference           (index/resolver_visit.go:162)
                            └─ Resolver.collectReferenceRelatives (index/resolver_mutation.go:36)
                                └─ Resolver.extractRelatives  (...resolver_relatives.go:27)
                                    └─ ... extractRelativesWithState
                                        └─ Resolver.extractRelativeReference (resolver_relatives.go:150)
                                            └─ Resolver.searchReferenceWithContext (resolver_visit.go:113)
                                                └─ SpecIndex.SearchIndexForReferenceByReferenceWithContext (search_index.go:278)
                                                    └─ Rolodex.OpenWithContext  (rolodex.go:720)
                                                        └─ Rolodex.openRemoteLocation (rolodex.go:828)  ← returns nil reader
                                                            └─ Rolodex.asRemoteFile (rolodex.go:842)
                                                                └─ consumeAdaptedFile (rolodex.go:865)
                                                                    └─ io.ReadAll(nil)  ← PANIC
# Unrecoverable/child goroutine
SpecIndex.ExtractComponentsFromRefs               (index/extract_refs_lookup.go:108)
    └─ go func1                                     (extract_refs_lookup.go:113)  ← spawned
        └─ singleflight.Group.Do                    (singleflight.go:113)
            └─ singleflight.Group.doCall            (singleflight.go:200)
                └─ goroutine, doCall.func2          (singleflight.go:198)        ← second goroutine
                    └─ ExtractComponentsFromRefs.func1.2 (extract_refs_lookup.go:121)
                        └─ SpecIndex.locateRef       (extract_refs_lookup.go:205)
                            └─ SpecIndex.FindComponent (find_component_entry.go:50)
                                └─ SpecIndex.lookupRolodex (find_component_external.go:44)
                                    └─ Rolodex.OpenWithContext (rolodex.go:720)
                                        └─ Rolodex.openRemoteLocation (rolodex.go:828)  ← same nil-reader bug
                                            └─ Rolodex.asRemoteFile (rolodex.go:842)
                                                └─ consumeAdaptedFile (rolodex.go:865)
                                                    └─ io.ReadAll(nil)  ← PANIC, on worker goroutine

singleflight.doCall.func1 (line 170) installs its own defer recover(), but it converts the recovered value into a *panicError and re-panics on a fresh goroutine at singleflight.go:205. That re-panic propagates to the Go runtime with no caller in scope to recover it — the program exits.

The simplest way to trigger the differing behaviors above appears to be by setting ExtractRefsSequentially to true or false. Though when we encountered the synchronous approach initially, we didn't change that value but instead were testing with the Square API and its spec seems to be just-right-shaped to somehow avoid the panic when extracting component refs only to have it blow up from the calling goroutine when checking for cycles.

Versions confirmed: github.com/pb33f/libopenapi v0.37.0, golang.org/x/sync v0.20.0, Go 1.26.3 (darwin/arm64).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions