#4447 (ok 4.5) PMA_SQP_parse() unexpected behaviour

[Chirayu Chiripal]

Issue #1 : If query passed to PMA_SQP_parse() contains this '//' (without quotes) then all the alpha_* type tokens such as "alpha_identifier","alpha_reservedWord" etc. are detected as "alpha" causing incorrect parsing. '//' can appear as delimiter in queries. For e.g.

<?php
echo "With //";
$query = 'CREATE TABLE x //';
var_dump(PMA_SQP_parse($query));

echo "Without //";
$query = 'CREATE TABLE x ';
var_dump(PMA_SQP_parse($query));
?>

Will generate this output : http://pastebin.com/RSQzKfNC

Issue #2 : PMA_SQP_parse() replaces all line endings in query passed to unix style (\n) and then parse without changing the actual query passed. It causes problem if query contains \r\n style line endings because the pos now gives wrong values (by 1 position) wrt to actual query. The error in pos gets accumulated with each such line ending. If someone wants to manipulate such query based on the pos values given by PMA_SQP_parse() it results in undesired behaviour because actual query and query parsed by it are different in terms of line endings and thus pos values are not applicable to actual query.

[Chirayu Chiripal]

For Issue #1: the PMA_SQP_getErrorString() function revealed that it was "Unknown Punctuation String" because $GLOBALS['sql_delimiter'] was not set to '//'. IMHO, rather than using $GLOBALS['sql_delimiter'] we should add another parameter ($delimiter = ';') with default value to function?

[Marc Delisle]

Chirayu,
good idea but I'm not sure that this will work when the delimiter has more than one character; it will need testing.

[Chirayu Chiripal]

Currently, $GLOBALS['sql_delimiter'] is defined in sqlparser.data.php as ';' and is used only in sqlparser.lib.php. Also, it doesn't seemed to be getting modified anywhere.

I've tested by setting as $GLOBALS['sql_delimiter'] = '//' and then called PMA_SQP_parse() and it parsed and gave desired results for issue #1. So, I thought it is better to remove the use of superglobal variable and add a default parameter to the function.

[Hugues Peccatte]

I agree with Chirayu: use of globals should be avoid.

@Chirayu: In which case had you this kind of query in pMA please?
Which function would know that the SQL delimiter is not the default one and so would be able to give the new one to PMA_SQP_parse?

[Chunqi Zhu]

Hi,

After stepping through the parser code I have found that unknown punctuation of length 1 and length > 1 are handled differently.

Flow of logic of the code:

if(len == 1) the punctuation character is checked in a switch statement and the result is 'punct' . $t_suffix, with the suffix empty as default (i.e. unrecognised punctuation)
--OR--
The punctuation string is checked against the $GLOBAL['sql_delimiter'] and the list of recognised 2 character punctuations.
--OR--
Many special case checks are done to the punctuation string. The last else if block catches all strings not ending with '~' and causes all type determination to be skipped. The PMA_SQP_arrayAdd call, which in my opinion should serve as the default output for unrecognised multi-character punctuation strings, outside of the if-else block also ends up being (almost) dead code.

I propose that the function call be the last else block of the punctuation loop of the parser and not throw an error that would skip all further processing.

I have submitted a pull request implementing my proposed fix at: https://github.com/phpmyadmin/phpmyadmin/pull/1611/files

[Marc Delisle]

I'm not sure we should touch the parser at this point, as there should be a GSoC 2015 project to rewrite the parser.

[Chunqi Zhu]

I agree with you but I also think that it would help the parser rewrite by providing a potential solution for an existing bug.

I am an applicant for this year's GSoC and this was the byproduct of me trying to get a better understanding of the codebase by working on a small bug.

Thanks.

[Marc Delisle]

The person rewriting the parser will have access to your suggestion, but we won't modify the parser at this point.

[Madhura Jayaratne]

Assigning to Marc since a parser rewrite is planned for this summer

[Dan Ungureanu]

I believe that these issues are no longer relevant. They were related to the internals of the old parser, which is no longer a part of phpMyAdmin.