JSON_MERGE_PATCH reads freed memory and returns NULL blaming an argument that holds no JSON

JSON_MERGE_PATCH() takes a whole document over, rather than merging into
it, when every argument before it was SQL NULL and the document is not an
object. It takes that document over by pointing at the buffer it arrived in.
The next argument is then evaluated into that same buffer, and an argument
needing more room leaves the buffer somewhere else and releases the old one.
The taken-over document is read from the old address anyway.

The result is a read of freed heap memory. What surfaces is a wrong answer:
NULL together with a syntax error naming argument 1, which in these queries
is the four characters NULL and holds no JSON text at all.

Whether it happens depends on how the arguments are spelled, not on what they
mean, so the same query gives two different answers.

How to repeat

SELECT JSON_MERGE_PATCH(NULL, '[1]', '{"aaaaaaaaaaaaaaaa":1}') AS literal;

literal

{"aaaaaaaaaaaaaaaa": 1}

The same query with the arguments computed instead of written out:

SELECT JSON_MERGE_PATCH(NULL, LOWER('[1]'),

                        LOWER('{"AAAAAAAAAAAAAAAA":1}')) AS built;

built

NULL

Warnings:

Warning  4038  Syntax error in JSON text in argument 1 to function 'json_merge_patch' at position 1

CONCAT and REVERSE show it in the same way:

SELECT JSON_MERGE_PATCH(NULL, CONCAT('[', '1]'),

                        CONCAT('{"aaaaaaaaaaaaaaaa"', ':1}')) AS concatenated;

SELECT JSON_MERGE_PATCH(NULL, REVERSE(']1['),

                        REVERSE('}1:"aaaaaaaaaaaaaaaa"{')) AS reversed;

concatenated

NULL

reversed

NULL

Out of a table, where the arguments change from row to row:

CREATE TABLE t (id INT, a VARCHAR(64), b VARCHAR(64));

INSERT INTO t VALUES

  (1, '[1]', '{"aaaaaaaaaaaaaaaa":1}'),

  (2, '[2]', '{"bbbbbbbbbbbbbbbb":2}');

SELECT id, JSON_MERGE_PATCH(NULL, a, b) AS plain FROM t ORDER BY id;

SELECT id, JSON_MERGE_PATCH(NULL, LOWER(a), LOWER(b)) AS built FROM t ORDER BY id;

id  plain

1   {"aaaaaaaaaaaaaaaa": 1}

2   {"bbbbbbbbbbbbbbbb": 2}

id  built

1   NULL

2   NULL

Warnings:

Warning  4038  Syntax error in JSON text in argument 1 to function 'json_merge_patch' at position 1

Warning  4037  Unexpected end of JSON text in argument 1 to function 'json_merge_patch'

What decides whether it happens

Three things have to line up.

1. Every argument before the taken-over one is SQL NULL, which is what puts
   the function into the state where it takes a document over instead of
   merging into one.
2. The taken-over document is not an object. An object is merged rather than
   taken over, so objects are unaffected. Every other kind is affected -
   number, string, boolean, array and null.
3. There is a further argument after it which needs more room than it did.

The third is why the spelling of the argument decides the answer. An argument
is evaluated into a buffer supplied by JSON_MERGE_PATCH, and whether a
function writes into that buffer or hands back a piece of its own argument is
a property of that function:

So whether JSON_MERGE_PATCH is correct depends on an implementation
detail of whichever function produced its argument.

Root cause

Item_func_json_merge_patch::val_str() takes the document over without
copying it:

/* sql/item_jsonfunc.cc */

if (merge_to_null)

{

  if (json_read_value(&je2))

    goto error_return;

  if (je2.value_type == JSON_VALUE_OBJECT)

    goto cont_point;

  merge_to_null= false;

  str->set(js2->ptr(), js2->length(), js2->charset());

  goto cont_point;

}

String::set(const char *, size_t) does not copy. It releases whatever the
String was holding and stores the pointer it was handed:

/* sql/sql_string.h */

inline void set(const char *str, size_t length)

{

  free_buffer();

  Ptr= (char*) str;

  str_length= (uint32) length;

  Alloced_length= 0;

}

js2 at that point is the function's own scratch String, the one every
argument is evaluated into:

js2= args[n_arg]->val_json(&tmp_js2);

The swap that follows makes the taken-over document the one to be merged
into, so it is now a pointer into the buffer that the next argument is about
to be evaluated into:

cont_point:

  {

    /* Swap str and js1. */

    if (str == &tmp_js1)

    { str= js1; js1= &tmp_js1; }

    else

    { js1= str; str= &tmp_js1; }

  }

On the next turn of the loop the next argument is evaluated first and the
taken-over document is read afterwards:

js2= args[n_arg]->val_json(&tmp_js2);        /* may move the buffer */

...

json_scan_start(&je1, js1->charset(), (const uchar *) js1->ptr(),

                (const uchar *) js1->ptr() + js1->length());

The buffer really is moved, not merely overwritten

Traced on a debug build of 10.11, printing the scratch String, its buffer
and the taken-over pointer, for the LOWER case above:

at the take-over : scratch String 0x7f42c8016608, buffer 0x7f42c816e4a8, length 3

at the next read : merged-into pointer 0x7f42c816e4a8, length 3

                   scratch String 0x7f42c8016608, buffer 0x7f42c8205118

The scratch String is the same object throughout, and js2 at the
take-over is that same object, so the document taken over is the scratch
buffer itself. That buffer moved from 0x7f42c816e4a8 to
0x7f42c8205118, and the read went to the old address. The old block had
been released by then.

A larger jump behaves the same way - a one-character document taken over,
followed by a 508-character one:

SELECT JSON_MERGE_PATCH(NULL, LOWER('1'),

                        LOWER(CONCAT('{"k":"', REPEAT('x', 500), '"}')))

       IS NULL AS is_null;

is_null

1

Warnings:

Warning  4038  Syntax error in JSON text in argument 1 to function 'json_merge_patch' at position 1

The reported position and argument come from reading whatever the allocator
has since put at the old address, which is why they bear no relation to the
statement.

Affected versions

Introduced by cd16d6d518761d144844f9f6294744f2aa42c715 (2019-05-17,
"MDEV-13992 Implement JSON_MERGE_PATCH"), the commit that added the function.
First released in 10.2.25.

The line is present unchanged at the tips of 10.5, 10.6, 10.11, 11.4, 11.8,
12.0, 12.1, 12.2, 12.3 and main:

str->set(js2->ptr(), js2->length(), js2->charset());

Every release from 10.2.25 onward is therefore expected to be affected.

Verified on 10.11 only (10.11.19-MariaDB-debug). Other branches were read
but not run.

Note

Found while auditing the JSON functions for an unrelated change. Nothing in
the test suite reaches it: a build which copies the buffer instead of pointing
at it runs the whole of main and json green with no recorded result
changing, so no existing test depends on the current behaviour either way.

Tests written with literal arguments cannot reach this defect at all, since a
literal is never evaluated into the caller's buffer.