[PATCH] Make jsonapi usable from libpq

On Tue, Jun 22, 2021 at 10:59:37PM +0000, Jacob Champion wrote:

Hmm. I'm honestly hesitant to start splitting files apart, mostly
because json_log_and_abort() is only called from two places, and both
those places are triggered by programmer error as opposed to user
error.

Would it make more sense to switch to an fprintf-and-abort case, to
match the approach taken by PGTHREAD_ERROR and the out-of-memory
conditions in fe-print.c? Or is there already precedent for handling
can't-happen code paths with in-band errors, through the PGconn?

Not really..

Looking more closely at that, I actually find a bit crazy the
requirement for any logging within jsonapi.c just to cope with the
fact that json_errdetail() and report_parse_error() just want to track
down if the caller is giving some garbage or not, which should never
be the case, really. So I would be tempted to eliminate this
dependency to begin with.

The second thing is how we should try to handle the way the error
message gets allocated in json_errdetail(). libpq cannot rely on
psprintf(), so I can think about two options here:
- Let the caller of json_errdetail() allocate the memory area of the
error message by itself, pass it down to the function.
- Do the allocation within json_errdetail(), and let callers cope the
case where json_errdetail() itself fails on OOM for any frontend code
using it.

Looking at HEAD, the OAUTH patch and the token handling, the second
option looks more interesting. I have to admit that handling the
token part makes the patch a bit special, but that avoids duplicating
all those error messages for libpq. Please see the idea as attached.
--
Michael

On Thu, 2021-06-24 at 14:56 +0900, Michael Paquier wrote:

Looking more closely at that, I actually find a bit crazy the
requirement for any logging within jsonapi.c just to cope with the
fact that json_errdetail() and report_parse_error() just want to track
down if the caller is giving some garbage or not, which should never
be the case, really. So I would be tempted to eliminate this
dependency to begin with.

I think that's a good plan.

The second thing is how we should try to handle the way the error
message gets allocated in json_errdetail(). libpq cannot rely on
psprintf(),

That surprised me. So there's currently no compiler-enforced
prohibition, just a policy? It looks like the bar was lowered a little
bit in commit c0cb87fbb6, as libpq currently has a symbol dependency on
pg_get_line_buf() and pfree() on my machine.

, so I can think about two options here:
- Let the caller of json_errdetail() allocate the memory area of the
error message by itself, pass it down to the function.
- Do the allocation within json_errdetail(), and let callers cope the
case where json_errdetail() itself fails on OOM for any frontend code
using it.

Looking at HEAD, the OAUTH patch and the token handling, the second
option looks more interesting. I have to admit that handling the
token part makes the patch a bit special, but that avoids duplicating
all those error messages for libpq. Please see the idea as attached.

I prefer the second approach as well. Looking at the sample
implementation -- has an allocating sprintf() for libpq really not been
implemented before? Doing it ourselves on the stack gives me some
heartburn; at the very least we'll have to make careful use of snprintf
so as to not smash the stack while parsing malicious JSON.

If our libpq-specific implementation is going to end up returning NULL
on bad allocation anyway, could we just modify the behavior of the
existing front-end palloc implementation to not exit() from inside
libpq? That would save a lot of one-off code for future implementers.

--Jacob

On Fri, Jun 25, 2021 at 08:58:46PM +0000, Jacob Champion wrote:

On Thu, 2021-06-24 at 14:56 +0900, Michael Paquier wrote:

Looking more closely at that, I actually find a bit crazy the
requirement for any logging within jsonapi.c just to cope with the
fact that json_errdetail() and report_parse_error() just want to track
down if the caller is giving some garbage or not, which should never
be the case, really. So I would be tempted to eliminate this
dependency to begin with.

I think that's a good plan.

We could do this cleanup first, as an independent patch. That's
simple enough. I am wondering if we'd better do this bit in 14
actually, so as the divergence between 15~ and 14 is lightly
minimized.

The second thing is how we should try to handle the way the error
message gets allocated in json_errdetail(). libpq cannot rely on
psprintf(),

That surprised me. So there's currently no compiler-enforced
prohibition, just a policy? It looks like the bar was lowered a little
bit in commit c0cb87fbb6, as libpq currently has a symbol dependency on
pg_get_line_buf() and pfree() on my machine.

Good point. That's worse than just pfree() which is just a plain call
to free() in the frontend. We could have more policies here, but my
take is that we'd better move fe_memutils.o to OBJS_FRONTEND in
src/common/Makefile so as shared libraries don't use those routines in
the long term.

In parseServiceFile(), initStringInfo() does a palloc() which would
simply exit() on OOM, in libpq. That's not good. The service file
parsing is the only piece in libpq using StringInfoData. @Tom,
@Daniel, you got involved in c0cb87f. It looks like this piece about
the limitations with service file parsing needs a rework. This code
is new in 14, which means a new open item.

If our libpq-specific implementation is going to end up returning NULL
on bad allocation anyway, could we just modify the behavior of the
existing front-end palloc implementation to not exit() from inside
libpq? That would save a lot of one-off code for future implementers.

Yeah, a side effect of that is to enforce a new rule for any frontend
code that calls palloc(), and these could be easily exposed to crashes
within knowing about it until their system is under resource
pressure. Silent breakages with very old guaranteed behaviors is
bad.
--
Michael

On 26 Jun 2021, at 02:36, Michael Paquier <michael@paquier.xyz> wrote:

The service file parsing is the only piece in libpq using StringInfoData.
@Tom, @Daniel, you got involved in c0cb87f. It looks like this piece about the
limitations with service file parsing needs a rework. This code is new in 14,
which means a new open item.

Reworking it at this point to use a pqexpbuffer would be too invasive for 14
IMO, so reverting this part seems like the best option, and then redo it with
a pqexpbuffer for 15.

--
Daniel Gustafsson https://vmware.com/

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Jun 25, 2021 at 08:58:46PM +0000, Jacob Champion wrote:

That surprised me. So there's currently no compiler-enforced
prohibition, just a policy? It looks like the bar was lowered a little
bit in commit c0cb87fbb6, as libpq currently has a symbol dependency on
pg_get_line_buf() and pfree() on my machine.

Good point. That's worse than just pfree() which is just a plain call
to free() in the frontend. We could have more policies here, but my
take is that we'd better move fe_memutils.o to OBJS_FRONTEND in
src/common/Makefile so as shared libraries don't use those routines in
the long term.

Ugh. Not only is that bad, but your proposed fix doesn't fix it.
At least in psql, and probably in most/all of our other clients,
removing fe_memutils.o from libpq's link just causes it to start
relying on the copy in the psql executable :-(. So I agree that
some sort of mechanical enforcement would be a really good thing,
but I'm not sure what it would look like.

In parseServiceFile(), initStringInfo() does a palloc() which would
simply exit() on OOM, in libpq. That's not good. The service file
parsing is the only piece in libpq using StringInfoData. @Tom,
@Daniel, you got involved in c0cb87f.

I concur with Daniel that the easiest fix for v14 is to revert
c0cb87f. Allowing unlimited-length lines in the service file seems
like a nice-to-have, but it's not worth a lot. (Looking at the patch,
I'm inclined to keep much of the code rearrangement, just remove the
dependency on stringinfo.c. Also I'm tempted to set the fixed buffer
size at 1024 not 256, after which we might never need to improve it.)

I spent some time looking for other undesirable symbol dependencies
in libpq, and soon found a couple. PGTHREAD_ERROR potentially calls
abort(), which seems even worse than exit-on-OOM, although I don't
think we've ever heard a report of that being hit. Also,
fe-print.c's handling of OOM isn't nice at all:

fprintf(stderr, libpq_gettext("out of memory\n"));
abort();

Although fe-print.c is semi-deprecated, it still seems like it'd
be a good idea to clean that up.

BTW, so far as the original topic of this thread is concerned,
it sounds like Jacob's ultimate goal is to put some functionality
into libpq that requires JSON parsing. I'm going to say up front
that that sounds like a terrible idea. As we've just seen, libpq
operates under very tight constraints, not all of which are
mechanically enforced. I am really doubtful that anything that
would require a JSON parser has any business being in libpq.
Unless you can sell us on that point, I do not think it's worth
complicating the src/common JSON code to the point where it can
work under libpq's constraints.

regards, tom lane

I wrote:

I spent some time looking for other undesirable symbol dependencies
in libpq, and soon found a couple. PGTHREAD_ERROR potentially calls
abort(), which seems even worse than exit-on-OOM, although I don't
think we've ever heard a report of that being hit. Also,
fe-print.c's handling of OOM isn't nice at all:
fprintf(stderr, libpq_gettext("out of memory\n"));
abort();
Although fe-print.c is semi-deprecated, it still seems like it'd
be a good idea to clean that up.

fe-print.c seems easy enough to clean up, as per attached.
Not real sure what to do about PGTHREAD_ERROR.

regards, tom lane

I wrote:

Not real sure what to do about PGTHREAD_ERROR.

I wonder if we shouldn't simply nuke that macro and change the
call sites into "Assert(false)". The only call sites are in
default_threadlock() (used in fe_auth.c) and pq_lockingcallback()
(for OpenSSL). I suggest that

1. "fprintf(stderr)" in these locking functions doesn't seem
remarkably well-advised either. Especially not on Windows;
but in general, we don't expect libpq to emit stuff on stderr
except under *very* limited circumstances.

2. In an assert-enabled build, Assert() ought to be about equivalent
to abort().

3. In a production build, if one of these mutex calls fails, ignoring
the failure might be the best thing to do anyway. Certainly, dumping
core is the worst possible outcome, while not doing anything would
have no impact except in the rather-unlikely case that multiple libpq
connections try to use this code concurrently.

It's certainly possible to quibble about point 3, but unless you
have a better alternative to offer, I don't think you have a lot
of room to complain.

regards, tom lane

On Sat, Jun 26, 2021 at 01:43:50PM -0400, Tom Lane wrote:

BTW, so far as the original topic of this thread is concerned,
it sounds like Jacob's ultimate goal is to put some functionality
into libpq that requires JSON parsing. I'm going to say up front
that that sounds like a terrible idea. As we've just seen, libpq
operates under very tight constraints, not all of which are
mechanically enforced. I am really doubtful that anything that
would require a JSON parser has any business being in libpq.
Unless you can sell us on that point, I do not think it's worth
complicating the src/common JSON code to the point where it can
work under libpq's constraints.

AFAIK, the SASL mechanism OAUTHBEARER described in RFC 7628 would
require such facilities as failures are reported in this format:
https://datatracker.ietf.org/doc/html/rfc7628

Perhaps you are right and we have no need to do any json parsing in
libpq as long as we pass down the JSON blob, but I am not completely
sure if we can avoid that either.

Separate topic: I find disturbing the dependency of jsonapi.c to
the logging parts just to cope with dummy error values that are
originally part of JsonParseErrorType.
--
Michael

I wrote:

Not real sure what to do about PGTHREAD_ERROR.

I wonder if we shouldn't simply nuke that macro and change the
call sites into "Assert(false)".

After further study this still seems like the best available choice.
We do not have the option to make either default_threadlock() or
pq_lockingcallback() do something saner, like return a failure
indication. pq_lockingcallback()'s API is dictated by OpenSSL,
while default_threadlock()'s API is exposed to users by libpq
(IOW, we could have gotten that one right years ago, but we
failed to, and it seems much too late to change it now).

Also, I trawled the mailing list archives, and I can find no
indication that any of the PGTHREAD_ERROR messages have ever
been seen in the field. The last relevant discussion seems
to be in

/messages/by-id/20130801142443.GO2706@tamriel.snowman.net

where it was observed that this code isn't very well thought
through :-(

My proposal is to replace PGTHREAD_ERROR by Assert(false)
in HEAD, but leave things alone in the back branches.

As far as the other patch to check for mistakes with "nm"
goes, we could either do nothing in the back branches, or
install a check for "exit" only, not "abort". But there's
probably no real need for such a check in the back branches
as long as we're enforcing it in HEAD.

regards, tom lane

On 28 Jun 2021, at 21:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Not real sure what to do about PGTHREAD_ERROR.

I wonder if we shouldn't simply nuke that macro and change the
call sites into "Assert(false)".

After further study this still seems like the best available choice.

While this solution has a potential downside as you mention upthread, I can't
see any better alternative, and this is clearly better than what we have now.

My proposal is to replace PGTHREAD_ERROR by Assert(false)
in HEAD, but leave things alone in the back branches.

+1

As far as the other patch to check for mistakes with "nm"
goes, we could either do nothing in the back branches, or
install a check for "exit" only, not "abort". But there's
probably no real need for such a check in the back branches
as long as we're enforcing it in HEAD.

I don't see any real reason to backport the check, but enforce it in HEAD going
forward. The risk of introducing an exit in backbranches when enforced against
in HEAD seem pretty manageable.

--
Daniel Gustafsson https://vmware.com/

On Sun, 2021-06-27 at 10:43 +0900, Michael Paquier wrote:

On Sat, Jun 26, 2021 at 01:43:50PM -0400, Tom Lane wrote:

BTW, so far as the original topic of this thread is concerned,
it sounds like Jacob's ultimate goal is to put some functionality
into libpq that requires JSON parsing. I'm going to say up front
that that sounds like a terrible idea. As we've just seen, libpq
operates under very tight constraints, not all of which are
mechanically enforced. I am really doubtful that anything that
would require a JSON parser has any business being in libpq.
Unless you can sell us on that point, I do not think it's worth
complicating the src/common JSON code to the point where it can
work under libpq's constraints.

AFAIK, the SASL mechanism OAUTHBEARER described in RFC 7628 would
require such facilities as failures are reported in this format:
https://datatracker.ietf.org/doc/html/rfc7628

Right. So it really comes down to whether or not OAUTHBEARER support is
worth this additional complication, and that probably belongs to the
main thread on the topic.

But hey, we're getting some code cleanup out of the deal either way.

Perhaps you are right and we have no need to do any json parsing in
libpq as long as we pass down the JSON blob, but I am not completely
sure if we can avoid that either.

It is definitely an option.

With the current architecture of the proof-of-concept, I feel like
forcing every client to implement JSON parsing just to be able to use
OAUTHBEARER would be a significant barrier to entry. Again, that's
probably conversation for the main thread.

Separate topic: I find disturbing the dependency of jsonapi.c to
the logging parts just to cope with dummy error values that are
originally part of JsonParseErrorType.

I think we should clean this up regardless.

--Jacob

On Sat, 2021-06-26 at 09:36 +0900, Michael Paquier wrote:

On Fri, Jun 25, 2021 at 08:58:46PM +0000, Jacob Champion wrote:

On Thu, 2021-06-24 at 14:56 +0900, Michael Paquier wrote:

Looking more closely at that, I actually find a bit crazy the
requirement for any logging within jsonapi.c just to cope with the
fact that json_errdetail() and report_parse_error() just want to track
down if the caller is giving some garbage or not, which should never
be the case, really. So I would be tempted to eliminate this
dependency to begin with.

I think that's a good plan.

We could do this cleanup first, as an independent patch. That's
simple enough. I am wondering if we'd better do this bit in 14
actually, so as the divergence between 15~ and 14 is lightly
minimized.

Up to you in the end; I don't have a good intuition for whether the
code motion would be worth it for 14, if it's not actively used.

The second thing is how we should try to handle the way the error
message gets allocated in json_errdetail(). libpq cannot rely on
psprintf(),

That surprised me. So there's currently no compiler-enforced
prohibition, just a policy? It looks like the bar was lowered a little
bit in commit c0cb87fbb6, as libpq currently has a symbol dependency on
pg_get_line_buf() and pfree() on my machine.

This seems to have spawned an entirely new thread over the weekend,
which I will watch with interest. :)

If our libpq-specific implementation is going to end up returning NULL
on bad allocation anyway, could we just modify the behavior of the
existing front-end palloc implementation to not exit() from inside
libpq? That would save a lot of one-off code for future implementers.

Yeah, a side effect of that is to enforce a new rule for any frontend
code that calls palloc(), and these could be easily exposed to crashes
within knowing about it until their system is under resource
pressure. Silent breakages with very old guaranteed behaviors is
bad.

Fair point.

What would you think about a src/port of asprintf()? Maybe libpq
doesn't change quickly enough to worry about it, but having developers
revisit stack allocation for strings every time they target the libpq
parts of the code seems like a recipe for security problems.

--Jacob

Jacob Champion <pchampion@vmware.com> writes:

What would you think about a src/port of asprintf()? Maybe libpq
doesn't change quickly enough to worry about it, but having developers
revisit stack allocation for strings every time they target the libpq
parts of the code seems like a recipe for security problems.

The existing convention is to use pqexpbuffer.c, which seems strictly
cleaner and more robust than asprintf. In particular its behavior under
OOM conditions is far easier/safer to work with. Maybe we should consider
moving that into src/common/ so that it can be used by code that's not
tightly bound into libpq?

regards, tom lane

On Tue, 2021-06-29 at 14:50 -0400, Tom Lane wrote:

Jacob Champion <pchampion@vmware.com> writes:

What would you think about a src/port of asprintf()? Maybe libpq
doesn't change quickly enough to worry about it, but having developers
revisit stack allocation for strings every time they target the libpq
parts of the code seems like a recipe for security problems.

The existing convention is to use pqexpbuffer.c, which seems strictly
cleaner and more robust than asprintf. In particular its behavior under
OOM conditions is far easier/safer to work with. Maybe we should consider
moving that into src/common/ so that it can be used by code that's not
tightly bound into libpq?

I will take a look. Were you thinking we'd (hypothetically) migrate all
string allocation code under src/common to pqexpbuffer as part of that
move? Or just have it there to use as needed, when nm complains?

--Jacob

Jacob Champion <pchampion@vmware.com> writes:

On Tue, 2021-06-29 at 14:50 -0400, Tom Lane wrote:

The existing convention is to use pqexpbuffer.c, which seems strictly
cleaner and more robust than asprintf. In particular its behavior under
OOM conditions is far easier/safer to work with. Maybe we should consider
moving that into src/common/ so that it can be used by code that's not
tightly bound into libpq?

I will take a look. Were you thinking we'd (hypothetically) migrate all
string allocation code under src/common to pqexpbuffer as part of that
move? Or just have it there to use as needed, when nm complains?

Actually, I'd forgotten that the PQExpBuffer functions are already
exported by libpq, and much of our frontend code already uses them
from there. So we don't really need to move anything unless there's
a call to use this code in clients that don't use libpq, which are
a pretty small set.

Also, having them be available both from libpq.so and from libpgcommon.a
would be a tad problematic I think; it'd be hard to tell which way the
linker would choose to resolve that.

regards, tom lane

On 26.06.21 19:43, Tom Lane wrote:

I spent some time looking for other undesirable symbol dependencies
in libpq, and soon found a couple. PGTHREAD_ERROR potentially calls
abort(), which seems even worse than exit-on-OOM, although I don't
think we've ever heard a report of that being hit. Also,
fe-print.c's handling of OOM isn't nice at all:

fprintf(stderr, libpq_gettext("out of memory\n"));
abort();

Although fe-print.c is semi-deprecated, it still seems like it'd
be a good idea to clean that up.

These abort() calls were put there on purpose by:

commit c6ea8ccea6bf23501962ddc7ac9ffdb99c8643e1
Author: Peter Eisentraut <peter_e@gmx.net>
Date: Mon Jan 30 20:34:00 2012

Use abort() instead of exit() to abort library functions

In some hopeless situations, certain library functions in libpq and
libpgport quit the program. Use abort() for that instead of exit(),
so we don't interfere with the normal exit codes the program might
use, we clearly signal the abnormal termination, and the caller has a
chance of catching the termination.

This was originally pointed out by Debian's Lintian program.

I don't object to refining this, but I think it is a mischaracterization
to calls this kind of code wrong. And I'm dubious about the backpatching.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 26.06.21 19:43, Tom Lane wrote:

fe-print.c's handling of OOM isn't nice at all:
fprintf(stderr, libpq_gettext("out of memory\n"));
abort();
Although fe-print.c is semi-deprecated, it still seems like it'd
be a good idea to clean that up.

These abort() calls were put there on purpose by:
commit c6ea8ccea6bf23501962ddc7ac9ffdb99c8643e1
Use abort() instead of exit() to abort library functions

Well, the exit() calls that that replaced were quite inappropriate
too IMO. I don't think it boots much to argue about which way was
less bad; libpq has no business doing either thing.

What might be more useful is to consider whether there's a way
to retrofit an error-reporting convention onto these functions.
I thought about that for a bit, but concluded that the possible
interactions with stdio's error handling made that fairly tricky,
and it didn't seem worth messing with for such backwater code.
(Too bad POSIX didn't see fit to provide seterr(FILE*), or maybe
we could have reported OOM in fe-print that way.)

regards, tom lane

On Tue, Jun 29, 2021 at 03:34:29PM -0400, Tom Lane wrote:

Actually, I'd forgotten that the PQExpBuffer functions are already
exported by libpq, and much of our frontend code already uses them
from there. So we don't really need to move anything unless there's
a call to use this code in clients that don't use libpq, which are
a pretty small set.

Also, having them be available both from libpq.so and from libpgcommon.a
would be a tad problematic I think; it'd be hard to tell which way the
linker would choose to resolve that.

Coming back to this thread. You were thinking about switching to
PQExpBuffer for the error strings generated depending on error code
for the frontend, right? Using a PQExpBuffer would be a problem if
attempting to get a more detailed error for pg_verifybackup, though I
guess that we can continue to live in this tool without this much
amount of details in the error strings.

It seems to me that this does not address yet the problems with the
palloc/pstrdup in jsonapi.c though, which would need to rely on
malloc() if we finish to use this code in libpq. I am not sure yet
that we have any need to do that yet as we may finish by not using
OAUTH as SASL mechanism at the end in core. So perhaps it would be
better to just give up on this thread for now?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

It seems to me that this does not address yet the problems with the
palloc/pstrdup in jsonapi.c though, which would need to rely on
malloc() if we finish to use this code in libpq. I am not sure yet
that we have any need to do that yet as we may finish by not using
OAUTH as SASL mechanism at the end in core. So perhaps it would be
better to just give up on this thread for now?

Yeah, I think there's nothing to do here unless we decide that we
have to have JSON-parsing ability inside libpq ... which is a
situation I think we should try hard to avoid.

regards, tom lane

On Wed, 2021-07-07 at 01:42 -0400, Tom Lane wrote:

Michael Paquier <michael@paquier.xyz> writes:

It seems to me that this does not address yet the problems with the
palloc/pstrdup in jsonapi.c though, which would need to rely on
malloc() if we finish to use this code in libpq. I am not sure yet
that we have any need to do that yet as we may finish by not using
OAUTH as SASL mechanism at the end in core. So perhaps it would be
better to just give up on this thread for now?

Yeah, I think there's nothing to do here unless we decide that we
have to have JSON-parsing ability inside libpq ... which is a
situation I think we should try hard to avoid.

I'm working on a corrected version of the allocation for the OAuth
proof of concept, so we can see what it might look like there. I will
withdraw this one from the commitfest. Thanks for all the feedback!

--Jacob