Analysis:
So, there were two problems that needed to be fixed.
1) To fix the crash.
2) After fixing the crash, the result was wrong.
Reason for crash: When we pass the hash to get_intersect_between_arrays(),
We were initialially not passing it value, so the operations were not
performed correctly.
Reason for wrong result: The number of rows that it was returning were same
as that in the table, but, only the first row had correct ouput, rest of
them were NULL (it should also be the result of interection). This was
because we modified the "items" HASH by deleting the "seen" elements.
So for next rows, it did not have the elements it should have in the hash.
Fix:
1) To fix the crash: pass the HASH by reference
2) To fix incorrect result: Maintain a separate "seen" hash, if an item
is found the the "items" hash, delete it ony temporarily and put it in the
seen hash. At then end, put the items from "seen" back into "items" and
reset "seen".
called with empty json arrays, UBSAN runtime error: member access within
null pointer of type 'struct String' in
Item_func_json_array_intersect::prepare_json_and_create_hash
Analysis:
Arguments are not initilized
Fix:
If the arguments are not initialized the the val_json() return NULL, so
if val_json() for either of the arguments, return NULL.
Regression from MDEV-36765 / 2b24ed87f0.
json_unescape can return a string length 0 without it being an error.
The regression caused this 0 length empty string to appear as an
error and result in a NULL return value.
numerous bugs in JSON_DETAILED and multibyte charsets:
* String:chop() must be charset-aware and not simply length--
* String::append(char) must be charset-aware and not simply length++
* json_nice() first removes value_len bytes, then a
certain number of characters
Clang processes the "int x=x" code from UNINIT_VAR
literally resulting in an uninitialized read and write.
This is something we want to avoid. Gcc does the same
without emitting warnings.
As the UNINIT_VAR was around avoiding compiler false detection,
and clang doesn't false detect, is default action is a
noop.
Static analysers (examined Infer and SonarQube) are
clang based and have the same detection.
Using a __clang__ instead of WITH_UBSAN would acheived
a better result, however reviewer wanted to keep WITH_UBSAN
only.
LINT_INIT_STRUCT is no longer required, even a gcc-4.8.5
doesn't warn with this construct removed which matches
the comment that it was fixed in gcc ~4.7.
mysql.cc - all paths in com_go populate buff before use.
json: Item_func_json_merge::val_str
LINT_INIT(js2) unneeded as usage in the previous statements
it is explicitly initialized to NULL.
Item_func_json_contains_path::val_bool n_found is guarded
by an uninitialized read by mode_one and from
gcc-13.3.0 in Ubuntu 24.04 this is detected. As the only
remaining use of LINIT_INIT this usage has been applied
with the expanded macro with the unused _lint define removed.
The LINT_INIT macro is removed.
_ma_ck_delete - org_key only valid under share->now_transactional
likewise with _ma_ck_write_btree_with_log
connect engine never used anything that FORCE_INIT_OF_VARS
would change.
Reviewer: Monty
Since MDEV-33209 (09ea2dc788)
the the stack overflow errors are just injected instead of
frailer mechanisms to consume stack. These mechanims where
not carried forward to the JSON_TABLE or JSON_SCHEMA_VALID where
the pattern was the same.
add_extra_deps also no-longer recursively iterates in
out of stack conditions.
Tests performed in json_debug_nonembedded(_noasan).
main.json_debug_nonembedded_noasan fails because of stack
overrun on Debug + MSAN testing.
Since MDEV-33209 (09ea2dc788)
the the stack overflow errors are just injected instead of
frailer mechanisms to consume stack. These mechanims where
not carried forward to the JSON_TABLE functions where
the pattern was the same.
Related MDEV-34099 (cf1c381bb8) makes check_stack_overrun never fail
under Address Sanitizer (only).
The previous ALLOCATE_MEM_ON_STACK did in MemorySanitizer consume
memory, but check_stack_overrun did fail because its 16000 byte
safety margin was exceeded. The allocation of the 448 byte error
ER_STACK_OVERRUN_NEED_MORE is well within these bounds, however
under the safemalloc implementation, "backtrace" library call is called,
which does further allocation for every stack frame. This exceeds the stack.
Fixes:
JSON_TABLE functions that trigger on out of memory debug instrumentation
replaced with the mechanism from MDEV-33209.
The get_disallowed_table_deps_for_list in a non-Debug build returned
incorrectly 1, instead of -1 indicating the out of memory condition.
In json_table add_extra_deps never passed the out of memory error
condition to the caller and would continue to run in a loop, potentially
recursively under these near out of stack conditions.
The Memory, Undefined Behaviour, Address and Thread sanitizers provide
sufficient instrumentation and a backtrace so the safemalloc
functionality provides insufficent value with these. As such is
disabled under WITH_SAFEMALLOC=AUTO.
With all of thse corrected the main.json_debug_nonembedded_noasan no
longer needs its ASAN exclusion.
The JSON_TABLE tests in this test case was dropped in a merge from 10.6
so these tests are re-added.
- Add a testcase showing JSON_HB histograms handle multi-byte characters
correctly.
- Make Item_func_json_unquote::val_str() handle situation where
it is reading non-UTF8 "JSON" and transcoding it into UTF-8.
(the JSON spec only allows UTF8 but MariaDB's implementation
supports non-UTF8 as well)
- Make Item_func_json_search::compare_json_value_wild() handle
json_unescape()'s return values in the same way its done in other
places.
- Coding style fixes.
Using report_json_error was incorrect as errors
in the je have already been handled earlier in the
json function.
The errors related to json_unescape are handled with
consistently with other functions.
Now pushes the OUTOFMEMORY error and ER_JSON_BAD_CHAR as a warning
if these resulted in those errors.
callers only expected a bool so the prototype was changed.
Json_engine_scan::check_and_get_value_scalar failed to handle the error
condition so set the *error if an error occured and return the correct
value.
JSON functions append in multiple ways, however there isn't always error
handling, and many time it doesn't make it to the end user.
Made the appending string functions withing item_jsonfunc warn if their
true/false result (did an error occur) isn't handled.
Add error handling to many json functions.
realloc_with_extra_if_needed was also previously lacking OOM handing.
Push a warning if the unescaping failed to resolve into the
target character set.
This uses the ER_JSON_BAD_CHAR, which is normally around functions,
but we can't add new error codes so use this as is. Use same args
for the error as JSON functions would for this error code.
Search conditions were evaluated using val_int(), which was wrong.
Fixing the code to use val_bool() instead.
Details:
- Adding a new item_base_t::IS_COND flag which marks Items used
as <search condition> in WHERE, HAVING, JOIN ON, CASE WHEN clauses.
The flag is at the parse time.
These expressions must be evaluated using val_bool() rather than val_int().
Note, the optimizer creates more Items which are used as search conditions.
Most of these items are not marked with IS_COND yet. This is OK for now,
but eventually these Items can also be fixed to have the flag.
- Adding a method Item::is_cond() which tests if the Item has the IS_COND flag.
- Implementing Item_cache_bool. It evaluates the cached expression using
val_bool() rather than val_int().
Overriding Type_handler_bool::Item_get_cache() to create Item_cache_bool.
- Implementing Item::save_bool_in_field(). It uses val_bool() rather than
val_int() to evaluate the expression.
- Implementing Type_handler_bool::Item_save_in_field()
using Item::save_bool_in_field().
- Fixing all Item_bool_func descendants to implement a virtual val_bool()
rather than a virtual val_int().
- To find places where val_int() should be fixed to val_bool(), a few
DBUG_ASSERT(!is_cond()) where added into val_int() implementations
of selected (most frequent) classes:
Item_field
Item_str_func
Item_datefunc
Item_timefunc
Item_datetimefunc
Item_cache_bool
Item_bool_func
Item_func_hybrid_field_type
Item_basic_constant descendants
- Fixing all places where DBUG_ASSERT() happened during an "mtr" run
to use val_bool() instead of val_int().
Analysis:
The value gets appended as string instead of unescaped json value
Fix:
Append the value of json in a temporary string and then store it in the
field instead of directly storing as string.
non-default collation_connection
Analysis:
Due to different collation, the string has nothing to chop off.
Fix:
Got rid of chop(), only append " ," only when we have more elements to
add to the result.
Analysis:
When we scan json to get to a beginning according to the path, we end up
scanning json even if we have exhausted it. When eventually returns error.
Fix:
Continue scanning json only if we have not exhausted it and return result
accordingly.
Analysis:
When scanning json and getting the exact path at each step, if a path
is reached, we end up adding the item in the result and immediately get the
next item which results in current path changing.
Fix:
Instead of immediately returning the item, count the occurences of the path
in argument and append in the result as needed.
(returns NULL) and for Date/DateTime returns "INTEGER"
Analysis:
When the first character of json is scanned it is number. Based on that
integer is returned.
Fix:
Scan rest of the json before returning the final result to ensure json is
valid in the first place in order to have a valid type.
Json test about max statement time fails with freebsd because on some
architectures the test might execute faster and the statement may not fail.
To simulate failure regardless of architecture, introduce a wait of seconds
longer than the max_statement_time.
Some fixes related to commit f838b2d799 and
Rows_log_event::do_apply_event() and Update_rows_log_event::do_exec_row()
for system-versioned tables were provided by Nikita Malyavin.
This was required by test versioning.rpl,trx_id,row.
In the JSON functions, the debug injection for stack overflows is
inaccurate and may cause actual stack overflows. Let us simply
inject stack overflow errors without actually relying on the ability
of check_stack_overrun() to do so.
Reviewed by: Rucha Deodhar
