Re: Bug in searching path in jsonb_set when walking through JSONB array

Hello, Hackers!

While I was reviewed a patch with "json_insert" function I found a bug
which wasn't connected with the patch and reproduced at master.

It claims about non-integer whereas input values are obvious integers
and in an allowed range.
More testing lead to understanding it appears when numbers length are
multiplier of 4:

postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}', '"4"');
ERROR: path element at the position 2 is not an integer

postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"b", 1000}', '"4"');
ERROR: path element at the position 2 is not an integer

postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", -999}', '"4"');
ERROR: path element at the position 2 is not an integer

postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a",
10009999}', '"4"');
ERROR: path element at the position 2 is not an integer

Close values are ok:
postgres=# select jsonb_set('{"a":[[],1,2,3]}', '{"a", 0, 999}', '"4"');
jsonb_set
-------------------------
{"a": [["4"], 1, 2, 3]}
(1 row)

postgres=# select jsonb_set('{"a":[[],1,2,3]}', '{"a", 0, 10000}', '"4"');
jsonb_set
-------------------------
{"a": [["4"], 1, 2, 3]}
(1 row)

Research lead to setPathArray where a string which is got via
VARDATA_ANY but is passed to strtol which expects cstring.

In case of string number length is not a multiplier of 4 rest bytes
are padding by '\0', when length is a multiplier of 4 there is no
padding, just garbage after the last digit of the value.

Proposed patch in an attachment fixes it.

There is a magic number "20" as a length of an array for copying key
from a path before passing it to strtol. It is a maximal length of a
value which can be parsed by the function. I could not find a proper
constant for it. Also I found similar direct value in the code (e.g.
in numeric.c).

I've added a comment, I hope it is enough for it.

--
Best regards,
Vitaly Burovoy

On Wed, Mar 23, 2016 at 6:37 AM, Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com>
wrote:

> Hello, Hackers!
>
> While I was reviewed a patch with "json_insert" function I found a bug
> which wasn't connected with the patch and reproduced at master.
>
> It claims about non-integer whereas input values are obvious integers
> and in an allowed range.
> More testing lead to understanding it appears when numbers length are
> multiplier of 4:
>
> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}',
> '"4"');
> ERROR: path element at the position 2 is not an integer
>

Hmm, I see in master

select version();
version
-----------------------------------------------------------------------------------------------------------------
PostgreSQL 9.6devel on x86_64-apple-darwin15.4.0, compiled by Apple LLVM
version 7.3.0 (clang-703.0.29), 64-bit
(1 row)

select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}', '"4"');
jsonb_set
------------------------------------
{"a": [[], 1, 2, 3, "4"], "b": []}
(1 row)

>
> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"b", 1000}',
> '"4"');
> ERROR: path element at the position 2 is not an integer
>
> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", -999}',
> '"4"');
> ERROR: path element at the position 2 is not an integer
>
> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a",
> 10009999}', '"4"');
> ERROR: path element at the position 2 is not an integer
>
> Close values are ok:
> postgres=# select jsonb_set('{"a":[[],1,2,3]}', '{"a", 0, 999}', '"4"');
> jsonb_set
> -------------------------
> {"a": [["4"], 1, 2, 3]}
> (1 row)
>
> postgres=# select jsonb_set('{"a":[[],1,2,3]}', '{"a", 0, 10000}', '"4"');
> jsonb_set
> -------------------------
> {"a": [["4"], 1, 2, 3]}
> (1 row)
>
>
> Research lead to setPathArray where a string which is got via
> VARDATA_ANY but is passed to strtol which expects cstring.
>
> In case of string number length is not a multiplier of 4 rest bytes
> are padding by '\0', when length is a multiplier of 4 there is no
> padding, just garbage after the last digit of the value.
>
> Proposed patch in an attachment fixes it.
>
> There is a magic number "20" as a length of an array for copying key
> from a path before passing it to strtol. It is a maximal length of a
> value which can be parsed by the function. I could not find a proper
> constant for it. Also I found similar direct value in the code (e.g.
> in numeric.c).
>
> I've added a comment, I hope it is enough for it.
>
>
> --
> Best regards,
> Vitaly Burovoy
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On 2016-03-23, Oleg Bartunov <obartunov(at)gmail(dot)com> wrote:
> On Wed, Mar 23, 2016 at 6:37 AM, Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com>
> wrote:
>
>> Hello, Hackers!
>>
>> While I was reviewed a patch with "json_insert" function I found a bug
>> which wasn't connected with the patch and reproduced at master.
>>
>> It claims about non-integer whereas input values are obvious integers
>> and in an allowed range.
>> More testing lead to understanding it appears when numbers length are
>> multiplier of 4:
>>
>> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}',
>> '"4"');
>> ERROR: path element at the position 2 is not an integer
>>
>
> Hmm, I see in master
>
> select version();
> version
> -----------------------------------------------------------------------------------------------------------------
> PostgreSQL 9.6devel on x86_64-apple-darwin15.4.0, compiled by Apple LLVM
> version 7.3.0 (clang-703.0.29), 64-bit
> (1 row)
>
> select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}', '"4"');
> jsonb_set
> ------------------------------------
> {"a": [[], 1, 2, 3, "4"], "b": []}
> (1 row)

