Make QgsExpression parser reentrant. Fixes crashes when expressions a…

…re parsed in multiple threads. Sponsored by a big dose of caffeine.
qgis · Sep 25, 2014 · 2427546 · 2427546
1 parent 721cab1
commit 2427546
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 31 deletions.
diff --git a/src/core/qgsexpressionlexer.ll b/src/core/qgsexpressionlexer.ll
@@ -18,6 +18,10 @@
 %option never-interactive
 %option nounput
 %option prefix="exp_"
+ // this makes flex generate lexer with context + init/destroy functions
+%option reentrant
+ // this makes Bison send yylex another argument to use instead of using the global variable yylval
+%option bison-bridge
 
  // ensure that lexer will be 8-bit (and not just 7-bit)
 %option 8bit
@@ -27,6 +31,7 @@
 #include <stdlib.h>  // atof()
 
 #include "qgsexpression.h"
+struct expression_parser_context;
 #include "qgsexpressionparser.hpp"
 #include <QRegExp>
 #include <QLocale>
@@ -43,10 +48,10 @@
 #define YY_NO_UNISTD_H
 #endif
 
-#define B_OP(x) exp_lval.b_op = QgsExpression::x
-#define U_OP(x) exp_lval.u_op = QgsExpression::x
-#define TEXT                   exp_lval.text = new QString(); *exp_lval.text = QString::fromUtf8(yytext);
-#define TEXT_FILTER(filter_fn) exp_lval.text = new QString(); *exp_lval.text = filter_fn( QString::fromUtf8(yytext) );
+#define B_OP(x) yylval->b_op = QgsExpression::x
+#define U_OP(x) yylval->u_op = QgsExpression::x
+#define TEXT                   yylval->text = new QString(); *yylval->text = QString::fromUtf8(yytext);
+#define TEXT_FILTER(filter_fn) yylval->text = new QString(); *yylval->text = filter_fn( QString::fromUtf8(yytext) );
 
 static QString stripText(QString text)
 {
@@ -154,14 +159,14 @@ string      "'"{str_char}*"'"
 
 ","   { return COMMA; }
 
-{num_float}  { exp_lval.numberFloat  = cLocale.toDouble( QString::fromAscii(yytext) ); return NUMBER_FLOAT; }
+{num_float}  { yylval->numberFloat  = cLocale.toDouble( QString::fromAscii(yytext) ); return NUMBER_FLOAT; }
 {num_int}  {
 	bool ok;
-	exp_lval.numberInt = cLocale.toInt( QString::fromAscii(yytext), &ok, 10 );
+  yylval->numberInt = cLocale.toInt( QString::fromAscii(yytext), &ok, 10 );
 	if( ok )
 		return NUMBER_INT;
 
-	exp_lval.numberFloat  = cLocale.toDouble( QString::fromAscii(yytext), &ok );
+  yylval->numberFloat  = cLocale.toDouble( QString::fromAscii(yytext), &ok );
 	if( ok )
 		return NUMBER_FLOAT;
 
@@ -172,7 +177,7 @@ string      "'"{str_char}*"'"
 
 {special_col}        { TEXT; return SPECIAL_COL; }
 
-{column_ref}         { TEXT; return QgsExpression::isFunctionName(*exp_lval.text) ? FUNCTION : COLUMN_REF; }
+{column_ref}         { TEXT; return QgsExpression::isFunctionName(*yylval->text) ? FUNCTION : COLUMN_REF; }
 
 {column_ref_quoted}  { TEXT_FILTER(stripColumnRef); return COLUMN_REF; }
 
@@ -181,9 +186,3 @@ string      "'"{str_char}*"'"
 .       { return Unknown_CHARACTER; }
 
 %%
-
-void exp_set_input_buffer(const char* buffer)
-{
-  exp__scan_string(buffer);
-}
-
diff --git a/src/core/qgsexpressionparser.yy b/src/core/qgsexpressionparser.yy
@@ -27,22 +27,37 @@
 // don't redeclare malloc/free
 #define YYINCLUDED_STDLIB_H 1
 
+struct expression_parser_context;
+#include "qgsexpressionparser.hpp"
+
 //! from lexer
-extern int exp_lex();
-extern void exp_set_input_buffer(const char* buffer);
+typedef void* yyscan_t;
+typedef struct yy_buffer_state* YY_BUFFER_STATE;
+extern int exp_lex_init(yyscan_t* scanner);
+extern int exp_lex_destroy(yyscan_t scanner);
+extern int exp_lex(YYSTYPE* yylval_param, yyscan_t yyscanner);
+extern YY_BUFFER_STATE exp__scan_string(const char* buffer, yyscan_t scanner);
 
 /** returns parsed tree, otherwise returns NULL and sets parserErrorMsg
     (interface function to be called from QgsExpression)
   */
 QgsExpression::Node* parseExpression(const QString& str, QString& parserErrorMsg);
 
 /** error handler for bison */
-void exp_error(const char* msg);
+void exp_error(expression_parser_context* parser_ctx, const char* msg);
+
+struct expression_parser_context
+{
+  // lexer context
+  yyscan_t flex_scanner;
 
-//! varible where the parser error will be stored
-QString gExpParserErrorMsg;
-QgsExpression::Node* gExpParserRootNode;
+  // varible where the parser error will be stored
+  QString errorMsg;
+  // root node of the expression
+  QgsExpression::Node* rootNode;
+};
 
+#define scanner parser_ctx->flex_scanner
 
 // we want verbose error messages
 #define YYERROR_VERBOSE 1
@@ -51,6 +66,11 @@ QgsExpression::Node* gExpParserRootNode;
 
 %}
 
+// make the parser reentrant
+%define api.pure
+%lex-param {void * scanner}
+%parse-param {expression_parser_context* parser_ctx}
+
 %name-prefix "exp_"
 
 %union
@@ -130,7 +150,7 @@ QgsExpression::Node* gExpParserRootNode;
 
 %%
 
-root: expression { gExpParserRootNode = $1; }
+root: expression { parser_ctx->rootNode = $1; }
     ;
 
 expression:
@@ -162,13 +182,13 @@ expression:
           {
             // this should not actually happen because already in lexer we check whether an identifier is a known function
             // (if the name is not known the token is parsed as a column)
-            exp_error("Function is not known");
+            exp_error(parser_ctx, "Function is not known");
             YYERROR;
           }
           if ( QgsExpression::Functions()[fnIndex]->params() != -1
                && QgsExpression::Functions()[fnIndex]->params() != $3->count() )
           {
-            exp_error("Function is called with wrong number of arguments");
+            exp_error(parser_ctx, "Function is called with wrong number of arguments");
             YYERROR;
           }
           $$ = new QgsExpression::NodeFunction(fnIndex, $3);
@@ -195,7 +215,7 @@ expression:
           {
       if ( !QgsExpression::hasSpecialColumn( *$1 ) )
 	    {
-	      exp_error("Special column is not known");
+        exp_error(parser_ctx, "Special column is not known");
 	      YYERROR;
 	    }
 	    // $var is equivalent to _specialcol_( "$var" )
@@ -234,28 +254,33 @@ when_then_clause:
 
 %%
 
+
 // returns parsed tree, otherwise returns NULL and sets parserErrorMsg
 QgsExpression::Node* parseExpression(const QString& str, QString& parserErrorMsg)
 {
-  gExpParserRootNode = NULL;
-  exp_set_input_buffer(str.toUtf8().constData());
-  int res = exp_parse();
+  expression_parser_context ctx;
+  ctx.rootNode = 0;
+
+  exp_lex_init(&ctx.flex_scanner);
+  exp__scan_string(str.toUtf8().constData(), ctx.flex_scanner);
+  int res = exp_parse(&ctx);
+  exp_lex_destroy(ctx.flex_scanner);
 
   // list should be empty when parsing was OK
   if (res == 0) // success?
   {
-    return gExpParserRootNode;
+    return ctx.rootNode;
   }
   else // error?
   {
-    parserErrorMsg = gExpParserErrorMsg;
+    parserErrorMsg = ctx.errorMsg;
     return NULL;
   }
 }
 
 
-void exp_error(const char* msg)
+void exp_error(expression_parser_context* parser_ctx, const char* msg)
 {
-  gExpParserErrorMsg = msg;
+  parser_ctx->errorMsg = msg;
 }
 
diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -16,6 +16,8 @@
 #include <QObject>
 #include <QString>
 #include <QObject>
+#include <QtConcurrentMap>
+
 #include <qgsapplication.h>
 //header for class being tested
 #include <qgsexpression.h>
@@ -29,6 +31,16 @@
 Q_DECLARE_METATYPE( QVariant )
 #endif
 
+static void _parseAndEvalExpr( int arg )
+{
+  Q_UNUSED( arg );
+  for ( int i = 0; i < 100; ++i )
+  {
+    QgsExpression exp( "1 + 2 * 2" );
+    exp.evaluate();
+  }
+}
+
 class TestQgsExpression: public QObject
 {
     Q_OBJECT;
@@ -946,6 +958,15 @@ class TestQgsExpression: public QObject
       QCOMPARE( QgsExpression::quotedString( "hello\\world" ), QString( "'hello\\\\world'" ) );
     }
 
+    void reentrant()
+    {
+      // this simply should not crash
+
+      QList<int> lst;
+      for ( int i = 0; i < 10; ++i )
+        lst << i;
+      QtConcurrent::blockingMap( lst, _parseAndEvalExpr );
+    }
 };
 
 QTEST_MAIN( TestQgsExpression )