The internals of the frontend-side callbacks for SASL are visible in
libpq-int.h, but the header was not getting installed. This would cause
compilation failures for applications playing with the internals of
libpq.
Issue introduced in 9fd8557.
Author: Mikhail Kulagin
Reviewed-by: Jacob Champion
Discussion: https://postgr.es/m/05ce01d777cb$40f31d60$c2d95820$@postgrespro.ru
The following checks are added, to make the SASL infrastructure more
aware of defects when implementing new mechanisms:
- Detect that no output is generated by a mechanism if an exchange fails
in the backend, failing if there is a message waiting to be sent.
- Handle zero-length messages in the frontend. The backend handles that
already, and SCRAM would complain if sending empty messages as this is
not authorized for this mechanism, but other mechanisms may want this
capability (the SASL specification allows that).
- Make sure that a mechanism generates a message in the middle of the
exchange in the frontend.
SCRAM, as implemented, respects all these requirements already, and the
recent refactoring of SASL done in 9fd8557 helps in documenting that in
a cleaner way.
Analyzed-by: Jacob Champion
Author: Michael Paquier
Reviewed-by: Jacob Champion
Discussion: https://postgr.es/m/3d2a6f5d50e741117d6baf83eb67ebf1a8a35a11.camel@vmware.com
When sending queries in pipeline mode, we were careless about leaving
the connection in the right state so that PQgetResult would behave
correctly; trying to read further results after sending a query after
having read a result with an error would sometimes hang. Fix by
ensuring internal libpq state is changed properly. All the state
changes were being done by the callers of pqAppendCmdQueueEntry(); it
would have become too repetitious to have this logic in each of them, so
instead put it all in that function and relieve callers of the
responsibility.
Add a test to verify this case. Without the code fix, this new test
hangs sometimes.
Also, document that PQisBusy() would return false when no queries are
pending result. This is not intuitively obvious, and NULL would be
obtained by calling PQgetResult() at that point, which is confusing.
Wording by Boris Kolpackov.
In passing, fix bogus use of "false" to mean "0", per Ranier Vilela.
Backpatch to 14.
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reported-by: Boris Kolpackov <boris@codesynthesis.com>
Discussion: https://postgr.es/m/boris.20210624103805@codesynthesis.com
The code of SCRAM and SASL have been tightly linked together since SCRAM
exists in the core code, making hard to apprehend the addition of new
SASL mechanisms, but these are by design different facilities, with
SCRAM being an option for SASL. This refactors the code related to both
so as the backend and the frontend use a set of callbacks for SASL
mechanisms, documenting while on it what is expected by anybody adding a
new SASL mechanism.
The separation between both layers is neat, using two sets of callbacks
for the frontend and the backend to mark the frontier between both
facilities. The shape of the callbacks is now directly inspired from
the routines used by SCRAM, so the code change is straight-forward, and
the SASL code is moved into its own set of files. These will likely
change depending on how and if new SASL mechanisms get added in the
future.
Author: Jacob Champion
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/3d2a6f5d50e741117d6baf83eb67ebf1a8a35a11.camel@vmware.com
Disable this check altogether in --enable-coverage builds,
because newer versions of gcc insert exit() as well as abort()
calls for that. Also disable it on AIX and Solaris, because
those platforms tend to provide facilities such as libldap
as static libraries, which then get included in libpq's shlib.
We can't expect such libraries to honor our coding rules.
(That platform list might need additional tweaking, but I think
this is enough to keep the buildfarm happy.)
Per reports from Jacob Champion and Noah Misch.
Discussion: https://postgr.es/m/3128896.1624742969@sss.pgh.pa.us
Further fixes for commit dc227eb82. Per suggestion from
Peter Eisentraut, use a stamp-file to control when the check
is run, avoiding repeated executions during "make all".
Also, remove "-g" switch for nm: it's useless and some versions
of nm consider it to conflict with "-u". (Thanks to Noah Misch
for running down that portability issue.)
Discussion: https://postgr.es/m/3128896.1624742969@sss.pgh.pa.us
Give up on trying to mechanically forbid abort() within libpq.
Even though there are no such calls in the source code, we've now
seen three different scenarios where build toolchains silently
insert such calls: gcc does it for profiling, some platforms
implement assert() using it, and icc does so for no visible reason.
Checking for accidental use of exit() seems considerably more
important than checking for abort(), so we'll settle for doing
that for now.
Also, filter out __cxa_atexit() to avoid a false match. It seems
that OpenBSD inserts a call to that despite the fact that libpq
contains no C++ code.
Discussion: https://postgr.es/m/3128896.1624742969@sss.pgh.pa.us
The original coding required that PQpipelineSync had been called before
the first call to PQgetResult, and failure to do that would result in an
unexpected NULL result being returned. Fix by setting the right state
when a query is sent, rather than leaving it unchanged and having
PQpipelineSync apply the necessary state change.
A new test case to verify the behavior is added, which relies on the new
PQsendFlushRequest() function added by commit a7192326c7.
Backpatch to 14, where pipeline mode was added.
Reported-by: Boris Kolpackov <boris@codesynthesis.com>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/boris.20210616110321@codesynthesis.com
This new libpq function allows the application to send an 'H' message,
which instructs the server to flush its outgoing buffer.
This hasn't been needed so far because the Sync message already requests
a buffer; and I failed to realize that this was needed in pipeline mode
because PQpipelineSync also causes the buffer to be flushed. However,
sometimes it is useful to request a flush without establishing a
synchronization point.
Backpatch to 14, where pipeline mode was introduced in libpq.
Reported-by: Boris Kolpackov <boris@codesynthesis.com>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202106252350.t76x73nt643j@alvherre.pgsql
Directly exiting or aborting seems like poor form for a general-purpose
library. Now that libpq liberally uses bits out of src/common/,
it's very easy to accidentally include code that would do something
unwanted like calling exit(1) after OOM --- see for example 8ec00dc5c.
Hence, add a simple cross-check that no such calls have made it into
libpq.so.
The cross-check depends on nm(1) being available and being able to
work on a shared library, which probably isn't true everywhere.
But we can just make the test silently do nothing if nm fails.
As long as the check is effective on common platforms, that should
be good enough. (By the same logic, I've not worried about providing
an equivalent test in MSVC builds.)
Discussion: https://postgr.es/m/3128896.1624742969@sss.pgh.pa.us
Doing an abort() seems all right in development builds, but not in
production builds of general-purpose libraries. However, the functions
that were doing this lack any way to report a failure back up to their
callers. It seems like we can just get away with ignoring failures in
production builds, since (a) no such failures have been reported in the
dozen years that the code's been like this, and (b) failure to enforce
mutual exclusion during fe-auth.c operations would likely not cause any
problems anyway in most cases. (The OpenSSL callbacks that use this
macro are obsolete, so even less likely to cause interesting problems.)
Possibly a better answer would be to break compatibility of the
pgthreadlock_t callback API, but in the absence of field problem
reports, it doesn't really seem worth the trouble.
Discussion: https://postgr.es/m/3131385.1624746109@sss.pgh.pa.us
Causing a core dump on out-of-memory seems pretty unfriendly,
and surely is far outside the expected behavior of a general-purpose
library. Just print an error message (as we did already) and return.
These functions unfortunately don't have an error return convention,
but code using them is probably just looking for a quick-n-dirty
print method and wouldn't bother to check anyway.
Although these functions are semi-deprecated, it still seems
appropriate to back-patch this. In passing, also back-patch
b90e6cef1, just to reduce cosmetic differences between the
branches.
Discussion: https://postgr.es/m/3122443.1624735363@sss.pgh.pa.us
Commit c0cb87fbb unwisely introduced a dependency on the StringInfo
machinery in fe-connect.c. We must not use that in libpq, because
it will do a summary exit(1) if it hits OOM, and that is not
appropriate behavior for a general-purpose library. The goal of
allowing arbitrary line lengths in service files doesn't seem like
it's worth a lot of effort, so revert back to the previous method
of using a stack-allocated buffer and failing on buffer overflow.
This isn't an exact revert though. I kept that patch's refactoring
to have a single exit path, as that seems cleaner than having each
error path know what to do to clean up. Also, I made the fixed-size
buffer 1024 bytes not 256, just to push off the need for an expandable
buffer some more.
There is more to do here; in particular the lack of any mechanical
check for this type of mistake now seems pretty hazardous. But this
fix gets us back to the level of robustness we had in v13, anyway.
Discussion: https://postgr.es/m/daeb22ec6ca8ef61e94d766a9b35fb03cabed38e.camel@vmware.com
Our uses of gss_display_status() and gss_display_name() assumed
that the gss_buffer_desc strings returned by those functions are
null-terminated. It appears that they generally are, given the
lack of field complaints up to now. However, the available
documentation does not promise this, and some man pages
for gss_display_status() show examples that rely on the
gss_buffer_desc.length field instead of expecting null
termination. Also, we now have a report that on some
implementations, clang's address sanitizer is of the opinion
that the byte after the specified length is undefined.
Hence, change the code to rely on the length field instead.
This might well be cosmetic rather than fixing any real bug, but
it's hard to be sure, so back-patch to all supported branches.
While here, also back-patch the v12 changes that made pg_GSS_error
deal honestly with multiple messages available from
gss_display_status.
Per report from Sudheer H R.
Discussion: https://postgr.es/m/5372B6D4-8276-42C0-B8FB-BD0918826FC3@tekenlight.com
We had a request to provide a way to test at compile time for the
availability of the new pipeline features. More generally, it
seems like a good idea to provide a way to test via #ifdef for
all new libpq API features. People have been using the version
from pg_config.h for that; but that's more likely to represent the
server version than the libpq version, in the increasingly-common
scenario where they're different. It's safer if libpq-fe.h itself
is the source of truth about what features it offers.
Hence, establish a policy that starting in v14 we'll add a suitable
feature-is-present macro to libpq-fe.h when we add new API there.
(There doesn't seem to be much point in applying this policy
retroactively, but it's not too late for v14.)
Tom Lane and Alvaro Herrera, per suggestion from Boris Kolpackov.
Discussion: https://postgr.es/m/boris.20210617102439@codesynthesis.com
Commit acb7e4eb6b added a new implementation for PQsendQuery so that
it works in pipeline mode (by using extended query protocol), but it
behaves differently from the 'Q' message (in simple query protocol) used
by regular implementation: the new one doesn't close the unnamed portal.
Change the new code to have identical behavior to the old.
Reported-by: Yura Sokolov <y.sokolov@postgrespro.ru>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202106072107.d4i55hdscxqj@alvherre.pgsql
We have a dozen PQset*() functions. PQresultSetInstanceData() and this
were the libpq setter functions having a different word order. Adopt
the majority word order.
Reviewed by Alvaro Herrera and Robert Haas, though this choice of name
was not unanimous.
Discussion: https://postgr.es/m/20210605060555.GA216695@rfd.leadboat.com
Buildfarm member hamerkop has been reporting that two cases in
connect/test5.pgc show different error messages than the test expects,
because since commit ffa2e4670 libpq's connection failure messages
are exposing the fact that a GSS-encrypted connection was attempted
and failed. That's pretty interesting information in itself, and
I certainly don't wish to shoot the messenger, but we need to do
something to stabilize the ECPG results.
For the second of these two failure cases, we can add the
gssencmode=disable option to prevent the discrepancy. However,
that solution is problematic for the first failure, because the only
unique thing about that case is that it's testing a completely-omitted
connection target; there's noplace to add the option without defeating
the point of the test case. After some thrashing around with
alternative fixes that turned out to have undesirable side-effects,
the most workable answer is just to give up and remove that test case.
Perhaps we can revert this later, if we figure out why the GSS code
is misbehaving in hamerkop's environment.
Thanks to Michael Paquier for exploration of alternatives.
Discussion: https://postgr.es/m/YLRZH6CWs9N6Pusy@paquier.xyz
The FE/BE protocol identifies parameters with an Int16 index, which
limits the maximum number of parameters per query to 65535. With
batching added to postges_fdw this limit is much easier to hit, as
the whole batch is essentially a single query, making this error much
easier to hit.
The failures are a bit unpredictable, because it also depends on the
number of columns in the query. So instead of just failing, this patch
tweaks the batch_size to not exceed the maximum number of parameters.
Reported-by: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/OS0PR01MB571603973C0AC2874AD6BF2594299%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Fix handling of NULL host name (possibly by using hostaddr). It
previously crashed. Also, we should look at connhost, not pghost, to
handle multi-host specifications.
Also remove an unnecessary SSL_CTX_free().
Reported-by: Jacob Champion <pchampion@vmware.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/504c276ab6eee000bb23d571ea9b0ced4250774e.camel@vmware.com
An incorrectly-encoded multibyte character near the end of a string
could cause various processing loops to run past the string's
terminating NUL, with results ranging from no detectable issue to
a program crash, depending on what happens to be in the following
memory.
This isn't an issue in the server, because we take care to verify
the encoding of strings before doing any interesting processing
on them. However, that lack of care leaked into client-side code
which shouldn't assume that anyone has validated the encoding of
its input.
Although this is certainly a bug worth fixing, the PG security team
elected not to regard it as a security issue, primarily because
any untrusted text should be sanitized by PQescapeLiteral or
the like before being incorporated into a SQL or psql command.
(If an app fails to do so, the same technique can be used to
cause SQL injection, with probably much more dire consequences
than a mere client-program crash.) Those functions were already
made proof against this class of problem, cf CVE-2006-2313.
To fix, invent PQmblenBounded() which is like PQmblen() except it
won't return more than the number of bytes remaining in the string.
In HEAD we can make this a new libpq function, as PQmblen() is.
It seems imprudent to change libpq's API in stable branches though,
so in the back branches define PQmblenBounded as a macro in the files
that need it. (Note that just changing PQmblen's behavior would not
be a good idea; notably, it would completely break the escaping
functions' defense against this exact problem. So we just want a
version for those callers that don't have any better way of handling
this issue.)
Per private report from houjingyi. Back-patch to all supported branches.
Also "make reformat-dat-files".
The only change worthy of note is that pgindent messed up the formatting
of launcher.c's struct LogicalRepWorkerId, which led me to notice that
that struct wasn't used at all anymore, so I just took it out.
Instead, put them in via a format placeholder. This reduces the
number of distinct translatable messages and also reduces the chances
of typos during translation. We already did this for the system call
arguments in a number of cases, so this is just the same thing taken a
bit further.
Discussion: https://www.postgresql.org/message-id/flat/92d6f545-5102-65d8-3c87-489f71ea0a37%40enterprisedb.com
A (relatively minor) annoyance of ErrorResponse/NoticeResponse messages
as printed by PQtrace() is that their length might vary when we move
error messages from one source file to another, one function to another,
or even when their location line numbers change number of digits.
To avoid having to adjust expected files for some tests, make the
regress mode of PQtrace() suppress the length word of NoticeResponse and
ErrorResponse messages.
Discussion: https://postgr.es/m/20210402023010.GA13563@alvherre.pgsql
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
statements for language SQL with a function body that conforms to the
SQL standard and is portable to other implementations.
Instead of the PostgreSQL-specific AS $$ string literal $$ syntax,
this allows writing out the SQL statements making up the body
unquoted, either as a single statement:
CREATE FUNCTION add(a integer, b integer) RETURNS integer
LANGUAGE SQL
RETURN a + b;
or as a block
CREATE PROCEDURE insert_data(a integer, b integer)
LANGUAGE SQL
BEGIN ATOMIC
INSERT INTO tbl VALUES (a);
INSERT INTO tbl VALUES (b);
END;
The function body is parsed at function definition time and stored as
expression nodes in a new pg_proc column prosqlbody. So at run time,
no further parsing is required.
However, this form does not support polymorphic arguments, because
there is no more parse analysis done at call time.
Dependencies between the function and the objects it uses are fully
tracked.
A new RETURN statement is introduced. This can only be used inside
function bodies. Internally, it is treated much like a SELECT
statement.
psql needs some new intelligence to keep track of function body
boundaries so that it doesn't send off statements when it sees
semicolons that are inside a function body.
Tested-by: Jaime Casanova <jcasanov@systemguards.com.ec>
Reviewed-by: Julien Rouhaud <rjuju123@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/1c11f1eb-f00c-43b7-799d-2d44132c02d7@2ndquadrant.com
By default, have libpq set the TLS extension "Server Name Indication" (SNI).
This allows an SNI-aware SSL proxy to route connections. (This
requires a proxy that is aware of the PostgreSQL protocol, not just
any SSL proxy.)
In the future, this could also allow the server to use different SSL
certificates for different host specifications. (That would require
new server functionality. This would be the client-side functionality
for that.)
Since SNI makes the host name appear in cleartext in the network
traffic, this might be undesirable in some cases. Therefore, also add
a libpq connection option "sslsni" to turn it off.
Discussion: https://www.postgresql.org/message-id/flat/7289d5eb-62a5-a732-c3b9-438cee2cb709%40enterprisedb.com
It seems that in MSVC timeval's tv_sec field is of type long.
localtime() takes a time_t pointer. Since long is 32-bit even on 64-bit
builds in MSVC, passing a long pointer instead of the correct time_t
pointer generated a compiler warning. Fix that.
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CAApHDvoRG25X_=ZCGSPb4KN_j2iu=G2uXsRSg8NBZeuhkOSETg@mail.gmail.com
Similarly to the cryptohash implementations, this refactors the existing
HMAC code into a single set of APIs that can be plugged with any crypto
libraries PostgreSQL is built with (only OpenSSL currently). If there
is no such libraries, a fallback implementation is available. Those new
APIs are designed similarly to the existing cryptohash layer, so there
is no real new design here, with the same logic around buffer bound
checks and memory handling.
HMAC has a dependency on cryptohashes, so all the cryptohash types
supported by cryptohash{_openssl}.c can be used with HMAC. This
refactoring is an advantage mainly for SCRAM, that included its own
implementation of HMAC with SHA256 without relying on the existing
crypto libraries even if PostgreSQL was built with their support.
This code has been tested on Windows and Linux, with and without
OpenSSL, across all the versions supported on HEAD from 1.1.1 down to
1.0.1. I have also checked that the implementations are working fine
using some sample results, a custom extension of my own, and doing
cross-checks across different major versions with SCRAM with the client
and the backend.
Author: Michael Paquier
Reviewed-by: Bruce Momjian
Discussion: https://postgr.es/m/X9m0nkEJEzIPXjeZ@paquier.xyz
It's misplaced there -- it's not libpq's output stream to tweak in that
way. In particular, POSIX says that it has to be called before any
other operation on the file, so if a stream previously used by the
calling application, bad things may happen.
Put setvbuf() in libpq_pipeline for good measure.
Also, reduce fopen(..., "w+") to just fopen(..., "w") in
libpq_pipeline.c. It's not clear that this fixes anything, but we don't
use w+ anywhere.
Per complaints from Tom Lane.
Discussion: https://postgr.es/m/3337422.1617229905@sss.pgh.pa.us
Failing to do this can cause a crash, and I suspect is what has happened
with a buildfarm member reporting mysterious failures.
This is an ancient bug, but I'm not backpatching since evidently nobody
cares about PQtrace in older releases.
Discussion: https://postgr.es/m/3333908.1617227066@sss.pgh.pa.us
We must cast the arguments of <ctype.h> functions to unsigned
char to avoid problems where char is signed.
Speaking of which, considering that this *is* a <ctype.h> function,
it's rather remarkable that we aren't seeing more complaints about
not having included that header.
Per buildfarm.
Remove confusion between time_t and pg_time_t; neither
gettimeofday() nor localtime() deal in the latter.
libpq indeed has no business using <pgtime.h> at all.
Use snprintf not sprintf, to ensure we can't overrun the
supplied buffer. (Unlikely, but let's be safe.)
Per buildfarm.
Transform the PQtrace output format from its ancient (and mostly
useless) byte-level output format to a logical-message-level output,
making it much more usable. This implementation allows the printing
code to be written (as it indeed was) by looking at the protocol
documentation, which gives more confidence that the three (docs, trace
code and actual code) actually match.
Author: 岩田 彩 (Aya Iwata) <iwata.aya@fujitsu.com>
Reviewed-by: 綱川 貴之 (Takayuki Tsunakawa) <tsunakawa.takay@fujitsu.com>
Reviewed-by: Kirk Jamison <k.jamison@fujitsu.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: 黒田 隼人 (Hayato Kuroda) <kuroda.hayato@fujitsu.com>
Reviewed-by: "Nagaura, Ryohei" <nagaura.ryohei@jp.fujitsu.com>
Reviewed-by: Ryo Matsumura <matsumura.ryo@fujitsu.com>
Reviewed-by: Greg Nancarrow <gregn4422@gmail.com>
Reviewed-by: Jim Doty <jdoty@pivotal.io>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/71E660EB361DF14299875B198D4CE5423DE3FBA4@g01jpexmbkw25
Michael Paquier
Alvaro Herrera
Tom Lane
Peter Eisentraut
Noah Misch
Tomas Vondra
Andrew Dunstan
Bruce Momjian
David Rowley
Michael Meskes