Not all messages that libpq received from the server would be sent
through our message tracing logic. This commit tries to fix that by
introducing a new function pqParseDone which make it harder to forget
about doing so.
The messages that we now newly send through our tracing logic are:
- CopyData (received by COPY TO STDOUT)
- Authentication requests
- NegotiateProtocolVersion
- Some ErrorResponse messages during connection startup
- ReadyForQuery when received after a FunctionCall message
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
libpq checks the permissions of the password file before opening it.
The way this is done in two separate operations, a static analyzer
would flag as a time-of-check-time-of-use violation. In practice, you
can't do anything with that, but it still seems better style to fix
it.
To fix it, open the file first and then check the permissions on the
opened file handle.
Reviewed-by: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/a3356054-14ae-4e7a-acc6-249d19dac20b%40eisentraut.org
This commit attempts to update a few places, such as the money,
numeric, and timestamp types, to no longer rely on signed integer
wrapping for correctness. This is intended to move us closer
towards removing -fwrapv, which may enable some compiler
optimizations. However, there is presently no plan to actually
remove that compiler option in the near future.
Besides using some of the existing overflow-aware routines in
int.h, this commit introduces and makes use of some new ones.
Specifically, it adds functions that accept a signed integer and
return its absolute value as an unsigned integer with the same
width (e.g., pg_abs_s64()). It also adds functions that accept an
unsigned integer, store the result of negating that integer in a
signed integer with the same width, and return whether the negation
overflowed (e.g., pg_neg_u64_overflow()).
Finally, this commit adds a couple of tests for timestamps near
POSTGRES_EPOCH_JDATE.
Author: Joseph Koshakow
Reviewed-by: Tom Lane, Heikki Linnakangas, Jian He
Discussion: https://postgr.es/m/CAAvxfHdBPOyEGS7s%2Bxf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw%40mail.gmail.com
ecpg's lexer and parser files aren't normally processed by
pgindent, and unsurprisingly there's a lot of code in there
that doesn't really match project style. I spent some time
running pgindent over the fragments of these files that are
C code, and this is the result. This is in the same spirit
as commit 30ed71e42, though apparently Peter used a different
method for that one, since it didn't find these problems.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
If tracing was enabled during connection startup, these messages would
previously be listed in the trace output as something like this:
F 54 Unknown message: 70
mismatched message length: consumed 4, expected 54
With this commit their type and contents are now correctly listed:
F 36 StartupMessage 3 0 "user" "foo" "database" "alvherre"
F 54 SASLInitialResponse "SCRAM-SHA-256" 32 'n,,n=,r=nq5zEPR/VREHEpOAZzH8Rujm'
F 108 SASLResponse 'c=biws,r=nq5zEPR/VREHEpOAZzH8RujmVtWZDQ8glcrvy9OMNw7ZqFUn,p=BBwAKe0WjSvigB6RsmmArAC+hwucLeuwJrR5C/HQD5M='
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
Some code using atol() would not work correctly if sizeof(long)==4:
- src/bin/pg_basebackup/pg_basebackup.c: Would miscount size of a
tablespace over 2 TB.
- src/bin/pg_basebackup/streamutil.c: Would truncate a timeline ID
beyond INT32_MAX.
- src/bin/pg_rewind/libpq_source.c: Would miscount size of files
larger than 2 GB (but this currently cannot happen).
Replace these with atoll().
In one case, the use of atol() did not result in incorrect behavior
but seems inconsistent with related code:
- src/interfaces/ecpg/ecpglib/execute.c: Gratuitous, since it
processes a value from pg_type.typlen, which is int16.
Replace this with atoi().
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/a52738ad-06bc-4d45-b59f-b38a8a89de49%40eisentraut.org
libpq tracing via PQtrace would uselessly print the wrong thing for
these types of messages. With this commit, their type and contents
would be correctly listed. (This can be verified with PQconnectStart(),
but we don't use that in libpq_pipeline, so I (Álvaro) haven't bothered
to add any tests.)
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
In future commits we're going to trace authentication related messages.
Some of these messages contain challenge bytes as part of a
challenge-response flow. Since these bytes are different for every
connection, we want to normalize them when the PQTRACE_REGRESS_MODE
trace flag is set. This commit modifies pqTraceOutputNchar to take a
suppress argument, which makes it possible to do so.
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
This warning flag detects global variables not declared in header
files. This is similar to what -Wmissing-prototypes does for
functions. (More correctly, it is similar to what
-Wmissing-declarations does for functions, but -Wmissing-prototypes is
a superset of that in C.)
This flag is new in GCC 14. Clang has supported it for a while.
Several recent commits have cleaned up warnings triggered by this, so
it should now be clean.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/e0a62134-83da-4ba4-8cdb-ceb0111c95ce@eisentraut.org
Before Bison 3.4, the generated parser implementation files run afoul
of -Wmissing-variable-declarations (in spite of commit ab61c40bfa2)
because declarations for yylval and possibly yylloc are missing. The
generated header files contain an extern declaration, but the
implementation files don't include the header files. Since Bison 3.4,
the generated implementation files automatically include the generated
header files, so then it works.
To make this work with older Bison versions as well, include the
generated header file from the .y file.
(With older Bison versions, the generated implementation file contains
effectively a copy of the header file pasted in, so including the
header file is redundant. But we know this works anyway because the
core grammar uses this arrangement already.)
Discussion: https://www.postgresql.org/message-id/flat/e0a62134-83da-4ba4-8cdb-ceb0111c95ce@eisentraut.org
Commit 453c4687377 introduced a use of strerror() into libpq, but that
is not thread-safe. Fix by using strerror_r() instead.
In passing, update some of the code comments added by 453c4687377, as
we have learned more about the reason for the change in OpenSSL that
started this.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: Discussion: https://postgr.es/m/b6fb018b-f05c-4afd-abd3-318c649faf18@highgo.ca
The new test tests the libpq fallback behavior on an early error,
which was fixed in the previous commit.
This adds an IS_INJECTION_POINT_ATTACHED() macro, to allow writing
injected test code alongside the normal source code. In principle, the
new test could've been implemented by an extra test module with a
callback that sets the FrontendProtocol global variable, but I think
it's more clear to have the test code right where the injection point
is, because it has pretty intimate knowledge of the surrounding
context it runs in.
Reviewed-by: Michael Paquier
Discussion: https://www.postgresql.org/message-id/CAOYmi%2Bnwvu21mJ4DYKUa98HdfM_KZJi7B1MhyXtnsyOO-PB6Ww%40mail.gmail.com
With sslmode=prefer, the desired behavior is to completely fail the
connection attempt, *not* fall back to a plaintext connection, if the
server responds to the SSLRequest with an error ('E') response instead
of rejecting SSL with an 'N' response. This was broken in commit
05fd30c0e7.
Reported-by: Jacob Champion
Reviewed-by: Michael Paquier
Discussion: https://www.postgresql.org/message-id/CAOYmi%2Bnwvu21mJ4DYKUa98HdfM_KZJi7B1MhyXtnsyOO-PB6Ww%40mail.gmail.com
Backpatch-through: 17
This adds extern declarations for some global variables produced by
Bison that are not already declared in its generated header file.
This is a workaround to be able to add -Wmissing-variable-declarations
to the global set of warning options in the near future.
Another longer-term solution would be to convert these grammars to
"pure" parsers in Bison, to avoid global variables altogether. Note
that the core grammar is already pure, so this patch did not need to
touch it.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/e0a62134-83da-4ba4-8cdb-ceb0111c95ce@eisentraut.org
This is a continuation of 0c1aca461481, with some cleanup in:
- msvc_gendef.pl
- pgindent
- 005_negotiate_encryption.pl, as of an oversight of d39a49c1e459 that
has removed %params in test_matrix(), making also $server_config
useless.
Author: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/87wmm4dkci.fsf@wibble.ilmari.org
When an OOM occurred, this function was incorrectly setting a status of
CONNECTION_BAD on the passed in PGconn instead of on the newly created
PGcancelConn.
Mistake introduced with 61461a300c1c. Backpatch to 17.
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20240630190040.26.nmisch@google.com
Commit f5e4dedfa exposed libpq's internal function PQsocketPoll
without a lot of thought about whether that was an API we really
wanted to chisel in stone. The main problem with it is the use of
time_t to specify the timeout. While we do want an absolute time
so that a loop around PQsocketPoll doesn't have problems with
timeout slippage, time_t has only 1-second resolution. That's
already problematic for libpq's own internal usage --- for example,
pqConnectDBComplete has long had a kluge to treat "connect_timeout=1"
as 2 seconds so that it doesn't accidentally round to nearly zero.
And it's even less likely to be satisfactory for external callers.
Hence, let's change this while we still can.
The best idea seems to be to use an int64 count of microseconds since
the epoch --- basically the same thing as the backend's TimestampTz,
but let's use the standard Unix epoch (1970-01-01) since that's more
likely for clients to be easy to calculate. Millisecond resolution
would be plenty for foreseeable uses, but maybe the day will come that
we're glad we used microseconds.
Also, since time(2) isn't especially helpful for computing timeouts
defined this way, introduce a new function PQgetCurrentTimeUSec
to get the current time in this form.
Remove the hack in pqConnectDBComplete, so that "connect_timeout=1"
now means what you'd expect.
We can also remove the "#include <time.h>" that f5e4dedfa added to
libpq-fe.h, since there's no longer a need for time_t in that header.
It seems better for v17 not to enlarge libpq-fe.h's include footprint
from what it's historically been, anyway.
I also failed to resist the temptation to do some wordsmithing
on PQsocketPoll's documentation.
Patch by me, per complaint from Dominique Devienne.
Discussion: https://postgr.es/m/913559.1718055575@sss.pgh.pa.us
Follow-up to 87d2801d4b: That commit restored some lost error
messages, but they ended up in a place where xgettext wouldn't find
them. Rather than elevating ENCRYPTION_NEGOTIATION_FAILED() to a
gettext trigger, it's easiest for now to put in some explicit
libpq_gettext() calls in the couple of call sites.
This one error message is just a workaround for a missing OpenSSL
error string. But OpenSSL does not have gettext support, so we don't
need to provide it in our workaround either. That way, the
user-facing behavior is consistent whether the user has a fixed
OpenSSL or not.
Commit faff8f8e47 allowed integer literals to contain underscores, but
failed to update the lexer's "numericfail" rule. As a result, a
decimal integer literal containing underscores would fail to parse, if
used in an integer range with no whitespace after the first number,
such as "1_001..1_003" in a PL/pgSQL FOR loop.
Fix and backpatch to v16, where support for underscores in integer
literals was added.
Report and patch by Erik Wienhold.
Discussion: https://postgr.es/m/808ce947-46ec-4628-85fa-3dd600b2c154%40ewie.name
Coverity complains that ECPGdebug is accessing debugstream without
holding debug_mutex, which is a fair complaint: we should take
debug_mutex while changing the settings ecpg_log looks at.
In some branches it also complains about unlocked use of simple_debug.
I think it's intentional and safe to have a quick unlocked check of
simple_debug at the start of ecpg_log, since that early exit will
always be taken in non-debug cases. But we should recheck
simple_debug after acquiring the mutex. In the worst case, calling
ECPGdebug concurrently with ecpg_log in another thread could result
in a null-pointer dereference due to debugstream transiently being
NULL while simple_debug isn't 0.
This is largely hypothetical, since it's unlikely anybody uses
ECPGdebug() at all in the field, and our own regression tests
don't seem to be hitting the theoretical race conditions either.
Still, if we're going to the trouble of having mutexes here, we ought
to be using them in a way that's actually safe not just almost safe.
Hence, back-patch to all supported branches.
After further review, we want to move in the direction of always
quoting GUC names in error messages, rather than the previous (PG16)
wildly mixed practice or the intermittent (mid-PG17) idea of doing
this depending on how possibly confusing the GUC name is.
This commit applies appropriate quotes to (almost?) all mentions of
GUC names in error messages. It partially supersedes a243569bf65 and
8d9978a7176, which had moved things a bit in the opposite direction
but which then were abandoned in a partial state.
Author: Peter Smith <smithpb2250@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAHut%2BPv-kSN8SkxSdoHano_wPubqcg5789ejhCDZAcLFceBR-w%40mail.gmail.com
There were three problems with the sslnegotiation options:
1. The sslmode=prefer and sslnegotiation=requiredirect combination was
somewhat dangerous, as you might unintentionally fall back to
plaintext authentication when connecting to a pre-v17 server.
2. There was an asymmetry between 'postgres' and 'direct'
options. 'postgres' meant "try only traditional negotiation", while
'direct' meant "try direct first, and fall back to traditional
negotiation if it fails". That was apparent only if you knew that the
'requiredirect' mode also exists.
3. The "require" word in 'requiredirect' suggests that it's somehow
more strict or more secure, similar to sslmode. However, I don't
consider direct SSL connections to be a security feature.
To address these problems:
- Only allow sslnegotiation='direct' if sslmode='require' or
stronger. And for the record, Jacob and Robert felt that we should do
that (or have sslnegotiation='direct' imply sslmode='require') anyway,
regardless of the first issue.
- Remove the 'direct' mode that falls back to traditional negotiation,
and rename what was called 'requiredirect' to 'direct' instead. In
other words, there is no "try both methods" option anymore, 'postgres'
now means the traditional negotiation and 'direct' means a direct SSL
connection.
Reviewed-by: Jelte Fennema-Nio, Robert Haas, Jacob Champion
Discussion: https://www.postgresql.org/message-id/d3b1608a-a1b6-4eda-9ec5-ddb3e4375808%40iki.fi
Underscores were added to numeric literals in faff8f8e47. This change
also affected the positional parameters (e.g., $1) rule, which uses
the same production for its digits. But this did not actually work,
because the digits for parameters are processed using atol(), which
does not handle underscores and ignores whatever it cannot parse.
The underscores notation is probably not useful for positional
parameters, so for simplicity revert that rule to its old form that
only accepts digits 0-9.
Author: Erik Wienhold <ewie@ewie.name>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/5d216d1c-91f6-4cbe-95e2-b4cbd930520c%40ewie.name
Run pgindent, pgperltidy, and reformat-dat-files.
The pgindent part of this is pretty small, consisting mainly of
fixing up self-inflicted formatting damage from patches that
hadn't bothered to add their new typedefs to typedefs.list.
In order to keep it from making anything worse, I manually added
a dozen or so typedefs that appeared in the existing typedefs.list
but not in the buildfarm's list. Perhaps we should formalize that,
or better find a way to get those typedefs into the automatic list.
pgperltidy is as opinionated as always, and reformat-dat-files too.
libpq's pqTraceOutputMessage() used to look like this:
case 'Z': /* Ready For Query */
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;
Commit f4b54e1ed98 introduced macros for protocol characters, so now
it looks like this:
case PqMsg_ReadyForQuery:
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;
But this introduced a disconnect between the symbol in the switch case
and the function name to be called, so this made the manageability of
this file a bit worse.
This patch changes the function names to match, so now it looks like
this:
case PqMsg_ReadyForQuery:
pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, &logCursor);
break;
(This also improves the readability of the file in general, since some
function names like "pqTraceOutputt" were a little hard to read
accurately.)
Some protocol characters have different meanings to and from the
server. The old code structure had a common function for both, for
example, pqTraceOutputD(). The new structure splits this up into
separate ones to match the protocol message name, like
pqTraceOutput_Describe() and pqTraceOutput_DataRow().
Reviewed-by: Yugo NAGATA <nagata@sraoss.co.jp>
Discussion: https://www.postgresql.org/message-id/flat/575e4f9d-acfe-45e3-b7f1-7e32c579090e%40eisentraut.org
If the client supports ALPN but tries to use some other protocol, like
HTTPS, reject the connection in the server. That is surely a confusion
of some sort. Furthermore, the ALPN RFC 7301 says:
> In the event that the server supports no protocols that the client
> advertises, then the server SHALL respond with a fatal
> "no_application_protocol" alert.
This commit makes the server follow that advice.
In the client, specifically check for the OpenSSL error code for the
"no_application_protocol" alert. Otherwise you got a cryptic "SSL
error: SSL error code 167773280" error if you tried to connect to a
non-PostgreSQL server that rejects the connection with
"no_application_protocol". ERR_reason_error_string() returns NULL for
that code, which frankly seems like an OpenSSL bug to me, but we can
easily print a better message ourselves.
Reported-by: Jacob Champion
Discussion: https://www.postgresql.org/message-id/6aedcaa5-60f3-49af-a857-2c76ba55a1f3@iki.fi
These messages were lost in commit 05fd30c0e7. Put them back.
This makes one change in the error message behavior compared to v16,
in the case that the server responds to GSSRequest with an error
instead of rejecting it with 'N'. Previously, libpq would hide the
error that the server sent, assuming that you got the error because
the server is an old pre-v12 version that doesn't understand the
GSSRequest message. A v11 server sends a "FATAL: unsupported frontend
protocol 1234.5680: server supports 2.0 to 3.0" error if you try to
connect to it with GSS. That was a reasonable assumption when the
feature was introduced, but v12 was released a long time ago and I
don't think it's the most probable cause anymore. The attached patch
changes things so that libpq prints the error message that the server
sent in that case, making the "server responds with error to
GSSRequest" case behave the same as the "server responds with error to
SSLRequest" case.
Reported-by: Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/bb3b94da-afc7-438d-8940-cb946e553d9d@eisentraut.org
The ECPG and main 2PC tests have been using rather-generic names for the
prepared transactions they generate. This commit switches the 2PC
transactions to use more complex GIDs, reducing the risk of naming
conflicts.
The main 2PC tests also include scans of pg_prepared_xacts that do not
apply filters on the GID of the prepared transactions, making it
possible to fail the test when any 2PC transaction runs concurrently.
The CI has been able to see such failures with an installcheck
running the ECPG and the main regression test suites in parallel. The
queries on pg_prepared_xacts gain quals to only look after the GIDs
generated locally.
The race is very hard to reproduce, so no backbatch is done for now.
Reported-by: Richard Guo
Discussion: https://postgr.es/m/CAMbWs4-mWCGbbE_bne5=AfqjYGDaUZmjCw2+soLjrdNA0xUDFw@mail.gmail.com
The documentation says that PQsslAttribute(conn, "alpn") returns an
empty string if ALPN is not used, but the code actually returned
NULL. Fix the code to match the documentation.
Reported-by: Michael Paquier
Discussion: https://www.postgresql.org/message-id/ZideNHji0G4gxmc3@paquier.xyz
The paragraph in the docs and the comment applied to
sslnegotiaton=direct, but not sslnegotiation=requiredirect. In
'requiredirect' mode, negotiated SSL is never used. Move the paragraph
in the docs under the description of 'direct' mode, and rephrase it.
Also the comment's reference to reusing a plaintext connection was
bogus. Authentication failure in plaintext mode only happens after
sending the startup packet, so the connection cannot be reused.
Reported-by: Jacob Champion
Discussion: https://www.postgresql.org/message-id/CAOYmi+=sj+1uydS0NR4nYzw-LRWp3Q-s5speBug5UCLSPMbvGA@mail.gmail.com
HAVE_LONG_LONG_INT was not added to ecpg_config.h.in by the meson
build system, but rather than add it there, we decided to remove it
from the makefile build system, to make both consistent that way.
There is no documentation or examples that suggest that the presence
of this symbol was publicly advertised, and of course the feature is
required by C99 (but we don't necessarily require C99 for ecpg user
code). ecpg core code and ecpg tests use the symbol
HAVE_LONG_LONG_INT_64 instead, which is still there.
Discussion: https://www.postgresql.org/message-id/flat/bf35d032-02fc-4173-9f4f-840999cc3ef3%40eisentraut.org
