Repository: sysown/proxysql (language: cpp)

Issue:
Not ignoring comments on "use database" statements
I find that the following works as expected:

    use db /* COMMENT */

it will connect to the database `db`.

However, if there is a newline in the comment:

    use db /*
       COMMENT */

it will try to switch to the database `db /* COMMENT */`, which obviously not what should happen. 

The other types of comment aren't ignored either:

    use db -- COMMENT
    use db # COMMENT

result in attempts to switch to database `db -- COMMENT` and `db # COMMENT`.

I have not seen this issue with other statements than the `use` statement, but I haven't done an exhaustive search.


Discussion / hints:
Hi @Abigail,

the issue template is not a form to scrap and replace with free-writing. There are several details required to follow up on this, all of them missing, but that would be present if the template was filled. When opening an issue, please, at least make sure the points on the issue template are filled.

Thanks, Javier.
Sorry, I don't know all the details being asked.

You can just ignore my bug report, or, alternatively, investigate the regexp on line 6139 of `lib/MySQL_Session.cpp`. The regexp is:
```
     /\\*.*\\*/
```
which is problematic in three ways:
1. `.` matches any character, but a newline.
2. The regexp is too greedy, meaning that if you have two comments on the same line (like `SQL1 /* comment */ SQL2 /* comment */`) it replaces the code between the comments (replacing `/* comment */ SQL2 /* comment */` by a space).
3. It doesn't match `-- ` or `#` style comments.

May I suggest something like
```
     (?:(?:#|--(?=\s))[^\n]*\n|/\*(?:(?>[^*]+)|\*(?!/))*(?:;|\*/))
```
instead?
Hi @Abigail,

> You can just ignore my bug report, or, alternatively, investigate the regexp on line 6139 of lib/MySQL_Session.cpp. The regexp is:

What an interesting wording you used, I find myself at a crossroads, only two ways ahead! I understand this is your *not so nice* way of presenting your findings. Let me be clear, my request was not because there was the need of extra info for a regular contributor to find the **obvious** cause of the behavior, or to change a simple regex, into a **sightly** more complex one. A couple of `git blames` would tell you that the change was intentional, and that the behavior is well known, and was changed like this by design for the time being. Giving you more context, for several parsing related issues, we solve the most common and pressing cases, and (this has changed overtime) we might take a different approach for fixing these kind of parsing issues all together, or *maybe*, while waiting for this more ideal solution, we might use more complex regexes (like the one you provided).

The point of asking those details, as you can probably guess, is procedural. Those details are generic, and help with issue categorization, even without knowledge of the codebase, something that universal and common to all projects (even outside software). So please, just for the sake of curiosity, and maybe improving the template, which one of this highly technical details, was not possible to share?

```
If you are submitting a reproducible bug report, please provide:
- [ ] A clear description of the issue  // you already did this
- [ ] ProxySQL version // ??
- [ ] OS version // ??
- [ ] The steps to reproduce the issue // ??
- [ ] The **full** ProxySQL error log (default location: `/var/lib/proxysql/proxysql.log`) // copy paste?
```

Regards, Javier.

Fix patch:
diff --git a/include/set_parser.h b/include/set_parser.h
index 5421868342..bd89bbab22 100644
--- a/include/set_parser.h
+++ b/include/set_parser.h
@@ -42,6 +42,12 @@ class SetParser {
 	// First implemenation of the parser for TRANSACTION ISOLATION LEVEL and TRANSACTION READ/WRITE
 	std::map<std::string, std::vector<std::string>> parse2();
 	std::string parse_character_set();
+	std::string parse_USE_query();
+	std::string remove_comments(const std::string& q);
+#ifdef DEBUG
+	// built-in testing
+	void test_parse_USE_query();
+#endif // DEBUG
 	~SetParser();
 };
 
