Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: potential nil ctx on connEx.close() #51467

Open
andreimatei opened this issue Jul 15, 2020 · 3 comments
Open

sql: potential nil ctx on connEx.close() #51467

andreimatei opened this issue Jul 15, 2020 · 3 comments
Labels
A-sql-executor SQL txn logic C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team
Projects

Comments

@andreimatei
Copy link
Contributor

andreimatei commented Jul 15, 2020

Spin off from #51460.

I think there's a bug in the connExecutor closing code. I think it can lead to crashes, although I'm not sure.

Look at what this event does

Next: stateAborted{},
Action: cleanupAndFinishOnError,

This event is generated here when the connExecutor is closing (i.e. when the pgwire connection is terminated). This event leaves the connExecutor in stateAborted but also clears txnState.Ctx (through cleanupAndFinishOnError(). That's not kosher, since connEx.Ctx() expects there to be a non-nil txnState.Ctx whenever the state is not stateNoTxn. Even though the executor is closing, I don't think this is benign. I think we might try to use that nil ctx and crash.

Jira issue: CRDB-4042

@andreimatei andreimatei added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 15, 2020
@blathers-crl

This comment has been minimized.

@andreimatei
Copy link
Contributor Author

Another case where the same bug might be triggered is here. If preparing a COMMIT fails, and we're already in the aborted state, we'd generate an eventNonRetriableErr{IsCommit: fsm.True}. That's not what we want here - we want an eventNonRetriableErr{IsCommit: fsm.False}.
I don't know if preparing a COMMIT is actually possible. I think PG doesn't allow it; I don't know if we protect against it in any way. In any case, we should fix this.

@jordanlewis jordanlewis moved this from Triage to 20.2 Stability Period in BACKLOG, NO NEW ISSUES: SQL Execution Jul 21, 2020
@asubiotto asubiotto moved this from 20.2 Stability Period to 20.2 F in BACKLOG, NO NEW ISSUES: SQL Execution Sep 1, 2020
@asubiotto asubiotto moved this from 20.2 F to [GENERAL BACKLOG] Enhancements/Features/Investigations in BACKLOG, NO NEW ISSUES: SQL Execution Oct 7, 2020
@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
@yuzefovich yuzefovich removed their assignment May 31, 2022
@yuzefovich yuzefovich added this to Triage in SQL Queries via automation May 31, 2022
@yuzefovich yuzefovich removed this from [GENERAL BACKLOG] Enhancements/Features/Investigations in BACKLOG, NO NEW ISSUES: SQL Execution May 31, 2022
@yuzefovich yuzefovich moved this from Triage to Backlog in SQL Queries May 31, 2022
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-executor SQL txn logic C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team
Projects
Status: Bugs to Fix
SQL Queries
Backlog (DO NOT ADD NEW ISSUES)
Development

No branches or pull requests

3 participants