Skip to content

Fix panic in InitThriftClient when endpoint URL is malformed#394

Merged
vikrantpuppala merged 2 commits into
databricks:mainfrom
gretasharoyan:fix/thrift-client-nil-transport-panic
Jul 13, 2026
Merged

Fix panic in InitThriftClient when endpoint URL is malformed#394
vikrantpuppala merged 2 commits into
databricks:mainfrom
gretasharoyan:fix/thrift-client-nil-transport-panic

Conversation

@gretasharoyan

Copy link
Copy Markdown
Contributor

This surfaces as:

interface conversion: thrift.TTransport is nil, not *thrift.THttpClient

Root cause

In internal/client/client.go, the HTTP transport path type-asserts the transport before checking the error returned by thrift.NewTHttpClientWithOptions:

tTrans, err = thrift.NewTHttpClientWithOptions(endpoint, thrift.THttpClientOptions{Client: httpclient})

thriftHttpClient := tTrans.(*thrift.THttpClient)   // panics
...
if err != nil {                                    // never reached on failure
    return nil, dbsqlerrint.NewRequestError(context.TODO(), dbsqlerr.ErrInvalidURL, err)
}

NewTHttpClientWithOptions returns a nil TTransport when url.Parse(endpoint) fails (e.g. a host or HTTP path containing a control character or an invalid %-escape). Asserting a nil TTransport to *thrift.THttpClient panics, so the existing err check below the switch is never reached.

Because the transport is initialized lazily on the first connection (connector.ConnectInitThriftClient), this surfaces as a crash while acquiring a connection rather than a returned error.

Fix

Move the error check to immediately after NewTHttpClientWithOptions, before the type assertion. The now-unreachable post-switch error check is removed; the same ErrInvalidURL error is returned as before, without panicking.

Testing

Added TestInitThriftClientMalformedEndpointDoesNotPanic, which builds a config whose HTTP path passes ToEndpointURL's non-empty validation but fails url.Parse, and asserts InitThriftClient returns an error instead of panicking. Verified the test fails (panics) without the fix and passes with it. go vet and gofmt are clean and the full internal/client package tests pass.

@gretasharoyan gretasharoyan force-pushed the fix/thrift-client-nil-transport-panic branch from 8da3b03 to 1740211 Compare July 10, 2026 21:33
Signed-off-by: Greta Sharoyan <greta@sigmacomputing.com>
@gretasharoyan gretasharoyan force-pushed the fix/thrift-client-nil-transport-panic branch from 1740211 to 51e4fa7 Compare July 10, 2026 21:34
@vikrantpuppala vikrantpuppala merged commit f43ee9c into databricks:main Jul 13, 2026
5 of 9 checks passed
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