#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)
Change History (8)
by , 13 years ago
Attachment: | swq_parser.y.patch added |
---|
comment:1 by , 13 years ago
comment:2 by , 13 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 , 13 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 \ || (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:5 by , 13 years ago
comment:6 by , 12 years ago
Component: | default → OGR_SF |
---|---|
Keywords: | sql added |
Resolution: | → fixed |
Status: | new → closed |
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