Skip to content

Commit 357fc03

Browse files
renecannaorenecannao
committed
fix: harden MySQL user variable parsing
1 parent 36bbafa commit 357fc03

8 files changed

Lines changed: 152 additions & 3 deletions

File tree

‎include/sql_parser/emitter.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,19 @@ class Emitter {
157157
}
158158

159159
void emit_user_variable(const AstNode* node) {
160+
if (mode_ == EmitMode::DIGEST) {
161+
StringRef source = node->source();
162+
if (source.len >= 2 &&
163+
(source.ptr[1] == '\'' || source.ptr[1] == '"' || source.ptr[1] == '`')) {
164+
sb_.append("@?", 2);
165+
return;
166+
}
167+
}
168+
StringRef source = node->source();
169+
if (!source.empty()) {
170+
sb_.append(source.ptr, source.len);
171+
return;
172+
}
160173
sb_.append_char('@');
161174
emit_value(node);
162175
}

‎include/sql_parser/tokenizer.h‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ class Tokenizer {
1616
end_ = input + len;
1717
has_peeked_ = false;
1818
has_error_ = false;
19+
has_fatal_error_ = false;
1920
has_user_variables_ = false;
21+
paren_depth_ = 0;
22+
first_open_paren_ = nullptr;
2023
error_source_ = {};
2124
}
2225

