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

LDEV-4866 fix query parser out of bounds (6.2) take 2 #2399

Open
wants to merge 4 commits into
base: 6.2
Choose a base branch
from

Conversation

zspitzer
Copy link
Member

@zspitzer zspitzer commented Jul 5, 2024

@zspitzer zspitzer changed the title LDEV-4866 fix query parser out of bounds LDEV-4866 fix query parser out of bounds (6.2) take 2 Jul 5, 2024
Copy link

github-actions bot commented Jul 5, 2024

Lucee Test Results

    1 files  ± 0  1 467 suites  24   5m 15s ⏱️ 41s
3 872 tests 90  3 680 ✅ 85  192 💤 5  0 ❌ ±0 
4 007 runs  96  3 795 ✅ 91  212 💤 5  0 ❌ ±0 

Results for commit fb693ae. ± Comparison against base commit 7841bc5.

♻️ This comment has been updated with latest results.


// handle single line comment
if (c == '-' && i < (sqlLen - 1) && sql.charAt(i 1) == '-') {
int end = sql.indexOf('\n', i 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not handle if the comment is at the last line of the SQL

if ( i < (sqlLen - 1)) {
// handle multi line comment
if (c == '/' && sql.charAt(i 1) == '*') {
int end = sql.indexOf("*/", i 2);
Copy link
Contributor

@michaeloffner michaeloffner Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't handle at all if the multi line or single line comment is within a literal string like
select '/* multi */ // -- single' as comments, that actually is the most complicated part about parsing, be aware of the current context ans switch between them. This also reinvents the wheel for code we already have in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants