Axisflow opened a new pull request, #174:
URL: https://github.com/apache/iotdb-client-go/pull/174
### What
Guard the optional `TSStatus.Message` in `verifySuccesses`
(`client/utils.go`) so a
failing sub-status without a message no longer panics inside the client.
### Problem
`TSStatus.Message` is optional in the Thrift IDL, so a server may return a
failing
sub-status carrying only a code. `verifySuccesses` dereferences it
unconditionally:
```go
// client/utils.go
func verifySuccesses(statuses []*common.TSStatus) error {
buff := bytes.Buffer{}
for _, status := range statuses {
if status.Code != SuccessStatus && status.Code !=
RedirectionRecommend {
buff.WriteString(*status.Message + ";") // panics
when Message is nil
}
}
...
```
So a `MULTIPLE_ERROR` response whose sub-statuses have no messages crashes
the
caller with `invalid memory address or nil pointer dereference`, inside the
client
rather than as a returned error. Every `Insert*` path reaches this through
`VerifySuccess`, so a batch insert is enough to trigger it.
`VerifySuccess`, a few lines below in the same file, already guards the
envelope
message the same way:
```go
if status.Code != SuccessStatus {
msg := ""
if status.Message != nil {
msg = *status.Message
}
return &ExecutionError{Code: status.Code, Message: msg}
}
```
which is what makes the missing guard in `verifySuccesses` look like an
oversight
rather than a deliberate assumption.
Present on `main`, on `dev/1.3`, and in the released `v1.3.7` and `v2.0.8`.
### Fix
Fall back to the status code when no message is supplied, so the returned
`BatchError` still identifies which sub-status failed instead of
contributing an
empty entry:
```go
if status.Message != nil {
buff.WriteString(*status.Message)
buff.WriteString(";")
} else {
fmt.Fprintf(&buff, "error code: %d;",
status.Code)
}
```
`BatchError.GetStatuses()` is unchanged, so callers that inspect the
per-tablet
statuses themselves are unaffected.
### Verification
Reproduced and verified locally against a `MULTIPLE_ERROR` whose failing
sub-status carries only a code:
- without this change, `VerifySuccess` panics with
`runtime error: invalid memory address or nil pointer dereference`
- with it, the call returns a `*BatchError` whose message names the failing
code
and whose `GetStatuses()` still holds the full sub-status slice
`go build ./...`, `go vet ./client/` and `go test ./client/` pass. `make
generate`
was not run locally (it needs the thrift toolchain); this change does not
touch the
generated code.
No test is included in this PR to keep it to the one-line guard. I have the
reproducing test (a `MULTIPLE_ERROR` with a message-less sub-status, which
panics
without the fix) and am happy to add it here or in a follow-up if you would
like it
in the suite.
### Please also cherry-pick to `dev/1.3`
The same line is present on `dev/1.3` and shipped in `v1.3.7`, so a fix that
lands
only on `main` will not reach users on the 1.3 line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
