ST_GEOMFROMGEOJSON requires JSON keys to appear in a specific order
(type before geometries/features/geometry/coordinates). When keys
appear in a different order, the JSON scanner is left in an
inconsistent state because json_skip_level() is not called to advance
past the value when the type has not yet been determined.
Fix: add json_skip_level() calls in the geometries, features, and
geometry key handlers when the type key has not yet been encountered.
This mirrors the existing pattern in the coordinates handler.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
Geometry::create_from_json used strings as things that reinitialize
the je (json_engine). By reinitializing with json_start the killed
pointer gets reset, and also the concept of depth is reset.
There are two states in create_from_json:
1. The search if for the type, once found ci is set
2. Search for the argument, arg becomes != T_NONE
If the type is found first, ci become the class, and once
the arg is found, continued execution on the same je occurs.
If the argument comes before the "type", we've save a copy
of the json_engine_t to process this argument.
In both cases je after the execution is the most progressed
pointer.
Debug assertions on je state aren't enforced by code, if
some invalid GeoJSON is passed, these are actual errors.
Because Geometry::create_from_json is recursive, from
geometrymcollections, a json_read_value is pushed from inside this
function up to Item_func_geometry_from_json::val_str.
Other Gis*::init_from_json start from their actual object and don't
need a json_read_value to start.
Test case an bug report thanks to byteoverride.
The byteorder field could contain an invalid value (> 1) on a GIS
multi_point object where there byteorder was on any of the inner points.
This can occur with any of the GIS "*FromWKB" functions that contained
a multipoint object.
This result in a invalid object being accepted and also potentially
trigger undefined behaviour in the processing of the object.
Geometry::as_wkt() didn't take into account bytes written into the
result string in the recusrive call of get_data_as_wkt().
Gis_geometry_collection::get_data_as_wkt() didn't reserve space at all.
Thus nested geometry collections could overflow the buffer.
before `data+= WKB_HEADER_SIZE` verify that there's enough data.
rewrite this line in Gis_multi_line_string::get_mbr to make clear
that it does not need this check.
Parsing a wkb of a multi_line_string needs to validate the
wkbByteOrder field for <= wkb_ndr to be valid, otherwise
return null.
Note that wkb[0] is a char, so can be signed, hence the cast and
> 0x80 values in the test case.
When creating Geometry objects from raw bytes, validate the byte order
is not > wkb_ndr, a constant for little endian being the upper bound of
the wkbByteOrder.
The 3 validation locations are the callers to the other init_from_wkb
functions for other geometry types, which prevent an invalid value from
reaching wkb_get_uint. As wkb[0], a char can signed, use uchar for
comparision. The other value in the wkb_ndr enum is wkb_xdr being 0
which makes > wk_ndr a valid test of the two allowed values.
Geometry::create_from_json reset the killedptr and the stack
depth (stack_p) every time json_scan_start was called. We save/
restore these values so that the ST_GeomFrom_GeoJSON can be killed
and incur timeout, and also so that its depth measurement is kept.
Test case an bug report thanks to byteoverride.
From the UBSAN error:
sql/spatial.cc:3364:10: runtime error: applying non-zero offset 1 to null pointer
In Gis_geometry_collection::init_from_opresult, a pointer argument
was being treated as a counter for the Special case of
GEOMETRYCOLLECTION EMPTY. The memory location was never accessed.
Rather than use points to count and return a difference at the end, the
code is replace to use g_len_total as a counter. This gets a 1 value for
the GEOMETRYCOLLECTION EMPTY case and no ointer undefined behaviours
occur.
As other init_from_opresult functions return uint both g_len and
result use that type.
ST_AsGeoJSON stripped the first '[' bracket when serializing an EMPTY
GeometryCollection, outputting '{"type": "GeometryCollection",
"geometries":]}'.
This occurred because several Gis_*::get_data_as_json functions
unconditionally truncated the last 2 characters of their String buffer
to remove the trailing ', ' appended inside their element loops. For
empty collections, the loop doesn't execute and there is no trailing
comma, causing the truncate to strip the initial '[' bracket.
Fixed by checking if the last character in the buffer is ' ' before
truncating.
The geometry type requires Type:"Feature" but the feature need
not be first in the JSON structure.
Adjust code to return an error if geometry isn't a JSON object,
but continue parsing searching for Type: "Feature" to trigger
the geometry parsing.
Thanks Derick Magnusen for the bug report.
Changes:
- To detect automatic strlen() I removed the methods in String that
uses 'const char *' without a length:
- String::append(const char*)
- Binary_string(const char *str)
- String(const char *str, CHARSET_INFO *cs)
- append_for_single_quote(const char *)
All usage of append(const char*) is changed to either use
String::append(char), String::append(const char*, size_t length) or
String::append(LEX_CSTRING)
- Added STRING_WITH_LEN() around constant string arguments to
String::append()
- Added overflow argument to escape_string_for_mysql() and
escape_quotes_for_mysql() instead of returning (size_t) -1 on overflow.
This was needed as most usage of the above functions never tested the
result for -1 and would have given wrong results or crashes in case
of overflows.
- Added Item_func_or_sum::func_name_cstring(), which returns LEX_CSTRING.
Changed all Item_func::func_name()'s to func_name_cstring()'s.
The old Item_func_or_sum::func_name() is now an inline function that
returns func_name_cstring().str.
- Changed Item::mode_name() and Item::func_name_ext() to return
LEX_CSTRING.
- Changed for some functions the name argument from const char * to
to const LEX_CSTRING &:
- Item::Item_func_fix_attributes()
- Item::check_type_...()
- Type_std_attributes::agg_item_collations()
- Type_std_attributes::agg_item_set_converter()
- Type_std_attributes::agg_arg_charsets...()
- Type_handler_hybrid_field_type::aggregate_for_result()
- Type_handler_geometry::check_type_geom_or_binary()
- Type_handler::Item_func_or_sum_illegal_param()
- Predicant_to_list_comparator::add_value_skip_null()
- Predicant_to_list_comparator::add_value()
- cmp_item_row::prepare_comparators()
- cmp_item_row::aggregate_row_elements_for_comparison()
- Cursor_ref::print_func()
- Removes String_space() as it was only used in one cases and that
could be simplified to not use String_space(), thanks to the fixed
my_vsnprintf().
- Added some const LEX_CSTRING's for common strings:
- NULL_clex_str, DATA_clex_str, INDEX_clex_str.
- Changed primary_key_name to a LEX_CSTRING
- Renamed String::set_quick() to String::set_buffer_if_not_allocated() to
clarify what the function really does.
- Rename of protocol function:
bool store(const char *from, CHARSET_INFO *cs) to
bool store_string_or_null(const char *from, CHARSET_INFO *cs).
This was done to both clarify the difference between this 'store' function
and also to make it easier to find unoptimal usage of store() calls.
- Added Protocol::store(const LEX_CSTRING*, CHARSET_INFO*)
- Changed some 'const char*' arrays to instead be of type LEX_CSTRING.
- class Item_func_units now used LEX_CSTRING for name.
Other things:
- Fixed a bug in mysql.cc:construct_prompt() where a wrong escape character
in the prompt would cause some part of the prompt to be duplicated.
- Fixed a lot of instances where the length of the argument to
append is known or easily obtain but was not used.
- Removed some not needed 'virtual' definition for functions that was
inherited from the parent. I added override to these.
- Fixed Ordered_key::print() to preallocate needed buffer. Old code could
case memory overruns.
- Simplified some loops when adding char * to a String with delimiters.
../sql/spatial.cc: In member function ‘double Gis_point::calculate_haversine(const Geometry*, double, int*)’:
../sql/spatial.cc:1093:45: error: ‘y1r’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1093 | res= 2*sphere_radius*asin((sqrt(dlat + cos(y1r)*cos(y2r)*dlong)));
| ~~~^~~~~
../sql/spatial.cc:1092:20: error: ‘x1r’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1092 | dlong= sin((x2r - x1r)/2)*sin((x2r - x1r)/2);
| ~~~~~^~~~~~
../sql/spatial.cc:1093:54: error: ‘y2r’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1093 | res= 2*sphere_radius*asin((sqrt(dlat + cos(y1r)*cos(y2r)*dlong)));
| ~~~^~~~~
../sql/spatial.cc:1092:20: error: ‘x2r’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1092 | dlong= sin((x2r - x1r)/2)*sin((x2r - x1r)/2);
| ~~~~~^~~~~~
c