diff --git a/lib/MySQL_Session.cpp b/lib/MySQL_Session.cpp
index b40d922539..42930c9b92 100644
--- a/lib/MySQL_Session.cpp
+++ b/lib/MySQL_Session.cpp
@@ -6138,30 +6138,27 @@ void MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
 	if (session_type == PROXYSQL_SESSION_MYSQL) {
 		__sync_fetch_and_add(&MyHGM->status.frontend_use_db, 1);
 		string nq=string((char *)pkt->ptr+sizeof(mysql_hdr)+1,pkt->size-sizeof(mysql_hdr)-1);
-		RE2::GlobalReplace(&nq,(char *)"(?U)/\\*.*\\*/",(char *)" ");
-		char *sn_tmp = (char *)nq.c_str();
-		while (sn_tmp < ( nq.c_str() + nq.length() - 4 ) && *sn_tmp == ' ')
-			sn_tmp++;
-		//char *schemaname=strdup(nq.c_str()+4);
-		char *schemaname=strdup(sn_tmp+3);
-		char *schemanameptr=trim_spaces_and_quotes_in_place(schemaname);
-		// handle cases like "USE `schemaname`
-		if(schemanameptr[0]=='`' && schemanameptr[strlen(schemanameptr)-1]=='`') {
-			schemanameptr[strlen(schemanameptr)-1]='\0';
-			schemanameptr++;
-		}
-		client_myds->myconn->userinfo->set_schemaname(schemanameptr,strlen(schemanameptr));
-		free(schemaname);
-		if (mirror==false) {
+		SetParser parser(nq);
+		string schemaname = parser.parse_USE_query();
+		if (schemaname != "") {
+			client_myds->myconn->userinfo->set_schemaname((char *)schemaname.c_str(),schemaname.length());
+			if (mirror==false) {
+				RequestEnd(NULL);
+			}
+			l_free(pkt->size,pkt->ptr);
+			client_myds->setDSS_STATE_QUERY_SENT_NET();
+			unsigned int nTrx=NumActiveTransactions();
+			uint16_t setStatus = (nTrx ? SERVER_STATUS_IN_TRANS : 0 );
+			if (autocommit) setStatus |= SERVER_STATUS_AUTOCOMMIT;
+			client_myds->myprot.generate_pkt_OK(true,NULL,NULL,1,0,0,setStatus,0,NULL);
+			GloMyLogger->log_audit_entry(PROXYSQL_MYSQL_INITDB, this, NULL);
+		} else {
+			l_free(pkt->size,pkt->ptr);
+			client_myds->setDSS_STATE_QUERY_SENT_NET();
+			std::string msg = "Unable to parse: " + nq;
+			client_myds->myprot.generate_pkt_ERR(true,NULL,NULL,client_myds->pkt_sid+1,1148,(char *)"42000", msg.c_str());
 			RequestEnd(NULL);
 		}
-		l_free(pkt->size,pkt->ptr);
-		client_myds->setDSS_STATE_QUERY_SENT_NET();
-		unsigned int nTrx=NumActiveTransactions();
-		uint16_t setStatus = (nTrx ? SERVER_STATUS_IN_TRANS : 0 );
-		if (autocommit) setStatus |= SERVER_STATUS_AUTOCOMMIT;
-		client_myds->myprot.generate_pkt_OK(true,NULL,NULL,1,0,0,setStatus,0,NULL);
-		GloMyLogger->log_audit_entry(PROXYSQL_MYSQL_INITDB, this, NULL);
 		client_myds->DSS=STATE_SLEEP;
 	} else {
 		l_free(pkt->size,pkt->ptr);
diff --git a/lib/set_parser.cpp b/lib/set_parser.cpp
index 521808d642..7abef9adf1 100644
--- a/lib/set_parser.cpp
+++ b/lib/set_parser.cpp
@@ -3,9 +3,11 @@
 #include <string>
 #include <vector>
 #include <map>
-#ifdef PARSERDEBUG
+#include <cassert>
+#include <utility> // for std::pair
+//#ifdef PARSERDEBUG
 #include <iostream>
-#endif
+//#endif
 
 using namespace std;
 
@@ -506,3 +508,148 @@ std::string SetParser::parse_character_set() {
 	return value4;
 }
 
+std::string SetParser::parse_USE_query() {
+#ifdef DEBUG
+	proxy_debug(PROXY_DEBUG_MYSQL_QUERY_PROCESSOR, 4, "Parsing query %s\n", query.c_str());
+#endif // DEBUG
+
+	re2::RE2::Options opt2(RE2::Quiet);
+	opt2.set_case_sensitive(false);
+	opt2.set_longest_match(false);
+
+	std::string dbname = remove_comments(query);
+	re2::RE2 re0("^\\s*", opt2);
+	re2::RE2::Replace(&dbname, re0, "");
+	if (dbname.size() >= 4) {
+		if (
+			strncasecmp(dbname.c_str(), "USE ",4) == 0
+			||
+			strncasecmp(dbname.c_str(), "USE`",4) == 0
+		) {
+			re2::RE2 re1("^USE\\s*", opt2);
+			re2::RE2::Replace(&dbname, re1, "");
+			re2::RE2 re2("\\s*$", opt2);
+			re2::RE2::Replace(&dbname, re2, "");
+			if (dbname[0] == '`') {
+				if (dbname.length() > 2) {
+					if (dbname[dbname.length()-1] == '`') {
+						// Remove the first character
+						dbname.erase(0, 1);
+						// Remove the last character
+						dbname.erase(dbname.length() - 1);
+						return dbname;
+					}
+				}
+			} else {
+				return dbname;
+			}
+		}
+	} else {
+		return "";
+	}
+
+	return "";
+}
+
+
+std::string SetParser::remove_comments(const std::string& q) {
+    std::string result = "";
+    bool in_multiline_comment = false;
+
+    for (size_t i = 0; i < query.size(); ++i) {
+        char current_char = query[i];
+
+        // Check for multiline comment start
+        if (current_char == '/' && i + 1 < query.size() && query[i + 1] == '*') {
+            in_multiline_comment = true;
+            i++; // Skip the '*'
+            continue;
+        }   
+
+        // Check for multiline comment end
+        if (in_multiline_comment && current_char == '*' && i + 1 < query.size() && query[i + 1] == '/') {
+            in_multiline_comment = false;
+            i++; // Skip the '/'
+            continue;
+        }   
+
+        // Skip characters inside multiline comment
+        if (in_multiline_comment) {
+            continue;
+        }
+
+        // Check for single-line comments
+        if (current_char == '#' || (current_char == '-' && i + 1 < query.size() && query[i + 1] == '-')) {
+            // Skip until the end of the line
+            while (i < query.size() && query[i] != '\n') {
+                i++;
+            }
+            continue;
+        }
+
+        // Append the character to the result if it's not a comment
+        result += current_char;
+    }
+
+    return result;
+}
+
+
+#ifdef DEBUG
+void SetParser::test_parse_USE_query() {
+
+	// Define vector of pairs (query, expected dbname)
+	std::vector<std::pair<std::string, std::string>> testCases = {
+		{"USE my_database",    "my_database"},                   // Basic Case
+		{"USE   my_database",  "my_database"},                   // Basic Case
+		{"USE   my_database ", "my_database"},                   // Basic Case
+		{"/* comment */USE dbname /* comment */",   "dbname"}, // With Comments
+		{"/* comment */   USE   dbname",            "dbname"}, // With Comments
+		{"USE    dbname           /* comment */",   "dbname"}, // With Comments
+		{"/* comment */USE `dbname` /* comment */", "dbname"}, // With backtick
+		{"/* comment */USE `dbname`/* comment */",  "dbname"}, // With backtick
+		{"/* comment */USE`dbname` /* comment */",  "dbname"}, // With backtick
+		{"/* comment */USE `dbname`/* comment */",  "dbname"}, // With backtick
+		{"/* comment\nmultiline comment */USE dbname /* comment */", "dbname"}, // Multiline Comment
+		{"/* comment */USE dbname # comment",       "dbname"}, // Hash Comment
+		{"/* comment */USE dbname -- comment",      "dbname"}, // Double Dash Comment
+		{"/* comment */USE dbname   # comment",     "dbname"}, // Hash Comment
+		{"/* comment */USE dbname    -- comment",   "dbname"}, // Double Dash Comment
+		{"USE dbname   # comment",                  "dbname"}, // Hash Comment
+		{"USE dbname    -- comment",                "dbname"}, // Double Dash Comment
+		{"SELECT * FROM my_table", ""}, // No match
+		{"/*+ placeholder_comment */ USE test_use_comment", "test_use_comment"},
+
+		{"USE /*+ placeholder_comment */ `test_use_comment-a1`",  "test_use_comment-a1"},
+		{"USE /*+ placeholder_comment */   `test_use_comment_1`", "test_use_comment_1"},
+		{"USE/*+ placeholder_comment */ `test_use_comment_2`",    "test_use_comment_2"},
+		{"USE /*+ placeholder_comment */`test_use_comment_3`",    "test_use_comment_3"},
+		{"USE /*+ placeholder_comment */   test_use_comment_4",   "test_use_comment_4"},
+		{"USE/*+ placeholder_comment */ test_use_comment_5",      "test_use_comment_5"},
+		{"USE /*+ placeholder_comment */test_use_comment_6",      "test_use_comment_6"},
+		{"USE /*+ placeholder_comment */   `test_use_comment-1`", "test_use_comment-1"},
+		{"use my_database", "my_database"},
+		{"/* comment */  use dbname -- comment",        "dbname"},
+		{"/* comment\nmultiline comment */USE dbname /* comment\nmultiline comment */", "dbname"}, // Multiline Comment
+
+		{"USE/*+ placeholder_comment */ `test_use_comment-2`",      "test_use_comment-2"},
+		{"USE /*+ placeholder_comment */`test_use_comment-3`",      "test_use_comment-3"},
+		{"/*+ placeholder_comment */USE      `test_use_comment-4`", "test_use_comment-4"},
+		{"USE/*+ placeholder_comment */`test_use_comment-5`",       "test_use_comment-5"},
+		{"/* comment */USE`test_use_comment-6`",                    "test_use_comment-6"},
+		{"USE`test_use_comment-7`",                                 "test_use_comment-7"},
+	};
+
+	// Run tests for each pair
+	for (const auto& p : testCases) {
+		set_query(p.first);
+		std::string dbname = parse_USE_query();
+		if (dbname != p.second) {
+			// we call parse_USE_query() again just to make it easier to create a breakpoint
+			std::string s = parse_USE_query();
+			assert(s == p.second);
+		}
+	}
+
+}
+#endif // DEBUG
diff --git a/src/main.cpp b/src/main.cpp
index c30a6bd999..d98743a604 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -1976,6 +1976,13 @@ int main(int argc, const char * argv[]) {
 //		std::cerr << "Main init phase0 completed in ";
 #endif
 	}
+#ifdef DEBUG
+	{
+		// Automated testing
+		SetParser parser("");
+		parser.test_parse_USE_query();
+	}
+#endif // DEBUG
 	{
 		cpu_timer t;
 		ProxySQL_Main_process_global_variables(argc, argv);


Test patch: