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/parse/parse.go is littered by invalid string splicing #57347

Closed
knz opened this issue Dec 2, 2020 · 1 comment · Fixed by #57354
Closed

sql/parse/parse.go is littered by invalid string splicing #57347

knz opened this issue Dec 2, 2020 · 1 comment · Fixed by #57354
Labels
A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@knz
Copy link
Contributor

knz commented Dec 2, 2020

We see code like this:

  stmt, ⊙ ≔ ParseOne(fmt.Sprintf("ALTER TABLE %s RENAME TO x", sql))
  stmt, ⊙ ≔ ParseOne(fmt.Sprintf("SELECT fakeprefix.%s", sql))
  expr, ⊙ ≔ ParseExpr(fmt.Sprintf("1::%s", sql))

This improperly expands the SQL name as a string, and misses escaping when the name contains special characters.

There may be other occurrences elsewhere in the source tree.

Example resulting bug, found in the wild: #57187

A general fix for this would be to apply the technique that @petermattis designed for the CC code here: https://github.com/cockroachlabs/managed-service/pull/4223

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect labels Dec 2, 2020
@knz knz added this to Triage in SQL Sessions - Deprecated via automation Dec 2, 2020
@petermattis
Copy link
Collaborator

I noticed recently that pkg/sql/delegate contains a lot of manually constructed SQL. The instances I looked at looked safe at first glance, but I'd feel better if we had a more principled approach to SQL string construction.

@craig craig bot closed this as completed in a1759d1 Dec 4, 2020
SQL Sessions - Deprecated automation moved this from Triage to Done Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants