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
Attachments:
fix_jsonb_set_path.0001.patchapplication/octet-stream; name=fix_jsonb_set_path.0001.patchDownload+10-1
On Wed, Mar 23, 2016 at 6:37 AM, Vitaly Burovoy <vitaly.burovoy@gmail.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)
Show quoted text
postgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"b", 1000}',
'"4"');
ERROR: path element at the position 2 is not an integerpostgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a", -999}',
'"4"');
ERROR: path element at the position 2 is not an integerpostgres=# select jsonb_set('{"a":[[],1,2,3],"b":[]}', '{"a",
10009999}', '"4"');
ERROR: path element at the position 2 is not an integerClose 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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-03-23, Oleg Bartunov <obartunov@gmail.com> wrote:
On Wed, Mar 23, 2016 at 6:37 AM, Vitaly Burovoy <vitaly.burovoy@gmail.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 integerHmm, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 23, 2016 at 7:48 PM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
On 2016-03-23, Oleg Bartunov <obartunov@gmail.com> wrote:
On Wed, Mar 23, 2016 at 6:37 AM, Vitaly Burovoy <vitaly.burovoy@gmail.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 integerHmm, 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 integerIt 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?
--
MichaelAttachments:
jsonb-set-fix-v1.patchbinary/octet-stream; name=jsonb-set-fix-v1.patchDownload+1-1
Michael Paquier <michael.paquier@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 23, 2016 at 11:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers