From 511b9432637510617b04bde92c51a184c1e3aea8 Mon Sep 17 00:00:00 2001 From: Alexey Botchkov Date: Sun, 10 Mar 2013 23:08:05 +0400 Subject: MDEV-4252 geometry query crashes server. The bug was found by Alyssa Milburn. If the number of points of a geometry feature read from binary representation is greater than 0x10000000, then the (uint32) (num_points * 16) will cut the higher byte, which leads to various errors. Fixed by additional check if (num_points > max_n_points). --- sql/spatial.cc | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index eec028eaef1..94d0238993c 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -556,7 +556,7 @@ bool Gis_line_string::get_data_as_wkt(String *txt, const char **end) const n_points= uint4korr(data); data += 4; - if (n_points < 1 || + if (n_points < 1 || n_points > max_n_points || no_data(data, SIZEOF_STORED_DOUBLE * 2 * n_points) || txt->reserve(((MAX_DIGITS_IN_DOUBLE + 1)*2 + 1) * n_points)) return 1; @@ -594,7 +594,8 @@ int Gis_line_string::geom_length(double *len) const return 1; n_points= uint4korr(data); data+= 4; - if (n_points < 1 || no_data(data, SIZEOF_STORED_DOUBLE * 2 * n_points)) + if (n_points < 1 || n_points > max_n_points || + no_data(data, SIZEOF_STORED_DOUBLE * 2 * n_points)) return 1; get_point(&prev_x, &prev_y, data); @@ -628,7 +629,7 @@ int Gis_line_string::is_closed(int *closed) const return 0; } data+= 4; - if (n_points == 0 || + if (n_points == 0 || n_points > max_n_points || no_data(data, SIZEOF_STORED_DOUBLE * 2 * n_points)) return 1; @@ -798,7 +799,8 @@ bool Gis_polygon::get_data_as_wkt(String *txt, const char **end) const return 1; n_points= uint4korr(data); data+= 4; - if (no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points) || + if (n_points > max_n_points || + no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points) || txt->reserve(2 + ((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; txt->qs_append('('); @@ -852,7 +854,8 @@ int Gis_polygon::area(double *ar, const char **end_of_data) const if (no_data(data, 4)) return 1; n_points= uint4korr(data); - if (no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) + if (n_points > max_n_points || + no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) return 1; get_point(&prev_x, &prev_y, data+4); data+= (4+SIZEOF_STORED_DOUBLE*2); @@ -888,7 +891,8 @@ int Gis_polygon::exterior_ring(String *result) const n_points= uint4korr(data); data+= 4; length= n_points * POINT_DATA_SIZE; - if (no_data(data, length) || result->reserve(1+4+4+ length)) + if (n_points > max_n_points || + no_data(data, length) || result->reserve(1+4+4+ length)) return 1; result->q_append((char) wkb_ndr); @@ -973,7 +977,8 @@ int Gis_polygon::centroid_xy(double *x, double *y) const return 1; org_n_points= n_points= uint4korr(data); data+= 4; - if (no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) + if (n_points > max_n_points || + no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) return 1; get_point(&prev_x, &prev_y, data); data+= (SIZEOF_STORED_DOUBLE*2); @@ -1260,7 +1265,8 @@ bool Gis_multi_line_string::get_data_as_wkt(String *txt, return 1; n_points= uint4korr(data + WKB_HEADER_SIZE); data+= WKB_HEADER_SIZE + 4; - if (no_data(data, n_points * (SIZEOF_STORED_DOUBLE*2)) || + if (n_points > max_n_points || + no_data(data, n_points * (SIZEOF_STORED_DOUBLE*2)) || txt->reserve(2 + ((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; txt->qs_append('('); @@ -1521,7 +1527,8 @@ bool Gis_multi_polygon::get_data_as_wkt(String *txt, const char **end) const return 1; uint32 n_points= uint4korr(data); data+= 4; - if (no_data(data, (SIZEOF_STORED_DOUBLE * 2) * n_points) || + if (n_points > max_n_points || + no_data(data, (SIZEOF_STORED_DOUBLE * 2) * n_points) || txt->reserve(2 + ((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points, 512)) return 1; @@ -1604,6 +1611,8 @@ int Gis_multi_polygon::geometry_n(uint32 num, String *result) const if (no_data(data, 4)) return 1; n_points= uint4korr(data); + if (n_points > max_n_points) + return 1; data+= 4 + POINT_DATA_SIZE * n_points; } } while (--num); -- cgit v1.2.1 From 589247ae86b25eaa9bd75e4f26ecd06831469311 Mon Sep 17 00:00:00 2001 From: Alexey Botchkov Date: Mon, 18 Mar 2013 17:58:00 +0400 Subject: MDEV-4252 geometry query crashes server. Additional fixes for possible overflows in length-related calculations in 'spatial' implementations. Checks added to the ::get_data_size() methods. max_n_points decreased to occupy less 2G size. An object of that size is practically inoperable anyway. --- sql/spatial.cc | 67 ++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 20 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index 94d0238993c..5a4b768140c 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -394,18 +394,19 @@ const char *Geometry::append_points(String *txt, uint32 n_points, const char *Geometry::get_mbr_for_points(MBR *mbr, const char *data, uint offset) const { - uint32 points; + uint32 n_points; /* read number of points */ if (no_data(data, 4)) return 0; - points= uint4korr(data); + n_points= uint4korr(data); data+= 4; - if (no_data(data, (SIZEOF_STORED_DOUBLE * 2 + offset) * points)) + if (n_points > max_n_points || + no_data(data, (POINT_DATA_SIZE + offset) * n_points)) return 0; /* Calculate MBR for points */ - while (points--) + while (n_points--) { data+= offset; mbr->add_xy(data, data + SIZEOF_STORED_DOUBLE); @@ -484,9 +485,12 @@ const Geometry::Class_info *Gis_point::get_class_info() const uint32 Gis_line_string::get_data_size() const { - if (no_data(m_data, 4)) + uint32 n_points, size; + if (no_data(m_data, 4) || + (n_points= uint4korr(m_data)) > max_n_points || + no_data(m_data, (size= 4 + n_points * POINT_DATA_SIZE))) return GET_SIZE_ERROR; - return 4 + uint4korr(m_data) * POINT_DATA_SIZE; + return size; } @@ -665,6 +669,9 @@ int Gis_line_string::end_point(String *result) const if (no_data(m_data, 4)) return 1; n_points= uint4korr(m_data); + if (n_points == 0 || n_points > max_n_points || + no_data(m_data, POINT_DATA_SIZE * n_points)) + return 1; return create_point(result, m_data + 4 + (n_points - 1) * POINT_DATA_SIZE); } @@ -674,11 +681,14 @@ int Gis_line_string::point_n(uint32 num, String *result) const uint32 n_points; if (no_data(m_data, 4)) return 1; + num--; n_points= uint4korr(m_data); - if ((uint32) (num - 1) >= n_points) // means (num > n_points || num < 1) + if (num >= n_points || + num > max_n_points || // means (num > n_points || num < 1) + no_data(m_data, num * POINT_DATA_SIZE)) return 1; - return create_point(result, m_data + 4 + (num - 1) * POINT_DATA_SIZE); + return create_point(result, m_data + 4 + num*POINT_DATA_SIZE); } const Geometry::Class_info *Gis_line_string::get_class_info() const @@ -692,6 +702,7 @@ const Geometry::Class_info *Gis_line_string::get_class_info() const uint32 Gis_polygon::get_data_size() const { uint32 n_linear_rings; + uint32 n_points; const char *data= m_data; if (no_data(data, 4)) @@ -701,10 +712,13 @@ uint32 Gis_polygon::get_data_size() const while (n_linear_rings--) { - if (no_data(data, 4)) + if (no_data(data, 4) || + (n_points= uint4korr(data)) > max_n_points) return GET_SIZE_ERROR; - data+= 4 + uint4korr(data)*POINT_DATA_SIZE; + data+= 4 + n_points*POINT_DATA_SIZE; } + if (no_data(data, 0)) + return GET_SIZE_ERROR; return (uint32) (data - m_data); } @@ -1037,9 +1051,14 @@ const Geometry::Class_info *Gis_polygon::get_class_info() const uint32 Gis_multi_point::get_data_size() const { - if (no_data(m_data, 4)) - return GET_SIZE_ERROR; - return 4 + uint4korr(m_data)*(POINT_DATA_SIZE + WKB_HEADER_SIZE); + uint32 n_points; + uint32 size; + + if (no_data(m_data, 4) || + (n_points= uint4korr(m_data)) > max_n_points || + no_data(m_data, (size= 4 + n_points*(POINT_DATA_SIZE + WKB_HEADER_SIZE)))) + return GET_SIZE_ERROR; + return size; } @@ -1107,7 +1126,8 @@ bool Gis_multi_point::get_data_as_wkt(String *txt, const char **end) const return 1; n_points= uint4korr(m_data); - if (no_data(m_data+4, + if (n_points > max_n_points || + no_data(m_data+4, n_points * (SIZEOF_STORED_DOUBLE * 2 + WKB_HEADER_SIZE)) || txt->reserve(((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; @@ -1160,6 +1180,7 @@ const Geometry::Class_info *Gis_multi_point::get_class_info() const uint32 Gis_multi_line_string::get_data_size() const { uint32 n_line_strings; + uint32 n_points; const char *data= m_data; if (no_data(data, 4)) @@ -1169,11 +1190,13 @@ uint32 Gis_multi_line_string::get_data_size() const while (n_line_strings--) { - if (no_data(data, WKB_HEADER_SIZE + 4)) + if (no_data(data, WKB_HEADER_SIZE + 4) || + (n_points= uint4korr(data + WKB_HEADER_SIZE)) > max_n_points) return GET_SIZE_ERROR; - data+= (WKB_HEADER_SIZE + 4 + uint4korr(data + WKB_HEADER_SIZE) * - POINT_DATA_SIZE); + data+= (WKB_HEADER_SIZE + 4 + n_points*POINT_DATA_SIZE); } + if (no_data(data, 0)) + return GET_SIZE_ERROR; return (uint32) (data - m_data); } @@ -1327,7 +1350,7 @@ int Gis_multi_line_string::geometry_n(uint32 num, String *result) const return 1; n_points= uint4korr(data + WKB_HEADER_SIZE); length= WKB_HEADER_SIZE + 4+ POINT_DATA_SIZE * n_points; - if (no_data(data, length)) + if (n_points > max_n_points || no_data(data, length)) return 1; if (!--num) break; @@ -1407,6 +1430,7 @@ const Geometry::Class_info *Gis_multi_line_string::get_class_info() const uint32 Gis_multi_polygon::get_data_size() const { uint32 n_polygons; + uint32 n_points; const char *data= m_data; if (no_data(data, 4)) @@ -1425,11 +1449,14 @@ uint32 Gis_multi_polygon::get_data_size() const while (n_linear_rings--) { - if (no_data(data, 4)) + if (no_data(data, 4) || + (n_points= uint4korr(data)) > max_n_points) return GET_SIZE_ERROR; - data+= 4 + uint4korr(data) * POINT_DATA_SIZE; + data+= 4 + n_points * POINT_DATA_SIZE; } } + if (no_data(data, 0)) + return GET_SIZE_ERROR; return (uint32) (data - m_data); } -- cgit v1.2.1 From 15a7335d77d056e860a9fdc844343c840e310e68 Mon Sep 17 00:00:00 2001 From: Alexey Botchkov Date: Tue, 19 Mar 2013 17:16:10 +0400 Subject: MDEV-4296 Assertion `n_linear_rings > 0' fails in Gis_polygon::centroid_xy. Forgotten DBUG_ASSERT should be replaced with the 'return error'. --- sql/spatial.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index 5a4b768140c..afaa67763e8 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -972,13 +972,11 @@ int Gis_polygon::centroid_xy(double *x, double *y) const const char *data= m_data; bool first_loop= 1; - if (no_data(data, 4)) + if (no_data(data, 4) || + (n_linear_rings= uint4korr(data)) == 0) return 1; - n_linear_rings= uint4korr(data); data+= 4; - DBUG_ASSERT(n_linear_rings > 0); - while (n_linear_rings--) { uint32 n_points, org_n_points; -- cgit v1.2.1 From ef737284b416292d21837d7dedbffe66a4b4b8d4 Mon Sep 17 00:00:00 2001 From: Alexey Botchkov Date: Tue, 19 Mar 2013 17:25:58 +0400 Subject: MDEV-4295 Server crashes in get_point on a query with Area, AsBinary, MultiPoint. Need to check if the number of points is 0 for the polygon. --- sql/spatial.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index afaa67763e8..52110960f96 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -868,7 +868,7 @@ int Gis_polygon::area(double *ar, const char **end_of_data) const if (no_data(data, 4)) return 1; n_points= uint4korr(data); - if (n_points > max_n_points || + if (n_points == 0 || n_points > max_n_points || no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) return 1; get_point(&prev_x, &prev_y, data+4); @@ -989,7 +989,7 @@ int Gis_polygon::centroid_xy(double *x, double *y) const return 1; org_n_points= n_points= uint4korr(data); data+= 4; - if (n_points > max_n_points || + if (n_points == 0 || n_points > max_n_points || no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) return 1; get_point(&prev_x, &prev_y, data); -- cgit v1.2.1 From 045c498691f77ac8e0d8c8b9b705325b3425c69d Mon Sep 17 00:00:00 2001 From: Alexey Botchkov Date: Tue, 26 Mar 2013 21:47:06 +0400 Subject: GEOMETRYCOLLECTION EMPTY handling fixed. The get_mbr() method shouldn't return the error, rather an invalid MBR in this case. --- sql/spatial.cc | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index 0da2fdf40b3..cec6150c22a 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -308,6 +308,9 @@ bool Geometry::envelope(String *result) const const char *end; if (get_mbr(&mbr, &end)) + return 1; + + if (!mbr.valid()) { /* Empty geometry */ if (result->reserve(1 + 4*2)) @@ -2322,7 +2325,7 @@ bool Gis_geometry_collection::get_mbr(MBR *mbr, const char **end) const n_objects= uint4korr(data); data+= 4; if (n_objects == 0) - return 1; + goto exit; while (n_objects--) { @@ -2339,6 +2342,7 @@ bool Gis_geometry_collection::get_mbr(MBR *mbr, const char **end) const if (geom->get_mbr(mbr, &data)) return 1; } +exit: *end= data; return 0; } @@ -2356,10 +2360,11 @@ int Gis_geometry_collection::area(double *ar, const char **end) const return 1; n_objects= uint4korr(data); data+= 4; - if (n_objects == 0) - return 1; result= 0.0; + if (n_objects == 0) + goto exit; + while (n_objects--) { uint32 wkb_type; @@ -2376,6 +2381,7 @@ int Gis_geometry_collection::area(double *ar, const char **end) const return 1; result+= *ar; } +exit: *end= data; *ar= result; return 0; @@ -2394,10 +2400,11 @@ int Gis_geometry_collection::geom_length(double *len, const char **end) const return 1; n_objects= uint4korr(data); data+= 4; + result= 0.0; + if (n_objects == 0) - return 1; + goto exit; - result= 0.0; while (n_objects--) { uint32 wkb_type; @@ -2414,6 +2421,8 @@ int Gis_geometry_collection::geom_length(double *len, const char **end) const return 1; result+= *len; } + +exit: *end= data; *len= result; return 0; -- cgit v1.2.1