From e7c48834ff1827df1a35e542d1682ec55f3fe46d Mon Sep 17 00:00:00 2001 From: Georgi Kodinov Date: Wed, 27 Mar 2013 16:03:00 +0200 Subject: Bug #16451878: GEOMETRY QUERY CRASHES SERVER The GIS WKB reader was checking for the presence of enough data by first multiplying the number read (where it could overflow) and only then comparing it to the number of bytes available. This can overflow and effectively turn off the check. Fixed by: 1. Introducing a new function that does division only so no overflow is possible. 2. Using the proper macros and parenthesizing them. 3. Doing an in-line division check in the only place where the boundary check is done over a data structure other than a dense points array. --- sql/spatial.cc | 53 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 20 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index b1cfd0e5d87..74fc863a18b 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -1,5 +1,5 @@ /* - Copyright (c) 2002, 2012, Oracle and/or its affiliates. All rights reserved. + Copyright (c) 2002, 2013, Oracle and/or its affiliates. All rights reserved. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -395,13 +395,19 @@ const char *Geometry::get_mbr_for_points(MBR *mbr, const char *data, uint offset) const { uint32 points; + size_t points_available; /* read number of points */ if (no_data(data, 4)) return 0; points= uint4korr(data); data+= 4; - if (no_data(data, (SIZEOF_STORED_DOUBLE * 2 + offset) * points)) + /* can't use any of the helper functions due to the offset */ + points_available= + data <= m_data_end ? + (m_data_end - data) / (POINT_DATA_SIZE + offset) : 0; + + if (points_available < points) return 0; /* Calculate MBR for points */ @@ -557,7 +563,7 @@ bool Gis_line_string::get_data_as_wkt(String *txt, const char **end) const data += 4; if (n_points < 1 || - no_data(data, SIZEOF_STORED_DOUBLE * 2 * n_points) || + not_enough_points(data, n_points) || txt->reserve(((MAX_DIGITS_IN_DOUBLE + 1)*2 + 1) * n_points)) return 1; @@ -594,7 +600,7 @@ 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 || not_enough_points(data, n_points)) return 1; get_point(&prev_x, &prev_y, data); @@ -628,8 +634,7 @@ int Gis_line_string::is_closed(int *closed) const return 0; } data+= 4; - if (n_points == 0 || - no_data(data, SIZEOF_STORED_DOUBLE * 2 * n_points)) + if (n_points == 0 || not_enough_points(data, n_points)) return 1; /* Get first point */ @@ -798,7 +803,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 (not_enough_points(data, n_points) || txt->reserve(2 + ((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; txt->qs_append('('); @@ -852,7 +858,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 (no_data(data, (SIZEOF_STORED_DOUBLE*2) * n_points)) + if (not_enough_points(data, n_points)) return 1; get_point(&prev_x, &prev_y, data+4); data+= (4+SIZEOF_STORED_DOUBLE*2); @@ -888,7 +894,7 @@ 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 (not_enough_points(data, n_points) || result->reserve(1+4+4+ length)) return 1; result->q_append((char) wkb_ndr); @@ -934,7 +940,7 @@ int Gis_polygon::interior_ring_n(uint32 num, String *result) const n_points= uint4korr(data); points_size= n_points * POINT_DATA_SIZE; data+= 4; - if (no_data(data, points_size) || result->reserve(1+4+4+ points_size)) + if (not_enough_points(data, n_points) || result->reserve(1+4+4+ points_size)) return 1; result->q_append((char) wkb_ndr); @@ -973,7 +979,7 @@ 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 (not_enough_points(data, n_points)) return 1; get_point(&prev_x, &prev_y, data); data+= (SIZEOF_STORED_DOUBLE*2); @@ -1098,15 +1104,22 @@ uint Gis_multi_point::init_from_wkb(const char *wkb, uint len, wkbByteOrder bo, bool Gis_multi_point::get_data_as_wkt(String *txt, const char **end) const { uint32 n_points; - if (no_data(m_data, 4)) + size_t points_available; + const char *data= m_data; + + if (no_data(data, 4)) return 1; - n_points= uint4korr(m_data); - if (no_data(m_data+4, - n_points * (SIZEOF_STORED_DOUBLE * 2 + WKB_HEADER_SIZE)) || + n_points= uint4korr(data); + data+= 4; + points_available= data <= m_data_end ? + (m_data_end - data) / (POINT_DATA_SIZE + WKB_HEADER_SIZE) : 0; + + /* can't use any of the helper functions due to WKB_HEADER_SIZE */ + if (n_points > points_available || txt->reserve(((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; - *end= append_points(txt, n_points, m_data+4, WKB_HEADER_SIZE); + *end= append_points(txt, n_points, data, WKB_HEADER_SIZE); txt->length(txt->length()-1); // Remove end ',' return 0; } @@ -1260,7 +1273,7 @@ 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 (not_enough_points(data, n_points) || txt->reserve(2 + ((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; txt->qs_append('('); @@ -1320,8 +1333,8 @@ int Gis_multi_line_string::geometry_n(uint32 num, String *result) const if (no_data(data, WKB_HEADER_SIZE + 4)) return 1; n_points= uint4korr(data + WKB_HEADER_SIZE); - length= WKB_HEADER_SIZE + 4+ POINT_DATA_SIZE * n_points; - if (no_data(data, length)) + length= WKB_HEADER_SIZE + 4 + POINT_DATA_SIZE * n_points; + if (not_enough_points(data + WKB_HEADER_SIZE + 4, n_points)) return 1; if (!--num) break; @@ -1521,7 +1534,7 @@ 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 (not_enough_points(data, n_points) || txt->reserve(2 + ((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points, 512)) return 1; -- cgit v1.2.1 From e927bda69f5213725c95615641db1bf511a9fcab Mon Sep 17 00:00:00 2001 From: Georgi Kodinov Date: Thu, 28 Mar 2013 17:37:29 +0200 Subject: Addendum #1 to the fix for bug #16451878 : GEOMETRY QUERY CRASHES SERVER Fixed the get_data_size() methods for multi-point features to check properly for end of their respective data arrays. Extended the point checking function to take a 3d optional argument so cases where there's additional data in each array element (besides the point data itself) can be covered by the helper function. Fixed the 3 cases where such offset was present to use the proper checking helper function. Test cases added. Fixed review comments. --- sql/spatial.cc | 67 +++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 19 deletions(-) (limited to 'sql/spatial.cc') diff --git a/sql/spatial.cc b/sql/spatial.cc index 74fc863a18b..ba1cbbcb85e 100644 --- a/sql/spatial.cc +++ b/sql/spatial.cc @@ -395,19 +395,13 @@ const char *Geometry::get_mbr_for_points(MBR *mbr, const char *data, uint offset) const { uint32 points; - size_t points_available; /* read number of points */ if (no_data(data, 4)) return 0; points= uint4korr(data); data+= 4; - /* can't use any of the helper functions due to the offset */ - points_available= - data <= m_data_end ? - (m_data_end - data) / (POINT_DATA_SIZE + offset) : 0; - - if (points_available < points) + if (not_enough_points(data, points, offset)) return 0; /* Calculate MBR for points */ @@ -490,9 +484,16 @@ const Geometry::Class_info *Gis_point::get_class_info() const uint32 Gis_line_string::get_data_size() const { + size_t n_points; if (no_data(m_data, 4)) return GET_SIZE_ERROR; - return 4 + uint4korr(m_data) * POINT_DATA_SIZE; + + n_points= uint4korr(m_data); + + if (not_enough_points(m_data + 4, n_points)) + return GET_SIZE_ERROR; + + return 4 + n_points * POINT_DATA_SIZE; } @@ -705,10 +706,18 @@ uint32 Gis_polygon::get_data_size() const while (n_linear_rings--) { + size_t n_points; if (no_data(data, 4)) return GET_SIZE_ERROR; - data+= 4 + uint4korr(data)*POINT_DATA_SIZE; + n_points= uint4korr(data); + data+= 4; + + if (not_enough_points(data, n_points)) + return GET_SIZE_ERROR; + + data+= n_points * POINT_DATA_SIZE; } + return (uint32) (data - m_data); } @@ -1038,9 +1047,17 @@ const Geometry::Class_info *Gis_polygon::get_class_info() const uint32 Gis_multi_point::get_data_size() const { + size_t n_points; + if (no_data(m_data, 4)) return GET_SIZE_ERROR; - return 4 + uint4korr(m_data)*(POINT_DATA_SIZE + WKB_HEADER_SIZE); + + n_points= uint4korr(m_data); + + if (not_enough_points(m_data + 4, n_points, WKB_HEADER_SIZE)) + return GET_SIZE_ERROR; + + return 4 + n_points * (POINT_DATA_SIZE + WKB_HEADER_SIZE); } @@ -1104,7 +1121,6 @@ uint Gis_multi_point::init_from_wkb(const char *wkb, uint len, wkbByteOrder bo, bool Gis_multi_point::get_data_as_wkt(String *txt, const char **end) const { uint32 n_points; - size_t points_available; const char *data= m_data; if (no_data(data, 4)) @@ -1112,13 +1128,11 @@ bool Gis_multi_point::get_data_as_wkt(String *txt, const char **end) const n_points= uint4korr(data); data+= 4; - points_available= data <= m_data_end ? - (m_data_end - data) / (POINT_DATA_SIZE + WKB_HEADER_SIZE) : 0; - /* can't use any of the helper functions due to WKB_HEADER_SIZE */ - if (n_points > points_available || + if (not_enough_points(data, n_points, WKB_HEADER_SIZE) || txt->reserve(((MAX_DIGITS_IN_DOUBLE + 1) * 2 + 1) * n_points)) return 1; + *end= append_points(txt, n_points, data, WKB_HEADER_SIZE); txt->length(txt->length()-1); // Remove end ',' return 0; @@ -1177,10 +1191,18 @@ uint32 Gis_multi_line_string::get_data_size() const while (n_line_strings--) { + size_t n_points; + if (no_data(data, WKB_HEADER_SIZE + 4)) return GET_SIZE_ERROR; - data+= (WKB_HEADER_SIZE + 4 + uint4korr(data + WKB_HEADER_SIZE) * - POINT_DATA_SIZE); + + n_points= uint4korr(data + WKB_HEADER_SIZE); + data+= WKB_HEADER_SIZE + 4; + + if (not_enough_points(data, n_points)) + return GET_SIZE_ERROR; + + data+= n_points * POINT_DATA_SIZE; } return (uint32) (data - m_data); } @@ -1432,9 +1454,16 @@ uint32 Gis_multi_polygon::get_data_size() const while (n_linear_rings--) { + size_t n_points; if (no_data(data, 4)) - return GET_SIZE_ERROR; - data+= 4 + uint4korr(data) * POINT_DATA_SIZE; + return GET_SIZE_ERROR; + n_points= uint4korr(data); + data+= 4; + + if (not_enough_points(data, n_points)) + return GET_SIZE_ERROR; + + data+= n_points * POINT_DATA_SIZE; } } return (uint32) (data - m_data); -- cgit v1.2.1