Skip to content

Commit

Permalink
sql: fix recently introduced bug around the internal rowsIterator
Browse files Browse the repository at this point in the history
This commit fixes a minor bug introduced in c7eb1bf
that could lead to a nil pointer panic. In particular, that commit
introduced `HasResults` method to the `rowsIterator`, and it assumed
that `first` field is always non-nil when the iterator was returned on
`QueryIteratorEx` call. However, that is not true - in case an error was
returned from the connExecutor goroutine, then `rowsIterator.lastErr` is
set while `first` is left nil. The expectation is that in such a case
the user of the iterator will receive that error either on `Next` or
`Close` call, properly cleaning up the iterator. We might want to
rethink this and return the error explicitly, but in the spirit of
making the least amount of changes, this commit simply added the non-nil
check for the `first` field.

Release note (bug fix): CockroachDB could previously encounter a nil
pointer crash when populating data for SQL Activity page in some rare
cases, and this is now fixed. The bug is present in 22.2.9 and 23.1.1
releases.
  • Loading branch information
yuzefovich committed May 17, 2023
1 parent c0a20cd commit 0f36592
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion pkg/sql/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ func (r *rowsIterator) Types() colinfo.ResultColumns {
}

func (r *rowsIterator) HasResults() bool {
return r.first.row != nil
return r.first != nil && r.first.row != nil
}

// QueryBuffered executes the supplied SQL statement and returns the resulting
Expand Down Expand Up @@ -1160,6 +1160,8 @@ func (ie *InternalExecutor) execInternal(
// Note that if a context cancellation error has occurred, we still return
// the iterator and nil retErr so that the iterator is properly closed by
// the caller which will clean up the connExecutor goroutine.
// TODO(yuzefovich): reconsider this and return an error explicitly if
// r.lastErr is non-nil.
return r, nil
}

Expand Down

0 comments on commit 0f36592

Please sign in to comment.