[mapserver-dev] App crash in mapparser.y

[Steve Lime]

Hi Tamas: Ok, reacquainting myself with that code. There are actually two
lexers in play - msyylex() is the main one that is used to parse a mapfile
and also expression strings - it is not thread-safe. With regards to
expressions, msyylex() is used to generate a series of tokens - see
msTokenizeExpression() in maplayer.c. There is a lock present when
msyylex() is used as part of that function. The Bison parser has its own
lexer (yylex(), defined in mapparser.y) that is used to traverse the series
of tokens. That function (and yyparse()) operate on a parseObj that fully
encapsulates expression evaluation. So the parser should be reentrant...

Assuming that LABELREQUIRES is in play here, the simplest explanation is
that its value contains a bad layer name in the expression. That would
leave something like "[badlayer]" remaining in the string passed to
msTokenizeString() and would result in the error you noted originally. We
could/should add a check in msEvalContext() to examine the expression
string to catch this condition. Using strchr to check for remaining '[' or
']' characters would probably do it.

--Steve

On Sat, Feb 19, 2022 at 12:19 PM Tamas Szekeres <szekerest at gmail.com> wrote:

> Hi Steve,
>
> According to the call stack below, yyparse also calling yylex internally,
> which is not considered to be reentrant I think.
>
> *mapserver!yylex+0x1b5 [e:\builds-t1\src\mapserver-intramaps\vc15x64\mapparser.y @ 858]
> mapserver!yyparse+0x247 [e:\home\even\mapserver\mapserver\mapparser.c @ 1461]
> mapserver!msEvalContext+0x178 [e:\builds-t1\src\mapserver-intramaps\maputil.c @ 442]*
>
>
> Best regards,
>
> Tamas
>
>
> Steve Lime <sdlime at gmail.com> ezt írta (időpont: 2022. febr. 19., Szo,
> 17:53):
>
>> I thought generating tokens for an expression needed a lock since it uses
>> the lexer but that the parser did not. I could be wrong though.
>>
>> On Sat, Feb 19, 2022 at 10:21 AM Steve Lime <sdlime at gmail.com> wrote:
>>
>>> I don’t remember offhand and will have to check. That said if there is a
>>> attribute level token in a layer level expression than that would explain
>>> your research. Is there really no way to get info on the expression(s) in
>>> question?
>>>
>>> On Sat, Feb 19, 2022 at 9:22 AM Tamas Szekeres <szekerest at gmail.com>
>>> wrote:
>>>
>>>> I'm not too familiar in the current parser/lexer implementation, but is
>>>> msTokenizeExpression and yyparse supposed to be protected by the same
>>>> parser lock in msEvalContext? Or if  msTokenizeExpression doesn't
>>>> leave any global variables behind, should yyparse be protected by it's own?
>>>>
>>>> Thanks,
>>>>
>>>> Tamas
>>>>
>>>>
>>>> Steve Lime <sdlime at gmail.com> ezt írta (időpont: 2022. febr. 19., Szo,
>>>> 16:02):
>>>>
>>>>> Requires and labelrequires aren’t operating at the feature level,
>>>>> rather the layer level so they definitely don’t support attribute binding.
>>>>> We’d need to whip up a test case to figure out a method to trap this. The
>>>>> workaround is to fix the expression. I mean, obviously we don’t want to
>>>>> crash but it’s never going to work if written as you suspect.
>>>>>
>>>>> On Sat, Feb 19, 2022 at 7:55 AM Tamas Szekeres <szekerest at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Devs,
>>>>>>
>>>>>> I got a crash log which shows that the access violation is happening
>>>>>> in the following location:
>>>>>>
>>>>>> *mapserver!yylex+0x1b5 [e:\builds-t1\src\mapserver-intramaps\vc15x64\mapparser.y @ 858]
>>>>>> 		mapserver!yyparse+0x247 [e:\home\even\mapserver\mapserver\mapparser.c @ 1461]
>>>>>> 		mapserver!msEvalContext+0x178 [e:\builds-t1\src\mapserver-intramaps\maputil.c @ 442]
>>>>>> 		mapserver!msDrawVectorLayer+0xb0 [e:\builds-t1\src\mapserver-intramaps\mapdraw.c @ 918]
>>>>>> 		mapserver!msDrawLayer+0x3a3 [e:\builds-t1\src\mapserver-intramaps\mapdraw.c @ 813]
>>>>>> 		mapserver!msDrawMap+0x415 [e:\builds-t1\src\mapserver-intramaps\mapdraw.c @ 403]
>>>>>> 		mapscript!CSharp_mapObj_draw+0xd*
>>>>>>
>>>>>> By looking into the corresponding code, the problem has happened in
>>>>>> the following location in yylex():
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> * case MS_TOKEN_BINDING_INTEGER:    token = NUMBER; ---->   (*lvalp).dblval = atof(p->shape->values[p->expr->curtoken->tokenval.bindval.index]);    break;*
>>>>>>
>>>>>> In maputil.c the call stack location is here (msEvalContext):
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> *msTokenizeExpression(&e, NULL, NULL);  p.shape = NULL;  p.expr = &e;  p.expr->curtoken = p.expr->tokens; /* reset */  p.type = MS_PARSE_TYPE_BOOLEAN;----->  status = yyparse(&p);  msFreeExpression(&e);*
>>>>>>
>>>>>> And in mapdraw.c
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> * /* TODO TBT: draw as raster layer in vector renderers */----->  annotate = msEvalContext(map, layer, layer->labelrequires);  if(map->scaledenom > 0) {    if((layer->labelmaxscaledenom != -1) && (map->scaledenom >= layer->labelmaxscaledenom)) annotate = MS_FALSE;    if((layer->labelminscaledenom != -1) && (map->scaledenom < layer->labelminscaledenom)) annotate = MS_FALSE;  }*
>>>>>>
>>>>>> I'm not sure if the LABELREQUIRES option supports attribute binding
>>>>>> (probably not) and in that case the
>>>>>> p->expr->curtoken->tokenval.bindval.index would probably have undefined
>>>>>> value. I don't have any information about the corresponding LABELREQUIRES
>>>>>> expression , but if I assume it doesn't contain attribute binding, then
>>>>>> this crash might probably happen due to a missing parser lock around *yyparse(&p).
>>>>>> *What do you think about the problem and how could we work around
>>>>>> this?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Tamas
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> MapServer-dev mailing list
>>>>>> MapServer-dev at lists.osgeo.org
>>>>>> https://lists.osgeo.org/mailman/listinfo/mapserver-dev
>>>>>>
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/mapserver-dev/attachments/20220222/5936539f/attachment.html>