Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wkt enforce whitespace #277

Closed
wants to merge 1 commit into
base: svn-trunk
from

Conversation

Projects
None yet
3 participants
@Algunenano
Copy link
Member

Algunenano commented Jul 23, 2018

Fix for https://trac.osgeo.org/postgis/ticket/4109

It'd great if someone with experience with Bison could have a look.

@Komzpa

This comment has been minimized.

Copy link
Member

Komzpa commented Jul 23, 2018

If it does not break an existing test, I'd say we merge this.
(WKT input is also tested via oss-fuzz so a major crash, if any, will be found.)

@pramsey

This comment has been minimized.

Copy link
Member

pramsey commented Jul 23, 2018

I find the solution breathtakingly ugly so I'm looking for a better way...

@Algunenano

This comment has been minimized.

Copy link
Member Author

Algunenano commented Jul 23, 2018

I find the solution breathtakingly ugly so I'm looking for a better way...

I'm not going to disagree with you on this one. It is extremely ugly 👹

@pramsey

This comment has been minimized.

Copy link
Member

pramsey commented Jul 23, 2018

I think the following two-liner is sufficient to fix the issue:

--?(([0-9]+\.?)|([0-9]*\.?[0-9]+)([eE][-+]?[0-9]+)?) {  
+-?(([0-9]+\.?)|([0-9]*\.?[0-9]+)([eE][-+]?[0-9]+)?)[ \,\)] {
       LWDEBUG(5,"DOUBLE");
       wkt_yylval.doublevalue = atof(wkt_yytext);
+      yyless(wkt_yyleng-1);
       return DOUBLE_TOK;
       }

The pattern change insists that all doubles must be anchored at the end by a space, comma or close bracket. The call to yyless puts that anchor back into the input stream so that it can subsequently be picked up by things like the comma and bracket patterns. (Probably that should also be included in the patch as a comment. :)

@Algunenano

This comment has been minimized.

Copy link
Member Author

Algunenano commented Jul 23, 2018

The pattern change insists that all doubles must be anchored at the end by a space, comma or close bracket. The call to yyless puts that anchor back into the input stream so that it can subsequently be picked up by things like the comma and bracket patterns.

What about multiple spaces, tabs or CRs? I'll try to add them with a few tests

@Algunenano Algunenano force-pushed the Algunenano:wkt_enforce_whitespace branch from 964fa4d to ffbeb8e Jul 23, 2018

@pramsey

This comment has been minimized.

Copy link
Member

pramsey commented Jul 23, 2018

I'm not sure how much more lenient we want to be WRT CRs. Tabs maybe. Multiples should end up being eaten by the whitespace rule at the bottom, so don't add them to the double pattern.

@Algunenano

This comment has been minimized.

Copy link
Member Author

Algunenano commented Jul 23, 2018

I've added single \t \n \r. This might still not be enough for Windows (\r\n); is it worth it to address it?

@pramsey

This comment has been minimized.

Copy link
Member

pramsey commented Jul 23, 2018

I don't think so. If your WKT is

POINT(123.
4123)

you're SOL, sorry.

@pramsey

This comment has been minimized.

Copy link
Member

pramsey commented Jul 23, 2018

👍

@strk strk closed this in 1ba28a8 Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.