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

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