Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#4273 closed defect (fixed)

too permissive WKT parser

Reported by: komzpa Owned by: pramsey
Priority: medium Milestone: PostGIS 2.5.2
Component: postgis Version: 2.4.x
Keywords: Cc:

Description (last modified by komzpa)

SELECT name, similarity(name, 'Минск') AS dist FROM planet_osm_point
WHERE name % 'Минск' AND ST_DWithin(ST_Transform(way, 4326)::geography, ST_GeomFromText('POINT(27.5618791 53.902334)::geography', 4326), 50000) ORDER BY dist DESC;

User by mistake added a cast on wrong side of ' ' and we did not get any parse error.

23:06:43 [kom] > select ST_AsEWKT(ST_GeomFromText('POINT(27.5618791 53.902334)::geography', 4326)); 
┌───────────────────────────────────────┐
│               st_asewkt               │
├───────────────────────────────────────┤
│ SRID=4326;POINT(27.5618791 53.902334) │
└───────────────────────────────────────┘
(1 row)

Time: 3,475 ms

Change History (15)

comment:1 by komzpa, 5 years ago

Description: modified (diff)

comment:2 by pramsey, 5 years ago

Really anything at all after the close of a valid geometry seems to be OK as long as it's not a WKB token:

select 'POINT(27.5618791 53.902334) asdf'::geometry;

But if you put in a token, nope:

select 'POINT(27.5618791 53.902334) POINT'::geometry;
select 'POINT(27.5618791 53.902334) )'::geometry;
select 'POINT(27.5618791 53.902334) ,'::geometry;
Last edited 5 years ago by pramsey (previous) (diff)

comment:3 by pramsey, 5 years ago

This gets weirder… a test case like

SELECT 'POINT(1 2) foobar'::geometry

will be parsed by PostGIS happily, but if I take that same case and run it in cunit, I get

parse error - invalid geometry

comment:4 by pramsey, 5 years ago

In 17127:

Tighten up parsing to account for strings
with junk after valid geometry
References #4273

comment:5 by pramsey, 5 years ago

In 17128:

Fix other test cases and move change closer to
the problem.
References #4273

comment:6 by pramsey, 5 years ago

In 17129:

Move fix further into parser, fix bad test case
References #4273

comment:7 by pramsey, 5 years ago

In 17130:

Tighter parsing of WKT
References #4273

comment:8 by pramsey, 5 years ago

Resolution: fixed
Status: newclosed

comment:9 by Algunenano, 5 years ago

Resolution: fixed
Status: closedreopened

This added a memory leak (detected in Travis): https://travis-ci.org/postgis/postgis/jobs/477506330

==6378== 
==6378== HEAP SUMMARY:
==6378==     in use at exit: 7,977 bytes in 200 blocks
==6378==   total heap usage: 126,917 allocs, 126,717 frees, 6,896,829 bytes allocated
==6378== 
==6378== 120 (32 direct, 88 indirect) bytes in 1 blocks are definitely lost in loss record 198 of 200
==6378==    at 0x483577F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6378==    by 0x486D674: lwpoint_construct (lwpoint.c:137)
==6378==    by 0x4884C56: wkt_parser_point_new (lwin_wkt.c:345)
==6378==    by 0x4882724: wkt_yyparse (lwin_wkt_parse.y:514)
==6378==    by 0x4882CE5: lwgeom_parse_wkt (lwin_wkt_parse.y:68)
==6378==    by 0x14361D: cu_wkt_in (cu_in_wkt.c:56)
==6378==    by 0x14442B: test_wkt_in_point (cu_in_wkt.c:71)
==6378==    by 0x4933336: ??? (in /usr/lib/x86_64-linux-gnu/libcunit.so.1.0.1)
==6378==    by 0x493356C: ??? (in /usr/lib/x86_64-linux-gnu/libcunit.so.1.0.1)
==6378==    by 0x49339DD: CU_run_all_tests (in /usr/lib/x86_64-linux-gnu/libcunit.so.1.0.1)
==6378==    by 0x110432: main (cu_tester.c:177)

comment:10 by pramsey, 5 years ago

In 17131:

Remove memory leak in "junky wkt" case
References #4273

comment:11 by pramsey, 5 years ago

In 17132:

Fix memory leak in error case when valid
object is followed by junk text
References #4273

comment:12 by pramsey, 5 years ago

Resolution: fixed
Status: reopenedclosed

In 17133:

Fix memory leak in case where valid WKT is
followed by garbage content
Closes #4273

comment:13 by pramsey, 5 years ago

In 17193:

Actually put fix for #4273 in the right place
so it isn't blown away when parser.y is evaluated.
References #4273

comment:14 by pramsey, 5 years ago

In 17194:

Actually put fix for #4273 in the right place
so it isn't blown away when parser.y is evaluated.
References #4273

comment:15 by pramsey, 5 years ago

In 17197:

Put fix for parser into y file
References #4273

Note: See TracTickets for help on using tickets.