Opened 6 years ago

Closed 6 years ago

Last modified 5 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
Cc:

Description

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 6 years ago.

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by Even Rouault

Attachment: swq_parser.y.patch added

comment:1 Changed 6 years ago by Even Rouault

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 Changed 6 years ago by tamas

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 Changed 6 years ago by Even Rouault

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 \
	 || (defined YYSTYPE_IS_TRIVIAL && YYSTYPE_IS_TRIVIAL)))

From the http://www.gnu.org/s/bison/manual/html_node/Memory-Management.html 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 Changed 6 years ago by tamas

Applied in trunk (r23206)

comment:5 Changed 6 years ago by Even Rouault

r23211 /trunk/autotest/ogr/ogr_sql_test.py: Test huge SQL queries (#4262)

comment:6 Changed 6 years ago by Even Rouault

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 Changed 5 years ago by Even Rouault

Milestone: 1.8.2

Milestone 1.8.2 deleted

Note: See TracTickets for help on using tickets.