Yes, I can't reproduce it with "CFLAGS=-O2", but it is still
reproduced with "CFLAGS='-O0 -g3'".

postgres=# select version();
version
----------------------------------------------------------------------------------------------------------
PostgreSQL 9.6devel on x86_64-pc-linux-gnu, compiled by gcc (Gentoo
4.8.4 p1.4, pie-0.6.1) 4.8.4, 64-bit
(1 row)

postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}', '"4"');
ERROR: path element at the position 2 is not an integer

It depends on memory after the string. In debug mode it always (most
of the time?) has a garbage (in my case the char '~' following by
'\x7f' multiple times) there.

I think it is just a question of complexity of reproducing in release,
not a question whether there is a bug or not.

All the other occurrences of strtol in the file have
TextDatumGetCString before, except the occurrence in the setPathArray
function. It seems its type is TEXT (which is not null-terminated),
not cstring.

--
Best regards,
Vitaly Burovoy

On Wed, Mar 23, 2016 at 7:48 PM, Vitaly Burovoy
<vitaly(dot)burovoy(at)gmail(dot)com> wrote:
> On 2016-03-23, Oleg Bartunov <obartunov(at)gmail(dot)com> wrote:
>> On Wed, Mar 23, 2016 at 6:37 AM, Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com>
>> wrote:
>>
>>> Hello, Hackers!
>>>
>>> While I was reviewed a patch with "json_insert" function I found a bug
>>> which wasn't connected with the patch and reproduced at master.
>>>
>>> It claims about non-integer whereas input values are obvious integers
>>> and in an allowed range.
>>> More testing lead to understanding it appears when numbers length are
>>> multiplier of 4:
>>>
>>> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}',
>>> '"4"');
>>> ERROR: path element at the position 2 is not an integer
>>>
>>
>> Hmm, I see in master
>>
>> select version();
>> version
>> -----------------------------------------------------------------------------------------------------------------
>> PostgreSQL 9.6devel on x86_64-apple-darwin15.4.0, compiled by Apple LLVM
>> version 7.3.0 (clang-703.0.29), 64-bit
>> (1 row)
>>
>> select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}', '"4"');
>> jsonb_set
>> ------------------------------------
>> {"a": [[], 1, 2, 3, "4"], "b": []}
>> (1 row)
>
> Yes, I can't reproduce it with "CFLAGS=-O2", but it is still
> reproduced with "CFLAGS='-O0 -g3'".

On my old-age laptop (OSX 10.8) I can reproduce the failure as well.

> postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", 9999}', '"4"');
> ERROR: path element at the position 2 is not an integer
>
> It depends on memory after the string. In debug mode it always (most
> of the time?) has a garbage (in my case the char '~' following by
> '\x7f' multiple times) there.
>
> I think it is just a question of complexity of reproducing in release,
> not a question whether there is a bug or not.

Er, this is definitely a bug. That's not really a problem I think.

> All the other occurrences of strtol in the file have
> TextDatumGetCString before, except the occurrence in the setPathArray
> function. It seems its type is TEXT (which is not null-terminated),
> not cstring.

- char *c = VARDATA_ANY(path_elems[level]);
+ char *keyptr = VARDATA_ANY(path_elems[level]);
+ int keylen = VARSIZE_ANY_EXHDR(path_elems[level]);
+ char c[20 + 1]; /* int64 = 18446744073709551615 (20
symbols) */
long lindex;
That's ugly. We should actually use TextDatumGetCString because the
index is stored as text here via a Datum, and then it is converted
back to an integer. So I propose instead the simple patch attached
that fixes the failure for me. Could you check if that works for you?
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> That's ugly. We should actually use TextDatumGetCString because the
> index is stored as text here via a Datum, and then it is converted
> back to an integer. So I propose instead the simple patch attached
> that fixes the failure for me. Could you check if that works for you?

Yeah, this seems better. But that code needs review anyway, as it's using
elog() for user-facing error conditions, and I'm thinking the error texts
could use a bit of attention from a native English speaker.

regards, tom lane

On Wed, Mar 23, 2016 at 11:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> That's ugly. We should actually use TextDatumGetCString because the
>> index is stored as text here via a Datum, and then it is converted
>> back to an integer. So I propose instead the simple patch attached
>> that fixes the failure for me. Could you check if that works for you?
>
> Yeah, this seems better. But that code needs review anyway, as it's using
> elog() for user-facing error conditions, and I'm thinking the error texts
> could use a bit of attention from a native English speaker.

Thanks. The bug fix is 384dfbd, and the improvements of error messages
is ea4b8bd.
--
Michael