I noticed that the sanity checks in the regression tests omitted to
check a couple of "poor man's enum" columns that you'd reasonably
expect them to check.
There are other "char"-type columns in system catalogs that are not
covered by either type_sanity or opr_sanity, e.g. pg_rewrite.ev_type.
However, those catalogs are not populated with any manually-created
data during bootstrap, so it seems less necessary to check them
this way.
A scan for missed proisstrict markings in the core code turned up
these functions:
brin_summarize_new_values
pg_stat_reset_single_table_counters
pg_stat_reset_single_function_counters
pg_create_logical_replication_slot
pg_create_physical_replication_slot
pg_drop_replication_slot
The first three of these take OID, so a null argument will normally look
like a zero to them, resulting in "ERROR: could not open relation with OID
0" for brin_summarize_new_values, and no action for the pg_stat_reset_XXX
functions. The other three will dump core on a null argument, though this
is mitigated by the fact that they won't do so until after checking that
the caller is superuser or has rolreplication privilege.
In addition, the pg_logical_slot_get/peek[_binary]_changes family was
intentionally marked nonstrict, but failed to make nullness checks on all
the arguments; so again a null-pointer-dereference crash is possible but
only for superusers and rolreplication users.
Add the missing ARGISNULL checks to the latter functions, and mark the
former functions as strict in pg_proc. Make that change in the back
branches too, even though we can't force initdb there, just so that
installations initdb'd in future won't have the issue. Since none of these
bugs rise to the level of security issues (and indeed the pg_stat_reset_XXX
functions hardly misbehave at all), it seems sufficient to do this.
In addition, fix some order-of-operations oddities in the slot_get_changes
family, mostly cosmetic, but not the part that moves the function's last
few operations into the PG_TRY block. As it stood, there was significant
risk for an error to exit without clearing historical information from
the system caches.
The slot_get_changes bugs go back to 9.4 where that code was introduced.
Back-patch appropriate subsets of the pg_proc changes into all active
branches, as well.
Given syntactically wrong input, widget_in() could call atof() with an
indeterminate pointer argument, typically leading to a crash; or if it
didn't do that, it might return a NULL pointer, which again would lead
to a crash since old-style C functions aren't supposed to do things
that way. Fix that by correcting the off-by-one syntax test and
throwing a proper error rather than just returning NULL.
Also, since widget_in and widget_out have been marked STRICT for a
long time, their tests for null inputs are just dead code; remove 'em.
In the oldest branches, also improve widget_out to use snprintf not
sprintf, just to be sure.
In passing, get rid of a long-since-useless sprintf into a local buffer
that nothing further is done with, and make some other minor coding
style cleanups.
In the intended regression-testing usage of these functions, none of
this is very significant; but if the regression test database were
left around in a production installation, these bugs could amount
to a minor security hazard.
Piotr Stefaniak, Michael Paquier, and Tom Lane
These functions readily crash when passed a NULL input value. The tests
themselves do not pass NULL values to them; but when the regression
database is used as a basis for fuzz testing, they cause a lot of noise.
Also, if someone were to leave a regression database lying about in a
production installation, these would create a minor security hazard.
Andreas Seltenreich
Replay of XLOG_BTREE_VACUUM during Hot Standby was previously thought to require
complex interlocking that matched the requirements on the master. This required
an O(N) operation that became a significant problem with large indexes, causing
replication delays of seconds or in some cases minutes while the
XLOG_BTREE_VACUUM was replayed.
This commit skips the “pin scan” that was previously required, by observing in
detail when and how it is safe to do so, with full documentation. The pin scan
is skipped only in replay; the VACUUM code path on master is not touched here.
The current commit still performs the pin scan for toast indexes, though this
can also be avoided if we recheck scans on toast indexes. Later patch will
address this.
No tests included. Manual tests using an additional patch to view WAL records
and their timing have shown the change in WAL records and their handling has
successfully reduced replication delay.
Further portability fix for a967613911. Mingw- and MSVC-based builds
appear to be working fine, but Cygwin needs an extra tweak whereby the
new win32security.c file is explicitely added to the list of files to
build in pgport, per Cygwin members brolga and lorikeet.
Author: Michael Paquier
Improve comments and make it a shade less messy. I think we might want
to move all of this somewhere else later, but it needs to be more
readable first.
In passing, re-pgindent the file, affecting some recently-added comments
concerning parallel query planning.
Once upon a time it was necessary for grouping_planner() to determine
the tlist it wanted from the scan/join plan subtree before it called
query_planner(), because query_planner() would actually make a Plan using
that. But we refactored things a long time ago to delay construction of
the Plan tree till later, so there's no need to build that tlist until
(and indeed unless) we're ready to plaster it onto the Plan. The only
thing query_planner() cares about is what Vars are going to be needed for
the tlist, and it can perfectly well get that by looking at the real tlist
rather than some masticated version.
Well, actually, there is one minor glitch in that argument, which is that
make_subplanTargetList also adds Vars appearing only in HAVING to the
tlist it produces. So now we have to account for HAVING explicitly in
build_base_rel_tlists. But that just adds a few lines of code, and
I doubt it moves the needle much on processing time; we might be doing
pull_var_clause() twice on the havingQual, but before we had it scanning
dummy tlist entries instead.
This is a very small down payment on rationalizing grouping_planner
enough so it can be refactored.
Turns out the only reason initdb -X worked is that pg_mkdir_p won't
whine if you point it at something that's a symlink to a directory.
Otherwise, the attempt to create pg_xlog/ just like all the other
subdirectories would have failed. Let's be a little more explicit
about what's happening. Oversight in my patch for bug #13853
(mea culpa for not testing -X ...)
When we're creating subdirectories of PGDATA during initdb, we know darn
well that the parent directory exists (or should exist) and that the new
subdirectory doesn't (or shouldn't). There is therefore no need to use
anything more complicated than mkdir(). Using pg_mkdir_p() just opens us
up to unexpected failure modes, such as the one exhibited in bug #13853
from Nuri Boardman. It's not very clear why pg_mkdir_p() went wrong there,
but it is clear that we didn't need to be trying to create parent
directories in the first place. We're not even saving any code, as proven
by the fact that this patch nets out at minus five lines.
Since this is a response to a field bug report, back-patch to all branches.
This new view provides insight into the state of a running WAL receiver
in a HOT standby node.
The information returned includes the PID of the WAL receiver process,
its status (stopped, starting, streaming, etc), start LSN and TLI, last
received LSN and TLI, timestamp of last message send and receipt, latest
end-of-WAL LSN and time, and the name of the slot (if any).
Access to the detailed data is only granted to superusers; others only
get the PID.
Author: Michael Paquier
Reviewer: Haribabu Kommi
Commit e710b65c inserted code in md5_crypt_verify to disable and later
re-enable interrupts, with a CHECK_FOR_INTERRUPTS call as part of the
second step, to process any interrupts that had been held off. Commit
6647248e removed the interrupt disable/re-enable code, but left behind
the CHECK_FOR_INTERRUPTS, even though this is now an entirely random,
pointless place for one. md5_crypt_verify doesn't run long enough to
need such a check, and if it did, this would still be the wrong place
to put one.
We tell people to examine the postmaster log if they're unsure why they are
getting auth failures, but actually only a few relatively-uncommon failure
cases were given their own log detail messages in commit 64e43c59b8.
Expand on that so that every failure case detected within md5_crypt_verify
gets a specific log detail message. This should cover pretty much every
ordinary password auth failure cause.
So far I've not noticed user demand for a similar level of auth detail
for the other auth methods, but sooner or later somebody might want to
work on them. This is not that patch, though.
pg_ctl is using isatty() to verify whether the process is running in a
terminal, and if not it sends its output to Windows' Event Log ... which
does the wrong thing when the output has been redirected to a pipe, as
reported in bug #13592.
To fix, make pg_ctl use the code we already have to detect service-ness:
in the master branch, move src/backend/port/win32/security.c to src/port
(with suitable tweaks so that it runs properly in backend and frontend
environments); pg_ctl already has access to pgport so it Just Works. In
older branches, that's likely to cause trouble, so instead duplicate the
required code in pg_ctl.c.
Author: Michael Paquier
Bug report and diagnosis: Egon Kocjan
Backpatch: all supported branches
Although these temp tables will get removed from template1 at the end of
the standalone-backend run, that's too late to keep them from getting
copied into the template0 and postgres databases, now that we use only a
single backend run for the whole sequence. While no real harm is done
by the extra copies (since they'd be deleted on first use of the temp
schema), it's still unsightly, and it would mean some wasted cycles for
every database creation for the life of the installation.
Oversight in commit c4a8812cf6. Noticed by Amit Langote.
In commit 921191912c I claimed that we weren't testing encoding
conversion functions, but further poking around reveals that we did
have an equivalent though hard-wired set of tests in conversion.sql.
AFAICS there is no advantage to doing it like that as compared to letting
the catalog contents drive the test, so let the opr_sanity addition stand
and remove the now-redundant tests in conversion.sql.
Also, remove some infrastructure in src/backend/utils/mb/conversion_procs
for building conversion.sql's list of tests. That was unmaintained, and
had not corresponded to the actual contents of conversion.sql since 2007
or perhaps even further back.
The order of inclusion of .o files makes a difference in linker output;
not a functional difference, but still a bitwise difference, which annoys
some packagers who would like reproducible builds.
Report and patch by Christoph Berg
A pointless and confusing error message is shown to the user when
attempting to identify a 9.3 or older remote server with a 9.5/9.6
pg_receivexlog, because the return signature of IDENTIFY_SYSTEM was
changed in 9.4. There's no good reason for the warning message, so
shuffle code around to keep it quiet.
(pg_recvlogical is also affected by this commit, but since it obviously
cannot work with 9.3 that doesn't actually matter much.)
Backpatch to 9.5.
Reported by Marco Nenciarini, who also wrote the initial patch. Further
tweaked by Robert Haas and Fujii Masao; reviewed by Michael Paquier and
Craig Ringer.
In light of commit ea0d494dae, it seems like a good idea to add
a regression test that will complain about random functions taking or
returning cstring. Only I/O support functions and encoding conversion
functions should be declared that way.
While at it, add some checks that encoding conversion functions are
declared properly. Since pg_conversion isn't populated manually,
it's not quite as necessary to check its contents as it is for catalogs
like pg_proc; but one thing we definitely have not tested in the past
is whether the identified conproc for a conversion actually does that
conversion vs. some other one.
Using cstring as the input type was a poor decision, because that's not
really a full-fledged type. In particular, it lacks implicit coercions
from text or varchar, meaning that usages like to_regproc('foo'||'bar')
wouldn't work; basically the only case that did work without explicit
casting was a simple literal constant argument.
The lack of field complaints about this suggests that hardly anyone
is using these functions, so hopefully fixing it won't cause much of
a compatibility problem. They've only been there since 9.4, anyway.
Petr Korobeinikov
While the in-core authentication mechanism doesn't need to access
pg_shseclabel at all, it's reasonable to think that an authentication
hook will want to look at the label for the role logging in, or for rows
in other catalogs used during the authentication phase of startup.
Catalog version bumped, because this changes the "is nailed" status for
pg_shseclabel.
Author: Adam Brightwell
This requires adding some more infrastructure to handle both case-sensitive
and case-insensitive matching, as well as the ability to match a prefix of
a previous word. So it ends up being about a wash line-count-wise, but
it's just as big a readability win here as in the SQL tab completion rules.
Michael Paquier, some adjustments by me
In the refactoring in commit d37b816dc9,
we mostly kept to the original design whereby only the last few words
on the line were matched to identify a completable pattern. However,
after commit d854118c8d, there's really
no reason to do it like that: where it's sensible, we can use patterns
that expect to match the entire input line. And mostly, it's sensible.
Matching the entire line greatly reduces the odds of a false match that
leads to offering irrelevant completions. Moreover (though I've not
tried to measure this), it should make tab completion faster since
many of the patterns will be discarded after a single integer comparison
that finds that the wrong number of words appear on the line.
There are certain identifiable places where we still need to use
TailMatches because the statement in question is allowed to appear
embedded in a larger statement. These are just a small minority of
the existing patterns, though, so the benefit of switching where
possible is large.
It's possible that this patch has removed some within-line matching
behaviors that are in fact desirable, but we can put those back when
we get complaints. Most of the removed behaviors are certainly silly.
Michael Paquier, with some further adjustments by me
Commit 43cd468cf0 added some wording to create_policy.sgml purporting
to warn users against a race condition of the sort that had been noted some
time ago by Peter Geoghegan. However, that warning was far too vague to be
useful (or at least, I completely failed to grasp what it was on about).
Since the problem case occurs with a security design pattern that lots of
people are likely to try to use, we need to be as clear as possible about
it. Provide a concrete example in the main-line docs in place of the
original warning.
Some time back we agreed that row_security=off should not be a way to
bypass RLS entirely, but only a way to get an error if it was being
applied. However, the code failed to act that way for table owners.
Per discussion, this is a must-fix bug for 9.5.0.
Adjust the logic in rls.c to behave as expected; also, modify the
error message to be more consistent with the new interpretation.
The regression tests need minor corrections as well. Also update
the comments about row_security in ddl.sgml to be correct. (The
official description of the GUC in config.sgml is already correct.)
I failed to resist the temptation to do some other very minor
cleanup as well, such as getting rid of a duplicate extern declaration.
Aside from any consistency arguments, this is logically necessary because
the I/O functions for these types also handle numeric OID values. Without
a quoting rule it is impossible to distinguish numeric OIDs from role or
namespace names that happen to contain only digits.
Also change the to_regrole and to_regnamespace functions to dequote their
arguments. While not logically essential, this seems like a good idea
since the other to_reg* functions do it. Anyone who really wants raw
lookup of an uninterpreted name can fall back on the time-honored solution
of (SELECT oid FROM pg_namespace WHERE nspname = whatever).
Report and patch by Jim Nasby, reviewed by Michael Paquier
Can't release the AccessExclusiveLock on the target table until commit.
Otherwise there is a race condition whereby other backends might service
our cache invalidation signals before they can actually see the updated
catalog rows.
Just to add insult to injury, RemovePolicyById was closing the rel (with
incorrect lock drop) and then passing the now-dangling rel pointer to
CacheInvalidateRelcache. Probably the reason this doesn't fall over on
CLOBBER_CACHE buildfarm members is that some outer level of the DROP logic
is still holding the rel open ... but it'd have bit us on the arse
eventually, no doubt.
The CHECK_IS_BINARY_UPGRADE macro is not sufficient security protection
if we're going to dereference pass-by-reference arguments before it.
But in any case we really need to explicitly check PG_ARGISNULL for all
the arguments of a non-strict function, not only the ones we expect null
values for.
Oversight in commits 30982be4e5 and
f92fc4c95d. Found by Andreas Seltenreich.
(The other usages in pg_upgrade_support.c seem safe.)
pgwin32_recv() has treated a non-error return of zero bytes from WSARecv()
as being a reason to block ever since the current implementation was
introduced in commit a4c40f140d. However, so far as one can tell
from Microsoft's documentation, that is just wrong: what it means is
graceful connection closure (in stream protocols) or receipt of a
zero-length message (in message protocols), and neither case should result
in blocking here. The only reason the code worked at all was that control
then fell into the retry loop, which did *not* treat zero bytes specially,
so we'd get out after only wasting some cycles. But as of 9.5 we do not
normally reach the retry loop and so the bug is exposed, as reported by
Shay Rojansky and diagnosed by Andres Freund.
Remove the unnecessary test on the byte count, and rearrange the code
in the retry loop so that it looks identical to the initial sequence.
Back-patch to 9.5. The code is wrong all the way back, AFAICS, but
since it's relatively harmless in earlier branches we'll leave it alone.
Commit c7e27becd2 fixed this on the backend side, but we neglected
the fact that several code paths in pg_dump were printing reloptions
values that had not gotten massaged by ruleutils. Apply essentially the
same quoting logic in those places, too.
spg_text_inner_consistent is capable of reconstructing an empty string
to pass down to the next index level; this happens if we have an empty
string coming in, no prefix, and a dummy node label. (In practice, what
is needed to trigger that is insertion of a whole bunch of empty-string
values.) Then, we will arrive at the next level with in->level == 0
and a non-NULL (but zero length) in->reconstructedValue, which is valid
but the Assert tests weren't expecting it.
Per report from Andreas Seltenreich. This has no impact in non-Assert
builds, so should not be a problem in production, but back-patch to
all affected branches anyway.
In passing, remove a couple of useless variable initializations and
shorten the code by not duplicating DatumGetPointer() calls.
As pointed out by Michael Paquier, recovery_min_apply_delay didn't exist
in 9.0-9.3, making the release note text not very useful. Instead make it
talk about recovery_target_xid, which did exist then.
9.0 is already out of support, but we can fix the text in the newer
branches' copies of its release notes.
The need for this is shown by the files it missed in Bruce's recent run.
I fixed it so that it will actually adjust the case when needed.
In passing, also make it skip .po files, since those will just get
overwritten anyway from the translation repository.
On closer inspection, the reason copyright.pl was missing files is
that it is looking for 'Copyright (c)' and they had 'Copyright (C)'.
Fix that, and update a couple more that grepping for that revealed.
Tom Lane