Horton

Thread

Commits

  1. jsonapi: add lexer option to keep token ownership

  1. pg_parse_json() should not leak token copies on failure

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-04-30T21:29:27Z

    Hi,

When a client of our JSON parser registers semantic action callbacks,
the parser will allocate copies of the lexed tokens to pass into those
callbacks. The intent is for the callbacks to always take ownership of
the copied memory, and if they don't want it then they can pfree() it.

However, if parsing fails on the token before the callback is invoked,
that allocation is leaked. That's not a problem for the server side,
or for clients that immediately exit on parse failure, but if the JSON
parser gets added to libpq for OAuth, it will become more of a
problem.

(I'm building a fuzzer to flush out these sorts of issues; not only
does it complain loudly about the leaks, but the accumulation of
leaked memory puts a hard limit on how long a fuzzer run can last. :D)

Attached is a draft patch to illustrate what I mean, but it's
incomplete: it only solves the problem for scalar values. We also make
a copy of object field names, which is much harder to fix, because we
make only a single copy and pass that to both the object_field_start
and object_field_end callbacks. Consider:

- If a client only implements object_field_start, it takes ownership
of the field name when we call it. It can free the allocation if it
decides that the field is irrelevant.
- The same is true for clients that only implement object_field_end.
- If a client implements both callbacks, it takes ownership of the
field name when we call object_field_start. But irrelevant field names
can't be freed during that callback, because the same copy is going to
be passed to object_field_end. And object_field_end isn't guaranteed
to be called, because parsing could fail for any number of reasons
between now and then. So what code should be responsible for cleanup?
The parser doesn't know whether the callback already freed the pointer
or kept a reference to it in semstate.

Any thoughts on how we can improve this? I was thinking we could maybe
make two copies of the field name and track ownership individually?

Thanks,
--Jacob

  2. Re: pg_parse_json() should not leak token copies on failure

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-05-24T15:43:01Z

    On Tue, Apr 30, 2024 at 2:29 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Attached is a draft patch to illustrate what I mean, but it's
> incomplete: it only solves the problem for scalar values.

(Attached is a v2 of that patch; in solving a frontend leak I should
probably not introduce a backend segfault.)

--Jacob

  3. Re: pg_parse_json() should not leak token copies on failure

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-06-04T04:32:31Z

    Hi,

At Fri, 24 May 2024 08:43:01 -0700, Jacob Champion <jacob.champion@enterprisedb.com> wrote in 
> On Tue, Apr 30, 2024 at 2:29 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> > Attached is a draft patch to illustrate what I mean, but it's
> > incomplete: it only solves the problem for scalar values.
> 
> (Attached is a v2 of that patch; in solving a frontend leak I should
> probably not introduce a backend segfault.)

I understand that the part enclosed in parentheses refers to the "if
(ptr) pfree(ptr)" structure. I believe we rely on the behavior of
free(NULL), which safely does nothing. (I couldn't find the related
discussion due to a timeout error on the ML search page.)

Although I don't fully understand the entire parser code, it seems
that the owner transfer only occurs in JSON_TOKEN_STRING cases. That
is, the memory pointed by scalar_val would become dangling in
JSON_TOKEN_NUBMER cases. Even if this is not the case, the ownership
transition apperas quite callenging to follow.

It might be safer or clearer to pstrdup the token in jsonb_in_scalar()
and avoid NULLifying scalar_val after calling callbacks, or to let
jsonb_in_sclar() NULLify the pointer.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: pg_parse_json() should not leak token copies on failure

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-06-04T17:10:06Z

    On Mon, Jun 3, 2024 at 9:32 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Hi,

Thanks for the review!

> I understand that the part enclosed in parentheses refers to the "if
> (ptr) pfree(ptr)" structure. I believe we rely on the behavior of
> free(NULL), which safely does nothing. (I couldn't find the related
> discussion due to a timeout error on the ML search page.)

For the frontend, right. For the backend, pfree(NULL) causes a crash.
I think [1] is a related discussion on the list, maybe the one you
were looking for?

> Although I don't fully understand the entire parser code, it seems
> that the owner transfer only occurs in JSON_TOKEN_STRING cases. That
> is, the memory pointed by scalar_val would become dangling in
> JSON_TOKEN_NUBMER cases.

It should happen for both cases, I think. I see that the
JSON_SEM_SCALAR_CALL case passes scalar_val into the callback
regardless of whether we're parsing a string or a number, and
parse_scalar() does the same thing over in the recursive
implementation. Have I missed a code path?

> Even if this is not the case, the ownership
> transition apperas quite callenging to follow.

I agree!

> It might be safer or clearer to pstrdup the token in jsonb_in_scalar()
> and avoid NULLifying scalar_val after calling callbacks, or to let
> jsonb_in_sclar() NULLify the pointer.

Maybe. jsonb_in_scalar isn't the only scalar callback implementation,
though. It might be a fairly cruel change for dependent extensions,
since there will be no compiler complaint.

