cockroachdb / cockroachdb/cockroach

decode_plan_gist nil deref in explain.Emit (nil root / Ordering)

Open
#169,754 2 comments 0 reactions 0 assignees View on GitHub
C-bug O-community T-sql-queries X-blathers-triaged
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

## Describe the problem

Calling `crdb_internal.decode_external_plan_gist` / `crdb_internal.decode_plan_gist` with specific **invalid** base64 gist payloads can trigger:

```text
ERROR: internal error: runtime error: invalid memory address or nil pointer dereference
```

The panic occurs when `(*explain.Node).Ordering()` is invoked on a **nil** receiver. The current implementation of `Ordering()` does not guard against a nil receiver (unlike `Columns()`, which returns nil when `n == nil`), and some call paths during **`Emit`** appear to invoke `Ordering()` without establishing that **`plan.Root` is non-nil**.

**Trigger characterization (hypothesis):** the issue appears when decoding yields a **partially constructed or empty** explain plan — i.e. **`plan.Root == nil`** — while the emission path still reaches code that reads root properties such as **ordering** (see stack: `emit.go` → `EnterNode("root", ...)` / `plan.Root.Ordering()`).

This suggests a **missing invariant**: `Emit` assumes a non-nil root node, which is **not guaranteed** for malformed or adversarial plan gists.

More broadly, invalid gist bytes should yield a **user-facing error**, not an internal error (`XX000`).

**Related prior work:** [#110410](https://github.com/cockroachdb/cockroach/issues/110410), [#109560](https://github.com/cockroachdb/cockroach/issues/109560), [PR #109627](https://github.com/cockroachdb/cockroach/pull/109627) addressed other malformed-gist failures; this report documents a **different stack** (`explain_factory.go` / `emit.go` vs prior index-out-of-bounds / decoder assumptions).

---

## To Reproduce

1. Start CockroachDB (`cockroach start-single-node --insecure` or your cluster).

2. Enable internals access if your build requires it:

```sql
SET allow_unsafe_internals = true;
```

_(Some builds may already allow the builtin without this setting.)_

3. Run:

```sql
SELECT * FROM crdb_internal.decode_external_plan_gist('Ah4GDio=') LIMIT 1;
```

The same gist typically reproduces with:

```sql
SELECT * FROM crdb_internal.decode_plan_gist('Ah4GDio=') LIMIT 1;
```

4. Observe internal error.

Representative frames:

- `pkg/sql/opt/exec/explain/explain_factory.go` — `Ordering()`
- `pkg/sql/opt/exec/explain/emit.go` — `emitInternal` (around `EnterNode("root", ...)`)

**Sample output** (from `cockroach sql -f new_bug/poc_decode_plan_gist_emit_nil_root_ordering_npe.sql` on the Environment build below; client prints the panic hook twice — one copy shown):

```text
SET
ERROR: internal error: runtime error: invalid memory address or nil pointer dereference
SQLSTATE: XX000
DETAIL: stack trace:
pkg/util/errorutil/catch.go:24: shouldCatch()
pkg/util/errorutil/catch.go:38: MaybeCatchPanic()
GOROOT/src/runtime/panic.go:860: gopanic()
GOROOT/src/runtime/panic.go:336: panicmem()
GOROOT/src/runtime/signal_unix.go:931: sigpanic()
pkg/sql/opt/exec/explain/explain_factory.go:76: Ordering()
pkg/sql/opt/exec/explain/emit.go:117: emitInternal()
pkg/sql/opt/exec/explain/emit.go:41: Emit()
pkg/sql/opt/exec/explain/plan_gist_factory.go:191: DecodePlanGistToRows()
pkg/sql/plan_opt.go:1171: DecodeGist()
pkg/sql/sem/builtins/generator_builtins.go:998: Start()
pkg/sql/rowexec/project_set.go:208: nextInputRow()
pkg/sql/rowexec/project_set.go:302: Next()
pkg/sql/colexec/columnarizer.go:244: Next()
pkg/sql/colflow/flow_coordinator.go:302: Run()
pkg/sql/distsql_running.go:1087: Run()
pkg/sql/conn_executor_exec.go:3694: execWithDistSQLEngine()
...
Failed running "sql"
```

---

## Expected behavior

- Malformed or adversarial gist strings should produce a **non-internal** error (e.g. invalid gist / unsupported encoding), consistent with hardening after [#109627](https://github.com/cockroachdb/cockroach/pull/109627).

**Potential fixes** (non-prescriptive; for triage / implementation discussion):

- Ensure **`plan.Root` is non-nil** (or bail out with a clear error) before calling **`Emit`** on decoded gist plans.
- Add **nil checks in `Ordering()`** analogous to **`Columns()`**, if retaining the current call shape is desirable.
- **Reject malformed gists earlier** during decode — before explain node construction / emission — with a regular SQL error instead of `AssertionFailedf` / panic paths surfacing as `XX000`.

---

## Environment

- **CockroachDB:** `v26.3.0-alpha.00000000-dev`, CCL, linux amd64, Go 1.26.2
- **OS:** Linux 4.18.0-553.el8_10.x86_64
- **Client:** `cockroach sql --insecure`

Jira issue: CRDB-63638

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the failure with crdb_internal.decode_external_plan_gist('Ah4GDio=') and trace the path through plan_gist_factory.go, emit.go, and explain_factory.go. Determine where the nil root or Ordering call should be rejected or handled, then verify that malformed gists return a regular user-facing error rather than an internal panic.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.