Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#4262 closed defect (fixed)

OGR SQL expression parser overflow

Reported by: tamas Owned by: tamas
Priority: normal Milestone:
Component: OGR_SF Version: unspecified
Severity: normal Keywords: sql


OGR SQL limits the size of the expression token stack to 200 and a "memory exhausted" error is generated for large expressions. This is related to the YYINITDEPTH setting in swq_parser.cpp. Currently the built in stack extension mechanism is not enabled.

Attached a fix for the parser definition file.

Attachments (1)

swq_parser.y.patch (775 bytes ) - added by Even Rouault 11 years ago.

Download all attachments as: .zip

Change History (8)

by Even Rouault, 11 years ago

Attachment: swq_parser.y.patch added

comment:1 by Even Rouault, 11 years ago

Tamas, I've updated your patch by just adding a few line of comments, because it took me quite a bit to understand how this worked. Looks fine to apply in trunk and 1.8 branch too

comment:2 by tamas, 11 years ago

Even, The documentation you refer to is quite confusing. It talks about YYMAXDEPTH which is strictly related to the feature you mean "undocumented".

I also have an impression that YYSTYPE_IS_TRIVIAL may depend on how YYSTYPE is defined actually. for an example see the mapserver parser in mapparser.c where YYSTYPE_IS_TRIVIAL is defined by default.

comment:3 by Even Rouault, 11 years ago

What I meant to be undocumented is YYSTYPE_IS_TRIVIAL itself. But perhaps I didn't look at the right place ? I found out why YYSTYPE_IS_TRIVIAL was necessary by grepping for it in swq_parser.cpp

#if (! defined yyoverflow \
     && (! defined __cplusplus \

From the page, my feeling is that the stack extension mechanism isn't enabled by default when generated the parser as a C++ file (which is OGR case, to be opposed to the C mapparser.c). So you then have to define YYSTYPE_IS_TRIVIAL yourself after ensuring that the YYSTYPE isn't a C++ object, but something that can be safely memcpy'ed for example. Well, this is my interpretation of a quick look at the generated code.

I tested a bit your patch with GCC on Linux and it worked fine. I suppose you tested it on Windows with MSVC ? Enhancing the test suite might be good too.

As the patch isn't invasive, so I think it is OK to apply it to trunk even if there's some uncertainty about its correctness, and in case of issues, the revert will be easy. If you wish, you could wait for it to be a bit more tested in trunk before backporting to 1.8. Just my 2 cents.

comment:4 by tamas, 11 years ago

Applied in trunk (r23206)

comment:5 by Even Rouault, 11 years ago

r23211 /trunk/autotest/ogr/ Test huge SQL queries (#4262)

comment:6 by Even Rouault, 11 years ago

Component: defaultOGR_SF
Keywords: sql added
Resolution: fixed
Status: newclosed

r23311 /branches/1.8/ (3 files in 2 dirs): Allow parser to deal with huge SQL queries (#4262)

comment:7 by Even Rouault, 9 years ago

Milestone: 1.8.2

Milestone 1.8.2 deleted

Note: See TracTickets for help on using tickets.