If a compatibility break is acceptable, maybe we should make the API
zero-copy for the recursive case, and just pass the length of the
parsed token? Either way, we'd have to change the object field
callbacks, too, but maybe an API change makes even more sense there,
given how convoluted it is right now.

Thanks,
--Jacob

[1] https://www.postgresql.org/message-id/flat/cf26e970-8e92-59f1-247a-aa265235075b%40enterprisedb.com

  5. Re: pg_parse_json() should not leak token copies on failure

    Michael Paquier <michael@paquier.xyz> — 2024-06-05T05:15:20Z

    On Tue, Jun 04, 2024 at 10:10:06AM -0700, Jacob Champion wrote:
> On Mon, Jun 3, 2024 at 9:32 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
>> I understand that the part enclosed in parentheses refers to the "if
>> (ptr) pfree(ptr)" structure. I believe we rely on the behavior of
>> free(NULL), which safely does nothing. (I couldn't find the related
>> discussion due to a timeout error on the ML search page.)
> 
> For the frontend, right. For the backend, pfree(NULL) causes a crash.
> I think [1] is a related discussion on the list, maybe the one you
> were looking for?

FYI, these choices relate also to 805a397db40b, e890ce7a4feb and
098c703d308f.
--
Michael

  6. Re: pg_parse_json() should not leak token copies on failure

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-10-01T19:04:53Z

    (Tom, thank you for the fixup in 75240f65!)

Off-list, Andrew suggested an alternative approach to the one I'd been
taking: let the client choose whether it wants to take token ownership
to begin with. v3 adds a new flag (and associated API) which will
allow libpq to refuse ownership of those tokens. The lexer is then
free to clean everything up during parse failures.

Usually I'm not a fan of "do the right thing" flags, but in this case,
libpq really is the outlier. And it's nice that existing clients
(potentially including extensions) don't have to worry about an API
change.

At the moment, we have a test matrix consisting of "standard frontend"
and "shlib frontend" tests for the incremental parser. I'm planning
for the v4 patch to extend that with a "owned/not owned" dimension;
any objections?

Thanks,
--Jacob

  7. Re: pg_parse_json() should not leak token copies on failure

    Andrew Dunstan <andrew@dunslane.net> — 2024-10-02T17:45:05Z

    On 2024-10-01 Tu 3:04 PM, Jacob Champion wrote:
> (Tom, thank you for the fixup in 75240f65!)
>
> Off-list, Andrew suggested an alternative approach to the one I'd been
> taking: let the client choose whether it wants to take token ownership
> to begin with. v3 adds a new flag (and associated API) which will
> allow libpq to refuse ownership of those tokens. The lexer is then
> free to clean everything up during parse failures.
>
> Usually I'm not a fan of "do the right thing" flags, but in this case,
> libpq really is the outlier. And it's nice that existing clients
> (potentially including extensions) don't have to worry about an API
> change.


Yeah.

Generally looks good. Should we have a check in 
setJsonLexContextOwnsTokens() that we haven't started parsing yet, for 
the incremental case?



>
> At the moment, we have a test matrix consisting of "standard frontend"
> and "shlib frontend" tests for the incremental parser. I'm planning
> for the v4 patch to extend that with a "owned/not owned" dimension;
> any objections?
>

Sounds reasonable.


cheers


andrew

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

  8. Re: pg_parse_json() should not leak token copies on failure

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-10-07T22:45:25Z

    On Wed, Oct 2, 2024 at 10:45 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> Generally looks good. Should we have a check in
> setJsonLexContextOwnsTokens() that we haven't started parsing yet, for
> the incremental case?

Good catch. Added in v4.

> > At the moment, we have a test matrix consisting of "standard frontend"
> > and "shlib frontend" tests for the incremental parser. I'm planning
> > for the v4 patch to extend that with a "owned/not owned" dimension;
> > any objections?
> >
>
> Sounds reasonable.

This is also done, along with runs of pgindent/perltidy.

I've added a 0002 as well. While running tests under Valgrind, it
complained about the uninitialized dummy_lex on the stack, which is
now fixed. (That bug was introduced with my patch in 0785d1b8b and is
technically orthogonal to 0001, but I figured I could track it here
for now.) This is also how I found out that my existing fuzzers
weren't checking for uninitialized memory, like I thought they were.
Grumble...

Thanks,
--Jacob

  9. Re: pg_parse_json() should not leak token copies on failure

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-10-31T21:10:14Z

    On Mon, Oct 7, 2024 at 3:45 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I've added a 0002 as well.

0002 has since been applied [1] so I'm reattaching v4-0001 to get the
cfbot happy again.

--Jacob

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=41b023946d

  10. Re: pg_parse_json() should not leak token copies on failure

    Andrew Dunstan <andrew@dunslane.net> — 2024-11-27T17:15:42Z

    On 2024-10-31 Th 5:10 PM, Jacob Champion wrote:
> On Mon, Oct 7, 2024 at 3:45 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> I've added a 0002 as well.
> 0002 has since been applied [1] so I'm reattaching v4-0001 to get the
> cfbot happy again.
>
> --Jacob
>
> [1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=41b023946d




Thanks, committed.


cheers


andrew

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