@@ -25,6 +28,7 @@ class Tokenizer {
2528
// from "the input was syntactically invalid" and surface the latter
2629
// as ParseResult::ERROR rather than PARTIAL.
2730
bool has_error() const { return has_error_; }
31+
bool has_fatal_error() const { return has_fatal_error_; }
2832
bool has_user_variables() const { return has_user_variables_; }
2933
StringRef error_source() const { return error_source_; }
3034

@@ -37,6 +41,10 @@ class Tokenizer {
3741
has_error_ = true;
3842
if (error_source_.empty()) error_source_ = source;
3943
}
44+
void flag_fatal_error_at(StringRef source) {
45+
has_fatal_error_ = true;
46+
flag_error_at(source);
47+
}
4048

4149
Token next_token() {
4250
if (has_peeked_) {
@@ -75,7 +83,10 @@ class Tokenizer {
7583
Token peeked_;
7684
bool has_peeked_ = false;
7785
bool has_error_ = false;
86+
bool has_fatal_error_ = false;
7887
bool has_user_variables_ = false;
88+
uint32_t paren_depth_ = 0;
89+
const char* first_open_paren_ = nullptr;
7990
StringRef error_source_;
8091

8192
uint32_t offset() const {
@@ -103,8 +114,14 @@ class Tokenizer {
103114
continue;
104115
}
105116

106-
// -- line comment (MySQL requires space after --, PgSQL doesn't but we handle both)
107-
if (c == '-' && peek_char(1) == '-') {
117+
// PostgreSQL accepts any `--` line comment. MySQL requires the
118+
// second dash to be followed by whitespace or a control byte.
119+
const bool dash_comment = c == '-' && peek_char(1) == '-' &&
120+
(D == Dialect::PostgreSQL ||
121+
(cursor_ + 2 < end_ &&
122+
(static_cast<unsigned char>(peek_char(2)) <= 0x20 ||
123+
static_cast<unsigned char>(peek_char(2)) == 0x7f)));
124+
if (dash_comment) {
108125
cursor_ += 2;
109126
while (cursor_ < end_ && *cursor_ != '\n') ++cursor_;
110127
continue;
@@ -121,6 +138,7 @@ class Tokenizer {
121138

122139
// /* block comment */
123140
if (c == '/' && peek_char(1) == '*') {
141+
const char* comment_start = cursor_;
124142
cursor_ += 2;
125143
if constexpr (D == Dialect::PostgreSQL) {
126144
// PostgreSQL supports nested block comments
@@ -136,15 +154,36 @@ class Tokenizer {
136154
++cursor_;
137155
}
138156
}
157+
if (depth != 0) {
158+
flag_fatal_error_at(StringRef{comment_start,
159+
static_cast<uint32_t>(end_ - comment_start)});
160+
}
139161
} else {
140162
// MySQL: no nesting
163+
const bool executable = cursor_ < end_ && *cursor_ == '!';
164+
bool has_user_variable_marker = false;
165+
bool closed = false;
141166
while (cursor_ < end_) {
142167
if (*cursor_ == '*' && peek_char(1) == '/') {
143168
cursor_ += 2;
169+
closed = true;
144170
break;
145171
}
172+
if (*cursor_ == '@') has_user_variable_marker = true;
146173
++cursor_;
147174
}
175+
if (!closed) {
176+
flag_fatal_error_at(StringRef{comment_start,
177+
static_cast<uint32_t>(end_ - comment_start)});
178+
}
179+
// Versioned comments execute as SQL on MySQL. Until their
180+
// contents are parsed exactly, preserve any possible user
181+
// variable use and force conservative classification.
182+
if (executable && has_user_variable_marker) {
183+
has_user_variables_ = true;
184+
flag_fatal_error_at(StringRef{comment_start,
185+
static_cast<uint32_t>(cursor_ - comment_start)});
186+
}
148187
}
149188
continue;
150189
}
@@ -163,6 +202,15 @@ class Tokenizer {
163202
has_error_ = true;
164203
if (error_source_.empty()) error_source_ = StringRef{source_start, source_len};
165204
}
205+
if (type == TokenType::TK_LPAREN) {
206+
if (paren_depth_ == 0) first_open_paren_ = source_start;
207+
++paren_depth_;
208+
} else if (type == TokenType::TK_RPAREN && paren_depth_ > 0) {
209+
if (--paren_depth_ == 0) first_open_paren_ = nullptr;
210+
} else if (type == TokenType::TK_EOF && paren_depth_ > 0) {
211+
flag_fatal_error_at(StringRef{first_open_paren_,
212+
static_cast<uint32_t>(end_ - first_open_paren_)});
213+
}
166214
if (type == TokenType::TK_USER_VARIABLE) has_user_variables_ = true;
167215
return Token{type, StringRef{text_start, text_len},
168216
StringRef{source_start, source_len},

‎src/sql_parser/parser.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -940,6 +940,7 @@ void Parser<D>::scan_to_end(ParseResult& result) {
940940
if (first.type == TokenType::TK_EOF) {
941941
StringRef error_source = tokenizer_.error_source();
942942
if (!error_source.empty()) {
943+
if (tokenizer_.has_fatal_error()) result.status = ParseResult::ERROR;
943944
result.remaining = StringRef{error_source.ptr,
944945
static_cast<uint32_t>(tokenizer_.input_end() - error_source.ptr)};
945946
} else {
@@ -951,6 +952,13 @@ void Parser<D>::scan_to_end(ParseResult& result) {
951952
if (first.type == TokenType::TK_SEMICOLON) {
952953
Token next = tokenizer_.next_token();
953954
if (next.type == TokenType::TK_EOF) {
955+
StringRef error_source = tokenizer_.error_source();
956+
if (tokenizer_.has_fatal_error() && !error_source.empty()) {
957+
result.status = ParseResult::ERROR;
958+
result.remaining = StringRef{error_source.ptr,
959+
static_cast<uint32_t>(tokenizer_.input_end() - error_source.ptr)};
960+
return;
961+
}
954962
result.full_input = true;
955963
return;
956964
}

‎tests/test_digest.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,20 @@ TEST_F(MySQLDigestTest, SetVariableDigest) {
154154
EXPECT_EQ(d1.hash, d2.hash);
155155
}
156156

157+
TEST_F(MySQLDigestTest, QuotedUserVariablesNormalizeSafelyAndConsistently) {
158+
const char* plain_sql = "SELECT @plain";
159+
EXPECT_EQ(normalized(plain_sql), "SELECT @plain");
160+
EXPECT_EQ(normalized_token(plain_sql), "SELECT @plain");
161+
162+
const char* select_sql = "SELECT @'a-b'";
163+
EXPECT_EQ(normalized(select_sql), "SELECT @?");
164+
EXPECT_EQ(normalized_token(select_sql), "SELECT @?");
165+
166+
const char* set_sql = "SET @`a``b` = 1";
167+
EXPECT_EQ(normalized(set_sql), "SET @? = ?");
168+
EXPECT_EQ(normalized_token(set_sql), "SET @? = ?");
169+
}
170+
157171
// ========== NULL and boolean literals ==========
158172

159173
TEST_F(MySQLDigestTest, NullPreserved) {

‎tests/test_emitter.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,14 @@ TEST_F(MySQLEmitterTest, SetDottedUserVariable) {
9292
EXPECT_EQ(out, "SET @user.var = 7");
9393
}
9494

95+
TEST_F(MySQLEmitterTest, QuotedUserVariableRetainsExactReplaySpelling) {
96+
EXPECT_EQ(round_trip("SELECT @'a-b'"), "SELECT @'a-b'");
97+
EXPECT_EQ(round_trip("SET @`a``b` = 1"), "SET @`a``b` = 1");
98+
}
99+
95100
TEST_F(MySQLEmitterTest, SetScopedCommaItemAfterUserVariable) {
96101
std::string out = round_trip("SET @'mix' := 1, LOCAL wait_timeout := 20");
97-
EXPECT_EQ(out, "SET @mix = 1, LOCAL wait_timeout = 20");
102+
EXPECT_EQ(out, "SET @'mix' = 1, LOCAL wait_timeout = 20");
98103
}
99104

100105
TEST_F(MySQLEmitterTest, SetTransaction) {

‎tests/test_set.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,17 @@ TEST(MySQLSetCompleteness, OnlyEofOrOneTrailingSemicolonIsFullInput) {
5353
}
5454
}
5555

56+
TEST(MySQLSetCompleteness, MissingClosingParenthesisIsAnError) {
57+
Parser<Dialect::MySQL> parser;
58+
const char* cases[] = {"SET @x=(1;", "SET @x=(1,2", "SET @x=(1"};
59+
for (const char* sql : cases) {
60+
SCOPED_TRACE(sql);
61+
ParseResult r = parser.parse(sql, strlen(sql));
62+
EXPECT_EQ(r.status, ParseResult::ERROR);
63+
EXPECT_FALSE(r.full_input);
64+
}
65+
}
66+
5667
// ============================================================================
5768
// Data-driven test infrastructure
5869
// ============================================================================

‎tests/test_tokenizer.cpp‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,38 @@ TEST_F(MySQLTokenizerTest, MalformedLosslessTokensAreErrors) {
181181
}
182182
}
183183

184+
TEST_F(MySQLTokenizerTest, DoubleDashRequiresFollowingWhitespaceOrControl) {
185+
const char* sql = "SELECT 1--@x";
186+
tok.reset(sql, strlen(sql));
187+
EXPECT_EQ(tok.next_token().type, TokenType::TK_SELECT);
188+
EXPECT_EQ(tok.next_token().type, TokenType::TK_INTEGER);
189+
EXPECT_EQ(tok.next_token().type, TokenType::TK_MINUS);
190+
EXPECT_EQ(tok.next_token().type, TokenType::TK_MINUS);
191+
EXPECT_EQ(tok.next_token().type, TokenType::TK_USER_VARIABLE);
192+
EXPECT_TRUE(tok.has_user_variables());
193+
194+
sql = "SELECT 1-- @x\n";
195+
tok.reset(sql, strlen(sql));
196+
EXPECT_EQ(tok.next_token().type, TokenType::TK_SELECT);
197+
EXPECT_EQ(tok.next_token().type, TokenType::TK_INTEGER);
198+
EXPECT_EQ(tok.next_token().type, TokenType::TK_EOF);
199+
EXPECT_FALSE(tok.has_user_variables());
200+
201+
const char control_sql[] = "SELECT 1--\x7f@x\n";
202+
tok.reset(control_sql, sizeof(control_sql) - 1);
203+
EXPECT_EQ(tok.next_token().type, TokenType::TK_SELECT);
204+
EXPECT_EQ(tok.next_token().type, TokenType::TK_INTEGER);
205+
EXPECT_EQ(tok.next_token().type, TokenType::TK_EOF);
206+
EXPECT_FALSE(tok.has_user_variables());
207+
}
208+
209+
TEST_F(MySQLTokenizerTest, UnterminatedBlockCommentIsAnError) {
210+
const char* sql = "SELECT @x /* unterminated";
211+
tok.reset(sql, strlen(sql));
212+
while (tok.next_token().type != TokenType::TK_EOF) {}
213+
EXPECT_TRUE(tok.has_error());
214+
}
215+
184216
TEST_F(MySQLTokenizerTest, Placeholder) {
185217
const char* sql = "?";
186218
tok.reset(sql, strlen(sql));

‎tests/test_user_variable.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,21 @@ TEST(MySQLUserVariableUsage, RejectsMalformedAndIncompleteParses) {
5858
EXPECT_EQ(classify(sql), UserVariableUsage::UNSAFE_OR_UNKNOWN);
5959
}
6060
}
61+
62+
TEST(MySQLUserVariableUsage, HandlesMySQLCommentBoundariesConservatively) {
63+
EXPECT_EQ(classify("SELECT 1--@x"), UserVariableUsage::READ_ONLY);
64+
EXPECT_EQ(classify("/*!40101 SET @x=1 */"),
65+
UserVariableUsage::UNSAFE_OR_UNKNOWN);
66+
EXPECT_EQ(classify("SELECT @x /* unterminated"),
67+
UserVariableUsage::UNSAFE_OR_UNKNOWN);
68+
EXPECT_EQ(classify("SET @x=1 /* unterminated"),
69+
UserVariableUsage::UNSAFE_OR_UNKNOWN);
70+
}
71+
72+
TEST(MySQLUserVariableUsage, UnterminatedBlockCommentIsNotFullInput) {
73+
Parser<Dialect::MySQL> parser;
74+
const char* sql = "SET @x=1 /* unterminated";
75+
ParseResult result = parser.parse(sql, std::strlen(sql));
76+
EXPECT_EQ(result.status, ParseResult::ERROR);
77+
EXPECT_FALSE(result.full_input);
78+
}

0 commit comments

Comments
 (0)