Re: More new SQL/JSON item methods

On 2024-01-25 Th 14:31, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Thanks, I have pushed this.
> The buildfarm is pretty widely unhappy, mostly failing on
>
> select jsonb_path_query('1.23', '$.string()');
>
> On a guess, I tried running that under valgrind, and behold it said
>
> ==00:00:00:05.637 435530== Conditional jump or move depends on uninitialised value(s)
> ==00:00:00:05.637 435530== at 0x8FD131: executeItemOptUnwrapTarget (jsonpath_exec.c:1547)
> ==00:00:00:05.637 435530== by 0x8FED03: executeItem (jsonpath_exec.c:626)
> ==00:00:00:05.637 435530== by 0x8FED03: executeNextItem (jsonpath_exec.c:1604)
> ==00:00:00:05.637 435530== by 0x8FCA58: executeItemOptUnwrapTarget (jsonpath_exec.c:956)
> ==00:00:00:05.637 435530== by 0x8FFDE4: executeItem (jsonpath_exec.c:626)
> ==00:00:00:05.637 435530== by 0x8FFDE4: executeJsonPath.constprop.30 (jsonpath_exec.c:612)
> ==00:00:00:05.637 435530== by 0x8FFF8C: jsonb_path_query_internal (jsonpath_exec.c:438)
>
> It's fairly obviously right about that:
>
> JsonbValue jbv;
> ...
> jb = &jbv;
> Assert(tmp != NULL); /* We must have set tmp above */
> jb->val.string.val = (jb->type == jbvString) ? tmp : pstrdup(tmp);
> ^^^^^^^^^^^^^^^^^^^^^
>
> Presumably, this is a mistaken attempt to test the type
> of the thing previously pointed to by "jb".
>
> On the whole, what I'd be inclined to do here is get rid
> of this test altogether and demand that every path through
> the preceding "switch" deliver a value that doesn't need
> pstrdup(). The only path that doesn't do that already is
>
> case jbvBool:
> tmp = (jb->val.boolean) ? "true" : "false";
> break;
>
> and TBH I'm not sure that we really need a pstrdup there
> either. The constants are immutable enough. Is something
> likely to try to pfree the pointer later? I tried
>
> @@ -1544,7 +1544,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
>
> jb = &jbv;
> Assert(tmp != NULL); /* We must have set tmp above */
> - jb->val.string.val = (jb->type == jbvString) ? tmp : pstrdup(tmp);
> + jb->val.string.val = tmp;
> jb->val.string.len = strlen(jb->val.string.val);
> jb->type = jbvString;
>
> and that quieted valgrind for this particular query and still
> passes regression.
>
> (The reported crashes seem to be happening later during a
> recursive invocation, seemingly because JsonbType(jb) is
> returning garbage. So there may be another bug after this one.)
>
>

Argh! Will look, thanks.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com