Fixing BufferedQuery when has odd number of backslashes in the end

[iifawzi]

Hi, this's a fix for the issues noticed @ phpmyadmin/phpmyadmin#15931.

The issue seems to happen when ANY statement has an odd number of backslashes in the end, and this was happening because:

sql-parser/src/Utils/BufferedQuery.php

Lines 187 to 197 in 3013045

	/*
	* Handling backslash.
	*
	* Even if the next character is a special character that should be
	* treated differently, because of the preceding backslash, it will
	* be ignored.
	*/
	if ((($this->status & self::STATUS_COMMENT) === 0) && ($this->query[$i] === '\\')) {
	$this->current .= $this->query[$i] . $this->query[++$i];
	continue;
	}

Here the code is assuming that any backslash will be followed by another backslash, that's why as per my understand the code is pre-incrementing $i to skip both the backslash and the followed character. if there's odd number of backslashes in the end of the statement, the pre-incremented variable would lead to empty string and will mess the variable up.

For example:

\<?php echo \'\1\'\; ?>\

The length ($len) of this statement is 24, the last backslash position ($i) is 23, and (++$i) is 24 which's empty string, and the for loop will also increment it again $i++ which will make this check always false, because $i !== $len 25 !== 24.

sql-parser/src/Utils/BufferedQuery.php

Lines 408 to 410 in 3013045

	if ($end && ($i === $len)) {
	// If the end of the buffer was reached, the buffer is emptied and
	// the current statement that was extracted is returned.

which will make the extract method return empty string, therefore this will go infinitely:

https://github.com/phpmyadmin/phpmyadmin/blob/81e1749cabe40bc429f8d55ff21501bba4c70ed0/libraries/classes/Plugins/Import/ImportSql.php#L163-L167

What I've made is that I've checked whether the ($i + 1) < $len or not, if false, it means that the backslash is in the end of the statement so we don't need to pre-increment and add the followed char to the query - it doesn't even exist - , if true, the variable will be pre-incremented and the followed char will be added to the query.

the ($i + 1) < $len check could be written in many ways, we can make the check this way ! empty($this->query[$i + 1]), which's the way that I've firstly written, but I think it's more meaningful the way in the commit.

That's all I found, I've tested with some queries to ensure that everything is working as expected, but I might be missing something though. You can test with any sql query that has odd number of backslashes in the end.

[williamdes]

Brilliant

[williamdes]

Could you rebase onto QA please?
Maybe write a test before making your fix to be sure it is covered :)
Let me know if you need help or prefer that I write the test

[iifawzi]

Thank you!

Rebased, Anyway it seems like that something went wrong - even before rebasing - This test is failing, I will try to figure out why and add some other tests to ensure that the fix is covered too

1) PhpMyAdmin\SqlParser\Tests\Utils\BufferedQueryTest::testExtract with data set #0 ('SELECT '\'';\nSELECT '\'';', 8, array(true, true), array('SELECT '\'';', 'SELECT '\'';'))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'SELECT '\'';'
-    1 => 'SELECT '\'';'
+    0 => 'SELECT '\'';\n
+    SELECT '\'';'
 )

EDIT => FIXED: The if statement was inverted mistakenly, I will work on adding more tests.

[iifawzi]

Hi @williamdes, I've added a test, but i'm not sure I made it the correct way.
After the fix, the test will pass, but before the fix the test will fail with unclear error (most probably because it will be parsed incorrectly I think ) that's why I'm not really sure.

Feel free to add the appropriate tests, I will also keep an eye, to learn how you will do it, if you will :')

[williamdes]

This all looks great !

[codecov]

Codecov Report

Merging #340 (97773b7) into QA (998338c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##                 QA     #340   +/-   ##
=========================================
  Coverage     99.73%   99.73%           
- Complexity     1911     1912    +1     
=========================================
  Files            63       63           
  Lines          4601     4601           
=========================================
  Hits           4589     4589           
  Misses           12       12           

Impacted Files	Coverage Δ
src/Utils/BufferedQuery.php	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 998338c...97773b7. Read the comment docs.

[williamdes]

Feel free to add the appropriate tests, I will also keep an eye, to learn how you will do it, if you will :')

This one is an example: 998338c

./tools/run_generators.sh will create the missing .out files, delete to have then re-created.

As you can see on the merge: 51ba37c

master branch uses a library that provided JSON syntax with the same features as serialize from PHP but adding more readability
This is why I work on master branch and then copy my patches to QA and create a commit, because on QA any serialized data change is not readable