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
@vikrantpuppala vikrantpuppala mentioned this pull request Jul 13, 2026
3 tasks
vikrantpuppala added a commit that referenced this pull request Jul 13, 2026
## Summary
Bump `DriverVersion` to `1.14.0` and add the `v1.14.0` section to
`CHANGELOG.md`.

### Notable changes since v1.13.1
- **Minimum Go version is now 1.25.0** (previously 1.20): the `go`
directive was raised while clearing OSV-Scanner findings and updating
dependencies. Consumers building with an older toolchain will need to
upgrade Go (#368)
- Fix panic in `InitThriftClient` when the endpoint URL is malformed
(#394)
- Stop using `html/template` in the U2M OAuth callback page to restore
linker dead-code elimination (fixes #343)
- Fix DECIMAL precision loss inside complex types (STRUCT/ARRAY/MAP)
(fixes #253)

## Test plan
- [x] `go test ./... -count=1 -short` passes locally (Go 1.26.4)
- [x] Multi-DBR suites in `universe/peco/correctness/go` passed (5/5 on
DBR `18.x-photon-scala2.13`)
- [ ] CI green

This pull request was AI-assisted by Isaac.

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
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