Re: More new SQL/JSON item methods - Mailing list pgsql-hackers

On 2024-01-25 Th 14:31, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.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.



Your fix looks sane. I also don't see why we need the pstrdup.


>
> (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.)
>
>             


I don't think so. AIUI The first call deals with the '$' and the second 
one deals with the '.string()', which is why we see the error on the 
second call.


cheers


andrew

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