On 01/03/2024 07:19, Jelte Fennema-Nio wrote:
I think this behaviour makes more sense as a minor protocol version
bump instead of a parameter.
Ok, the consensus is to bump the minor protocol version for this. Works
for me.
+ if (strcmp(conn->workBuffer.data, "_pq_.extended_query_cancel") == 0)
+ {
+ /* that's ok */
+ continue;
+ }
Please see this thread[1], which in the first few patches makes
pqGetNegotiateProtocolVersion3 actually usable for extending the
protocol. I started that, because very proposed protocol change that's
proposed on the list has similar changes to
pqGetNegotiateProtocolVersion3 and I think we shouldn't make these
changes ad-hoc hacked into the current code, but actually do them once
in a way that makes sense for all protocol changes.
Thanks, I will take a look and respond on that thread.
Documentation is still missing
I think at least protocol message type documentation would be very
helpful in reviewing, because that is really a core part of this
change.
Added some documentation. There's more work to be done there, but at
least the message type descriptions are now up-to-date.
Based on the current code I think it should have a few changes:
+ int cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);
+
+ conn->be_cancel_key = malloc(cancel_key_len);
+ if (conn->be_cancel_key == NULL)
This is using the message length to determine the length of the cancel
key in BackendKey. That is not something we generally do in the
protocol. It's even documented: "Notice that although each message
includes a byte count at the beginning, the message format is defined
so that the message end can be found without reference to the byte
count." So I think the patch should be changed to include the length
of the cancel key explicitly in the message.
The nice thing about relying on the message length was that we could
just redefine the CancelRequest message to have a variable length
secret, and old CancelRequest with 4-byte secret was compatible with the
new definition too. But it doesn't matter much, so added an explicit
length field.
FWIW I don't think that restriction makes sense. Any code that parses
the messages needs to have the message length available, and I don't
think it helps with sanity checking that much. I think the documentation
is a little anachronistic. The real reason that all the message types
include enough information to find the message end is that in protocol
version 2, there was no message length field. The only exception that
doesn't have that property is CopyData, and it's no coincidence that it
was added in protocol version 3.
On 03/03/2024 16:27, Jelte Fennema-Nio wrote:
On Fri, 1 Mar 2024 at 06:19, Jelte Fennema-Nio <postg...@jeltef.nl> wrote:
This is a preliminary review. I'll look at this more closely soon.
Some more thoughts after looking some more at the proposed changes
+#define EXTENDED_CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5677)
nit: I think the code should be 1234,5679 because it's the newer
message type, so to me it makes more sense if the code is a larger
number.
Unfortunately 1234,5679 already in use by NEGOTIATE_SSL_CODE. That's why
I went the other direction. If we want to add this to "the end", it
needs to be 1234,5681. But I wanted to keep the cancel requests together.
+ * FIXME: we used to use signal_child. I believe kill() is
+ * maybe even more correct, but verify that.
signal_child seems to be the correct one, not kill. signal_child has
this relevant comment explaining why it's better than plain kill:
* On systems that have setsid(), each child process sets itself up as a
* process group leader. For signals that are generally interpreted in the
* appropriate fashion, we signal the entire process group not just the
* direct child process. This allows us to, for example, SIGQUIT a blocked
* archive_recovery script, or SIGINT a script being run by a backend via
* system().
I changed it to signal the process group if HAVE_SETSID, like
pg_signal_backend() does. We don't need to signal both the process group
and the process itself like signal_child() does, because we don't have
the race condition with recently-forked children that signal_child()
talks about.
+SendCancelRequest(int backendPID, int32 cancelAuthCode)
I think this name of the function is quite confusing, it's not sending
a cancel request, it is processing one. It sends a SIGINT.
Heh, well, it's sending the cancel request signal to the right backend,
but I see your point. Renamed to ProcessCancelRequest.
While we're at it, switch to using atomics in pmsignal.c for the state
field. That feels easier to reason about than volatile
pointers.
I feel like this refactor would benefit from being a separate commit.
That would make it easier to follow which change to pmsignal.c is
necessary for what.
Point taken. I didn't do that yet, but it makes sense.
+ MyCancelKeyLength = (MyProcPort != NULL &&
MyProcPort->extended_query_cancel) ? MAX_CANCEL_KEY_LENGTH : 4;
I think we should be doing this check the opposite way, i.e. only fall
back to the smaller key when explicitly requested:
+ MyCancelKeyLength = (MyProcPort != NULL &&
MyProcPort->old_query_cancel) ? 4 : MAX_CANCEL_KEY_LENGTH;
That way we'd get the more secure, longer key length for non-backend
processes such as background workers.
+1, fixed.
On 03/03/2024 19:27, Jelte Fennema-Nio wrote:
On Sun, 3 Mar 2024 at 15:27, Jelte Fennema-Nio <postg...@jeltef.nl> wrote:
+ case EOF:
+ /* We'll come back when there is more data */
+ return PGRES_POLLING_READING;
Nice catch, I'll go steal this for my patchset which adds all the
necessary changes to be able to do a protocol bump[1].
Actually, it turns out your change to return PGRES_POLLING_READING on
EOF is incorrect (afaict). A little bit above there is this code
comment above a check to see if the whole body was received:
* Can't process if message body isn't all here yet.
*
* After this check passes, any further EOF during parsing
* implies that the server sent a bad/truncated message.
* Reading more bytes won't help in that case, so don't return
* PGRES_POLLING_READING after this point.
So I'll leave my patchset as is.
Yep, thanks.
One consequence of this patch that I didn't mention earlier is that it
makes libpq incompatible with server version 9.2 and below, because the
minor version negotiation was introduced in version 9.3. We could teach
libpq to disconnect and reconnect with minor protocol version 3.0, if
connecting with 3.1 fails, but IMHO it's better to not complicate this
and accept the break in backwards-compatibility. 9.3 was released in
2013. We dropped pg_dump support for versions older than 9.2 a few years
ago, this raises the bar for pg_dump to 9.3 as well.
--
Heikki Linnakangas
Neon (https://neon.tech)
From a1ea2c805210fa22982b1f12ba7eb33549814441 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 7 Mar 2024 21:53:03 +0200
Subject: [PATCH v2 1/2] Move cancel key generation to after forking the
backend
Move responsibility of generating the cancel key to the backend
process. The cancel key is now generated after forking, and the
backend advertises it in the PMSignalState array. When a cancel
request arrives, the backend handling it scans the PMSignalState array
to find the target pid and cancel key. This is similar to how this
previously worked in the EXEC_BACKEND case with the ShmemBackendArray,
but instead of having the separate array, we store the cancel keys in
PMSignalState array, and use it also in the !EXEC_BACKEND case.
While we're at it, switch to using atomics in pmsignal.c for the state
field. That feels easier to reason about than volatile
pointers. (TODO: also switch to pg_atomic_flag for PMSignalFlags
array. Weirdly we don't have "pg_atomic_test_clear_flag" nor
"pg_atomic_set_flag". Those would be the natural opertions for
PMSignalFlags. Maybe we should add those, but I didn't want to go that
deep down the rabbit hole in this patch.)
This is needed by the next patch, so that we can generate the cancel
key later, after negotiating the protocol version with the client.
Reviewed-by: Jelte Fennema-Nio
Discussion: https://www.postgresql.org/message-id/508d0505-8b7a-4864-a681-e7e5edfe3...@iki.fi
---
src/backend/postmaster/postmaster.c | 188 +---------------------------
src/backend/storage/ipc/ipci.c | 11 --
src/backend/storage/ipc/pmsignal.c | 124 +++++++++++++-----
src/backend/storage/lmgr/proc.c | 16 ++-
src/include/postmaster/postmaster.h | 3 -
src/include/storage/pmsignal.h | 3 +-
src/tools/pgindent/typedefs.list | 1 +
7 files changed, 116 insertions(+), 230 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f3c09e8dc0d..1eefe652e8e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -175,7 +175,6 @@
typedef struct bkend
{
pid_t pid; /* process id of backend */
- int32 cancel_key; /* cancel key for cancels for this backend */
int child_slot; /* PMChildSlot for this backend, if any */
int bkend_type; /* child process flavor, see above */
bool dead_end; /* is it going to send an error and quit? */
@@ -185,10 +184,6 @@ typedef struct bkend
static dlist_head BackendList = DLIST_STATIC_INIT(BackendList);
-#ifdef EXEC_BACKEND
-static Backend *ShmemBackendArray;
-#endif
-
BackgroundWorker *MyBgworkerEntry = NULL;
@@ -428,7 +423,6 @@ static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
static void processCancelRequest(Port *port, void *pkt);
static void report_fork_failure_to_client(Port *port, int errnum);
static CAC_state canAcceptConnections(int backend_type);
-static bool RandomCancelKey(int32 *cancel_key);
static void signal_child(pid_t pid, int signal);
static void sigquit_child(pid_t pid);
static bool SignalSomeChildren(int signal, int target);
@@ -505,7 +499,6 @@ typedef struct
BackgroundWorker bgworker;
char DataDir[MAXPGPATH];
- int32 MyCancelKey;
int MyPMChildSlot;
#ifndef WIN32
unsigned long UsedShmemSegID;
@@ -515,7 +508,6 @@ typedef struct
#endif
void *UsedShmemSegAddr;
slock_t *ShmemLock;
- Backend *ShmemBackendArray;
#ifndef HAVE_SPINLOCKS
PGSemaphore *SpinlockSemaArray;
#endif
@@ -557,9 +549,6 @@ static bool save_backend_variables(BackendParameters *param, Port *port, Backgro
static bool save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *worker,
HANDLE childProcess, pid_t childPid);
#endif
-
-static void ShmemBackendArrayAdd(Backend *bn);
-static void ShmemBackendArrayRemove(Backend *bn);
#endif /* EXEC_BACKEND */
/* Macros to check exit status of a child process */
@@ -2332,58 +2321,11 @@ processCancelRequest(Port *port, void *pkt)
CancelRequestPacket *canc = (CancelRequestPacket *) pkt;
int backendPID;
int32 cancelAuthCode;
- Backend *bp;
-
-#ifndef EXEC_BACKEND
- dlist_iter iter;
-#else
- int i;
-#endif
backendPID = (int) pg_ntoh32(canc->backendPID);
cancelAuthCode = (int32) pg_ntoh32(canc->cancelAuthCode);
- /*
- * See if we have a matching backend. In the EXEC_BACKEND case, we can no
- * longer access the postmaster's own backend list, and must rely on the
- * duplicate array in shared memory.
- */
-#ifndef EXEC_BACKEND
- dlist_foreach(iter, &BackendList)
- {
- bp = dlist_container(Backend, elem, iter.cur);
-#else
- for (i = MaxLivePostmasterChildren() - 1; i >= 0; i--)
- {
- bp = (Backend *) &ShmemBackendArray[i];
-#endif
- if (bp->pid == backendPID)
- {
- if (bp->cancel_key == cancelAuthCode)
- {
- /* Found a match; signal that backend to cancel current op */
- ereport(DEBUG2,
- (errmsg_internal("processing cancel request: sending SIGINT to process %d",
- backendPID)));
- signal_child(bp->pid, SIGINT);
- }
- else
- /* Right PID, wrong key: no way, Jose */
- ereport(LOG,
- (errmsg("wrong key in cancel request for process %d",
- backendPID)));
- return;
- }
-#ifndef EXEC_BACKEND /* make GNU Emacs 26.1 see brace balance */
- }
-#else
- }
-#endif
-
- /* No matching backend */
- ereport(LOG,
- (errmsg("PID %d in cancel request did not match any process",
- backendPID)));
+ SendCancelRequest(backendPID, cancelAuthCode);
}
/*
@@ -3294,9 +3236,6 @@ CleanupBackgroundWorker(int pid,
/* Get it out of the BackendList and clear out remaining data */
dlist_delete(&rw->rw_backend->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(rw->rw_backend);
-#endif
/*
* It's possible that this background worker started some OTHER
@@ -3382,9 +3321,6 @@ CleanupBackend(int pid,
HandleChildCrash(pid, exitstatus, _("server process"));
return;
}
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(bp);
-#endif
}
if (bp->bgworker_notify)
{
@@ -3452,9 +3388,6 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
*/
(void) ReleasePostmasterChildSlot(rw->rw_child_slot);
dlist_delete(&rw->rw_backend->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(rw->rw_backend);
-#endif
pfree(rw->rw_backend);
rw->rw_backend = NULL;
rw->rw_pid = 0;
@@ -3487,9 +3420,6 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
if (!bp->dead_end)
{
(void) ReleasePostmasterChildSlot(bp->child_slot);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(bp);
-#endif
}
dlist_delete(iter.cur);
pfree(bp);
@@ -4101,22 +4031,6 @@ BackendStartup(Port *port)
return STATUS_ERROR;
}
- /*
- * Compute the cancel key that will be assigned to this backend. The
- * backend will have its own copy in the forked-off process' value of
- * MyCancelKey, so that it can transmit the key to the frontend.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- pfree(bn);
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return STATUS_ERROR;
- }
-
- bn->cancel_key = MyCancelKey;
-
/* Pass down canAcceptConnections state */
port->canAcceptConnections = canAcceptConnections(BACKEND_TYPE_NORMAL);
bn->dead_end = (port->canAcceptConnections != CAC_OK);
@@ -4180,11 +4094,6 @@ BackendStartup(Port *port)
bn->bkend_type = BACKEND_TYPE_NORMAL; /* Can change later to WALSND */
dlist_push_head(&BackendList, &bn->elem);
-#ifdef EXEC_BACKEND
- if (!bn->dead_end)
- ShmemBackendArrayAdd(bn);
-#endif
-
return STATUS_OK;
}
@@ -5247,16 +5156,6 @@ StartupPacketTimeoutHandler(void)
_exit(1);
}
-
-/*
- * Generate a random cancel key.
- */
-static bool
-RandomCancelKey(int32 *cancel_key)
-{
- return pg_strong_random(cancel_key, sizeof(int32));
-}
-
/*
* Count up number of child processes of specified types (dead_end children
* are always excluded).
@@ -5432,25 +5331,9 @@ StartAutovacuumWorker(void)
*/
if (canAcceptConnections(BACKEND_TYPE_AUTOVAC) == CAC_OK)
{
- /*
- * Compute the cancel key that will be assigned to this session. We
- * probably don't need cancel keys for autovac workers, but we'd
- * better have something random in the field to prevent unfriendly
- * people from sending cancels to them.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return;
- }
-
bn = (Backend *) palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (bn)
{
- bn->cancel_key = MyCancelKey;
-
/* Autovac workers are not dead_end and need a child slot */
bn->dead_end = false;
bn->child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
@@ -5461,9 +5344,6 @@ StartAutovacuumWorker(void)
{
bn->bkend_type = BACKEND_TYPE_AUTOVAC;
dlist_push_head(&BackendList, &bn->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayAdd(bn);
-#endif
/* all OK */
return;
}
@@ -5595,11 +5475,10 @@ CreateOptsFile(int argc, char *argv[], char *fullprogname)
/*
* MaxLivePostmasterChildren
*
- * This reports the number of entries needed in per-child-process arrays
- * (the PMChildFlags array, and if EXEC_BACKEND the ShmemBackendArray).
- * These arrays include regular backends, autovac workers, walsenders
+ * This reports the number of entries needed in the per-child-process array.
+ * The array includes regular backends, autovac workers, walsenders
* and background workers, but not special children nor dead_end children.
- * This allows the arrays to have a fixed maximum size, to wit the same
+ * This allows the array to have a fixed maximum size, to wit the same
* too-many-children limit enforced by canAcceptConnections(). The exact value
* isn't too critical as long as it's more than MaxBackends.
*/
@@ -5799,9 +5678,6 @@ do_start_bgworker(RegisteredBgWorker *rw)
ReportBackgroundWorkerPID(rw);
/* add new worker to lists of backends */
dlist_push_head(&BackendList, &rw->rw_backend->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayAdd(rw->rw_backend);
-#endif
return true;
}
@@ -5872,20 +5748,6 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
return false;
}
- /*
- * Compute the cancel key that will be assigned to this session. We
- * probably don't need cancel keys for background workers, but we'd better
- * have something random in the field to prevent unfriendly people from
- * sending cancels to them.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return false;
- }
-
bn = palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (bn == NULL)
{
@@ -5895,7 +5757,6 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
return false;
}
- bn->cancel_key = MyCancelKey;
bn->child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
bn->bkend_type = BACKEND_TYPE_BGWORKER;
bn->dead_end = false;
@@ -6114,7 +5975,6 @@ save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *w
strlcpy(param->DataDir, DataDir, MAXPGPATH);
- param->MyCancelKey = MyCancelKey;
param->MyPMChildSlot = MyPMChildSlot;
#ifdef WIN32
@@ -6124,7 +5984,6 @@ save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *w
param->UsedShmemSegAddr = UsedShmemSegAddr;
param->ShmemLock = ShmemLock;
- param->ShmemBackendArray = ShmemBackendArray;
#ifndef HAVE_SPINLOCKS
param->SpinlockSemaArray = SpinlockSemaArray;
@@ -6359,7 +6218,6 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
SetDataDir(param->DataDir);
- MyCancelKey = param->MyCancelKey;
MyPMChildSlot = param->MyPMChildSlot;
#ifdef WIN32
@@ -6369,7 +6227,6 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
UsedShmemSegAddr = param->UsedShmemSegAddr;
ShmemLock = param->ShmemLock;
- ShmemBackendArray = param->ShmemBackendArray;
#ifndef HAVE_SPINLOCKS
SpinlockSemaArray = param->SpinlockSemaArray;
@@ -6421,43 +6278,6 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
ReserveExternalFD();
#endif
}
-
-
-Size
-ShmemBackendArraySize(void)
-{
- return mul_size(MaxLivePostmasterChildren(), sizeof(Backend));
-}
-
-void
-ShmemBackendArrayAllocation(void)
-{
- Size size = ShmemBackendArraySize();
-
- ShmemBackendArray = (Backend *) ShmemAlloc(size);
- /* Mark all slots as empty */
- memset(ShmemBackendArray, 0, size);
-}
-
-static void
-ShmemBackendArrayAdd(Backend *bn)
-{
- /* The array slot corresponding to my PMChildSlot should be free */
- int i = bn->child_slot - 1;
-
- Assert(ShmemBackendArray[i].pid == 0);
- ShmemBackendArray[i] = *bn;
-}
-
-static void
-ShmemBackendArrayRemove(Backend *bn)
-{
- int i = bn->child_slot - 1;
-
- Assert(ShmemBackendArray[i].pid == bn->pid);
- /* Mark the slot as empty */
- ShmemBackendArray[i].pid = 0;
-}
#endif /* EXEC_BACKEND */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 521ed5418cc..7cf7fe0549a 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -152,9 +152,6 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, WaitEventExtensionShmemSize());
size = add_size(size, InjectionPointShmemSize());
size = add_size(size, SlotSyncShmemSize());
-#ifdef EXEC_BACKEND
- size = add_size(size, ShmemBackendArraySize());
-#endif
/* include additional requested shmem from preload libraries */
size = add_size(size, total_addin_request);
@@ -244,14 +241,6 @@ CreateSharedMemoryAndSemaphores(void)
/* Initialize subsystems */
CreateOrAttachShmemStructs();
-#ifdef EXEC_BACKEND
-
- /*
- * Alloc the win32 shared backend array
- */
- ShmemBackendArrayAllocation();
-#endif
-
/* Initialize dynamic shared memory facilities. */
dsm_postmaster_startup(shim);
diff --git a/src/backend/storage/ipc/pmsignal.c b/src/backend/storage/ipc/pmsignal.c
index 27844b46a2b..9516b869cb2 100644
--- a/src/backend/storage/ipc/pmsignal.c
+++ b/src/backend/storage/ipc/pmsignal.c
@@ -22,6 +22,7 @@
#endif
#include "miscadmin.h"
+#include "port/atomics.h"
#include "postmaster/postmaster.h"
#include "replication/walsender.h"
#include "storage/pmsignal.h"
@@ -37,9 +38,8 @@
* if the same reason is signaled more than once simultaneously, the
* postmaster will observe it only once.)
*
- * The flags are actually declared as "volatile sig_atomic_t" for maximum
- * portability. This should ensure that loads and stores of the flag
- * values are atomic, allowing us to dispense with any explicit locking.
+ * The flag fields use atomics, allowing us to dispense with any explicit
+ * locking.
*
* In addition to the per-reason flags, we store a set of per-child-process
* flags that are currently used only for detecting whether a backend has
@@ -67,20 +67,29 @@
#define PM_CHILD_ACTIVE 2
#define PM_CHILD_WALSENDER 3
+typedef struct ChildSlotData
+{
+ pg_atomic_uint32 state;
+
+ int pid;
+ int32 cancel_key;
+} ChildSlotData;
+
/* "typedef struct PMSignalData PMSignalData" appears in pmsignal.h */
struct PMSignalData
{
/* per-reason flags for signaling the postmaster */
- sig_atomic_t PMSignalFlags[NUM_PMSIGNALS];
+ volatile sig_atomic_t PMSignalFlags[NUM_PMSIGNALS];
/* global flags for signals from postmaster to children */
QuitSignalReason sigquit_reason; /* why SIGQUIT was sent */
- /* per-child-process flags */
- int num_child_flags; /* # of entries in PMChildFlags[] */
- sig_atomic_t PMChildFlags[FLEXIBLE_ARRAY_MEMBER];
+
+ /* per-child-process slots */
+ int num_child_slots; /* # of entries in child_slots[] */
+ ChildSlotData child_slots[FLEXIBLE_ARRAY_MEMBER];
};
/* PMSignalState pointer is valid in both postmaster and child processes */
-NON_EXEC_STATIC volatile PMSignalData *PMSignalState = NULL;
+NON_EXEC_STATIC PMSignalData *PMSignalState = NULL;
/*
* These static variables are valid only in the postmaster. We keep a
@@ -130,9 +139,9 @@ PMSignalShmemSize(void)
{
Size size;
- size = offsetof(PMSignalData, PMChildFlags);
+ size = offsetof(PMSignalData, child_slots);
size = add_size(size, mul_size(MaxLivePostmasterChildren(),
- sizeof(sig_atomic_t)));
+ sizeof(ChildSlotData)));
return size;
}
@@ -151,9 +160,9 @@ PMSignalShmemInit(void)
if (!found)
{
/* initialize all flags to zeroes */
- MemSet(unvolatize(PMSignalData *, PMSignalState), 0, PMSignalShmemSize());
+ MemSet(PMSignalState, 0, PMSignalShmemSize());
num_child_inuse = MaxLivePostmasterChildren();
- PMSignalState->num_child_flags = num_child_inuse;
+ PMSignalState->num_child_slots = num_child_inuse;
/*
* Also allocate postmaster's private PMChildInUse[] array. We
@@ -262,14 +271,14 @@ AssignPostmasterChildSlot(void)
if (!PMChildInUse[slot])
{
PMChildInUse[slot] = true;
- PMSignalState->PMChildFlags[slot] = PM_CHILD_ASSIGNED;
+ pg_atomic_write_u32(&PMSignalState->child_slots[slot].state, PM_CHILD_ASSIGNED);
next_child_inuse = slot;
return slot + 1;
}
}
/* Out of slots ... should never happen, else postmaster.c messed up */
- elog(FATAL, "no free slots in PMChildFlags array");
+ elog(FATAL, "no free slots in postmaster child array");
return 0; /* keep compiler quiet */
}
@@ -283,7 +292,7 @@ AssignPostmasterChildSlot(void)
bool
ReleasePostmasterChildSlot(int slot)
{
- bool result;
+ uint32 oldstate;
Assert(slot > 0 && slot <= num_child_inuse);
slot--;
@@ -293,10 +302,9 @@ ReleasePostmasterChildSlot(int slot)
* postmaster.c is such that this might get called twice when a child
* crashes. So we don't try to Assert anything about the state.
*/
- result = (PMSignalState->PMChildFlags[slot] == PM_CHILD_ASSIGNED);
- PMSignalState->PMChildFlags[slot] = PM_CHILD_UNUSED;
+ oldstate = pg_atomic_exchange_u32(&PMSignalState->child_slots[slot].state, PM_CHILD_UNUSED);
PMChildInUse[slot] = false;
- return result;
+ return oldstate == PM_CHILD_ASSIGNED;
}
/*
@@ -309,7 +317,7 @@ IsPostmasterChildWalSender(int slot)
Assert(slot > 0 && slot <= num_child_inuse);
slot--;
- if (PMSignalState->PMChildFlags[slot] == PM_CHILD_WALSENDER)
+ if (pg_atomic_read_u32(&PMSignalState->child_slots[slot].state) == PM_CHILD_WALSENDER)
return true;
else
return false;
@@ -320,14 +328,17 @@ IsPostmasterChildWalSender(int slot)
* actively using shared memory. This is called in the child process.
*/
void
-MarkPostmasterChildActive(void)
+MarkPostmasterChildActive(int pid, int32 cancelAuthCode)
{
int slot = MyPMChildSlot;
- Assert(slot > 0 && slot <= PMSignalState->num_child_flags);
+ Assert(slot > 0 && slot <= PMSignalState->num_child_slots);
slot--;
- Assert(PMSignalState->PMChildFlags[slot] == PM_CHILD_ASSIGNED);
- PMSignalState->PMChildFlags[slot] = PM_CHILD_ACTIVE;
+ Assert(pg_atomic_read_u32(&PMSignalState->child_slots[slot].state) == PM_CHILD_ASSIGNED);
+ PMSignalState->child_slots[slot].pid = pid;
+ PMSignalState->child_slots[slot].cancel_key = cancelAuthCode;
+ pg_memory_barrier();
+ pg_atomic_write_u32(&PMSignalState->child_slots[slot].state, PM_CHILD_ACTIVE);
}
/*
@@ -342,10 +353,10 @@ MarkPostmasterChildWalSender(void)
Assert(am_walsender);
- Assert(slot > 0 && slot <= PMSignalState->num_child_flags);
+ Assert(slot > 0 && slot <= PMSignalState->num_child_slots);
slot--;
- Assert(PMSignalState->PMChildFlags[slot] == PM_CHILD_ACTIVE);
- PMSignalState->PMChildFlags[slot] = PM_CHILD_WALSENDER;
+ Assert(pg_atomic_read_u32(&PMSignalState->child_slots[slot].state) == PM_CHILD_ACTIVE);
+ pg_atomic_write_u32(&PMSignalState->child_slots[slot].state, PM_CHILD_WALSENDER);
}
/*
@@ -357,11 +368,17 @@ MarkPostmasterChildInactive(void)
{
int slot = MyPMChildSlot;
- Assert(slot > 0 && slot <= PMSignalState->num_child_flags);
+ Assert(slot > 0 && slot <= PMSignalState->num_child_slots);
slot--;
- Assert(PMSignalState->PMChildFlags[slot] == PM_CHILD_ACTIVE ||
- PMSignalState->PMChildFlags[slot] == PM_CHILD_WALSENDER);
- PMSignalState->PMChildFlags[slot] = PM_CHILD_ASSIGNED;
+#ifdef USE_ASSERT_CHECKING
+ {
+ uint32 oldstate;
+
+ oldstate = pg_atomic_read_u32(&PMSignalState->child_slots[slot].state);
+ Assert(oldstate == PM_CHILD_ACTIVE || oldstate == PM_CHILD_WALSENDER);
+ }
+#endif
+ pg_atomic_write_u32(&PMSignalState->child_slots[slot].state, PM_CHILD_ASSIGNED);
}
@@ -460,3 +477,50 @@ PostmasterDeathSignalInit(void)
postmaster_possibly_dead = true;
#endif /* USE_POSTMASTER_DEATH_SIGNAL */
}
+
+void
+SendCancelRequest(int backendPID, int32 cancelAuthCode)
+{
+ /*
+ * See if we have a matching backend. In the EXEC_BACKEND case, we can no
+ * longer access the postmaster's own backend list, and must rely on the
+ * duplicate array in shared memory. XXX
+ */
+ for (int i = MaxLivePostmasterChildren() - 1; i >= 0; i--)
+ {
+ ChildSlotData *slot = &PMSignalState->child_slots[i];
+ uint32 state = pg_atomic_read_u32(&slot->state);
+
+ if (state != PM_CHILD_ACTIVE)
+ continue;
+
+ pg_read_barrier();
+ if (slot->pid == backendPID)
+ {
+ if (slot->cancel_key == cancelAuthCode)
+ {
+ /* Found a match; signal that backend to cancel current op */
+ ereport(DEBUG2,
+ (errmsg_internal("processing cancel request: sending SIGINT to process %d",
+ backendPID)));
+
+ /*
+ * FIXME: we used to use signal_child. I believe kill() is
+ * maybe even more correct, but verify that.
+ */
+ kill(backendPID, SIGINT);
+ }
+ else
+ /* Right PID, wrong key: no way, Jose */
+ ereport(LOG,
+ (errmsg("wrong key in cancel request for process %d",
+ backendPID)));
+ return;
+ }
+ }
+
+ /* No matching backend */
+ ereport(LOG,
+ (errmsg("PID %d in cancel request did not match any process",
+ backendPID)));
+}
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index f3e20038f42..a30bba1499c 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -307,6 +307,20 @@ InitProcess(void)
if (MyProc != NULL)
elog(ERROR, "you already exist");
+ /*
+ * Generate a random cancel key.
+ *
+ * We probably don't need cancel keys for non-backend processes, but we'd
+ * better have something random in the field to prevent unfriendly people
+ * from sending cancels to them.
+ */
+ if (!pg_strong_random(&MyCancelKey, sizeof(int32)))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate random cancel key")));
+ }
+
/* Decide which list should supply our PGPROC. */
if (AmAutoVacuumLauncherProcess() || AmAutoVacuumWorkerProcess())
procgloballist = &ProcGlobal->autovacFreeProcs;
@@ -370,7 +384,7 @@ InitProcess(void)
*/
if (IsUnderPostmaster && !AmAutoVacuumLauncherProcess() &&
!AmLogicalSlotSyncWorkerProcess())
- MarkPostmasterChildActive();
+ MarkPostmasterChildActive(MyProcPid, MyCancelKey);
/*
* Initialize all fields of MyProc, except for those previously
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 46f95f7c8a6..dc133afd815 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -61,9 +61,6 @@ extern bool PostmasterMarkPIDForWorkerNotify(int);
#ifdef EXEC_BACKEND
extern pid_t postmaster_forkexec(int argc, char *argv[]);
extern void SubPostmasterMain(int argc, char *argv[]) pg_attribute_noreturn();
-
-extern Size ShmemBackendArraySize(void);
-extern void ShmemBackendArrayAllocation(void);
#endif
/*
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 029b7201093..8dff47e86e4 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -69,11 +69,12 @@ extern QuitSignalReason GetQuitSignalReason(void);
extern int AssignPostmasterChildSlot(void);
extern bool ReleasePostmasterChildSlot(int slot);
extern bool IsPostmasterChildWalSender(int slot);
-extern void MarkPostmasterChildActive(void);
+extern void MarkPostmasterChildActive(int pid, int32 cancelAuthCode);
extern void MarkPostmasterChildInactive(void);
extern void MarkPostmasterChildWalSender(void);
extern bool PostmasterIsAliveInternal(void);
extern void PostmasterDeathSignalInit(void);
+extern void SendCancelRequest(int backendPID, int32 cancelAuthCode);
/*
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d3a7f75b080..e2233af884c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -374,6 +374,7 @@ CatalogId
CatalogIdMapEntry
CatalogIndexState
ChangeVarNodes_context
+ChildSlotData
ReplaceVarnoContext
CheckPoint
CheckPointStmt
--
2.39.2
From 419eadc67021fd98cc7f0a661e315eefd27f3789 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Sat, 9 Mar 2024 00:16:31 +0200
Subject: [PATCH v2 2/2] Make cancel request keys longer, bump minor protocol
version
Currently, cancel request key is 32-bit token, which isn't very much
entropy. If you want to cancel another session's query, you can
brute-force it. In most environments, an unauthorized cancellation of
a query isn't very serious, but it nevertheless would be nice to have
more protection from it.
To make it harder to guess the key, make it larger.
The new longer key length is not hardcoded in the protocol anymore,
the client is expected to deal with variable length keys, up to some
reasonable upper limit (TODO: document the maximum). This flexibility
allows e.g. a connection pooler to add more information to the cancel
key, which might be useful for finding the connection.
This bumps the protocol version from 3.0 to 3.1. The server and libpq
still both understand version 3.0, for interoperability with old
client and server versions. If an old client connects to the server,
the server falls back to generate a short 32-bit token like before.
Note that the minor protocol version negotiation was added to the
server in version 9.3. Connecting to PostgreSQL 9.2 with new libpq
will fail with "unsupported frontend protocol" error. If we wanted to
stay compatible, we could teach libpq to reconnect with protocol
version 3.0, but I don't think that's necessary.
Reviewed-by: Jelte Fennema-Nio
Discussion: https://www.postgresql.org/message-id/508d0505-8b7a-4864-a681-e7e5edfe3...@iki.fi
---
doc/src/sgml/protocol.sgml | 120 +++++++++++++++++++++++++---
src/backend/postmaster/postmaster.c | 71 +++++++++++-----
src/backend/storage/ipc/pmsignal.c | 42 +++++++---
src/backend/storage/lmgr/proc.c | 10 ++-
src/backend/tcop/postgres.c | 12 ++-
src/backend/utils/init/globals.c | 1 -
src/include/libpq/pqcomm.h | 16 +++-
src/include/miscadmin.h | 3 +-
src/include/storage/pmsignal.h | 7 +-
src/interfaces/libpq/fe-cancel.c | 59 ++++++++++----
src/interfaces/libpq/fe-connect.c | 32 ++++++--
src/interfaces/libpq/fe-protocol3.c | 66 +++++++++++++--
src/interfaces/libpq/libpq-int.h | 8 +-
13 files changed, 366 insertions(+), 81 deletions(-)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index a5cb19357f5..357a7cb2fb0 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -18,9 +18,11 @@
</para>
<para>
- This document describes version 3.0 of the protocol, implemented in
+ This document describes version 3.1 of the protocol, introduces in
+ <productname>PostgreSQL</productname> 17. The server is compatible with
+ protocol version 3.0, implemented in
<productname>PostgreSQL</productname> 7.4 and later. For descriptions
- of the earlier protocol versions, see previous releases of the
+ of earlier protocol versions, see previous releases of the
<productname>PostgreSQL</productname> documentation. A single server
can support multiple protocol versions. The initial startup-request
message tells the server which protocol version the client is attempting to
@@ -34,6 +36,17 @@
minor protocol version which it supports. The client may then choose either
to continue with the connection using the specified protocol version or
to abort the connection.
+
+ TODO: mention that the minor version negotiation was introduced PostgreSQL 9.3
+ libpq can no longer connect to PostgreSQL 9.2. Maybe a support matrix?
+ </para>
+
+ <para>
+ The difference between protocol version 3.0 and 3.1 is that the secret key
+ used query cancellation was enlarged from 4 bytes to a variable length
+ field. The BackendKeyData message was changed to accomodate that, and a new
+ CancelRequestExtended message with variable length secret was introduced to
+ replace the old CancelRequest message.
</para>
<para>
@@ -405,8 +418,14 @@
this message indicates the highest supported minor version. This
message will also be sent if the client requested unsupported protocol
options (i.e., beginning with <literal>_pq_.</literal>) in the
- startup packet. This message will be followed by an ErrorResponse or
- a message indicating the success or failure of authentication.
+ startup packet.
+ </para>
+ <para>
+ After this message, the authentication will continue using the version
+ indicated by the server. If the client does not support the older
+ version, it should immediately close the connection. If the server
+ does not send this message, it supports the client's requested
+ protocol version and all the protocol options.
</para>
</listitem>
</varlistentry>
@@ -1381,7 +1400,7 @@ SELCT 1/0;<!-- this typo is intentional -->
<para>
To issue a cancel request, the frontend opens a new connection to
- the server and sends a CancelRequest message, rather than the
+ the server and sends a cancel request message (CancelRequestExtended or CancelRequest), rather
StartupMessage message that would ordinarily be sent across a new
connection. The server will process this request and then close
the connection. For security reasons, no direct reply is made to
@@ -1389,7 +1408,7 @@ SELCT 1/0;<!-- this typo is intentional -->
</para>
<para>
- A CancelRequest message will be ignored unless it contains the
+ A cancel request message will be ignored unless it contains the
same key data (PID and secret key) passed to the frontend during
connection start-up. If the request matches the PID and secret
key for a currently executing backend, the processing of the
@@ -1531,7 +1550,7 @@ SELCT 1/0;<!-- this typo is intentional -->
<para>
An initial SSLRequest can also be used in a connection that is being
- opened to send a CancelRequest message.
+ opened to send a CancelRequest or CancelRequestExtended message.
</para>
<para>
@@ -1607,7 +1626,7 @@ SELCT 1/0;<!-- this typo is intentional -->
<para>
An initial GSSENCRequest can also be used in a connection that is being
- opened to send a CancelRequest message.
+ opened to send a CancelRequest or CancelRequestExtended message.
</para>
<para>
@@ -3852,13 +3871,13 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
<para>
Identifies the message as cancellation key data.
The frontend must save these values if it wishes to be
- able to issue CancelRequest messages later.
+ able to issue cancel request messages later.
</para>
</listitem>
</varlistentry>
<varlistentry>
- <term>Int32(12)</term>
+ <term>Int32</term>
<listitem>
<para>
Length of message contents in bytes, including self.
@@ -3876,7 +3895,21 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
</varlistentry>
<varlistentry>
- <term>Int32</term>
+ <term>Int16</term>
+ <listitem>
+ <para>
+ Length of the secret key.
+ </para>
+ <para>
+ This field was added in minor protocol version 3.1. In protocol
+ version 3.0, it is omitted, and the secret key is always 4 bytes
+ long.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>Byten</term>
<listitem>
<para>
The secret key of this backend.
@@ -4088,8 +4121,69 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>Byte4</term>
+ <listitem>
+ <para>
+ The secret key for the target backend.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ <para>
+ This is for the older protocol version 3.0. Starting with protocol
+ version 3.1, the secret key is longer and the new CancelRequestExtended
+ message must be used instead.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="protocol-message-formats-CancelRequestExtended">
+ <term>CancelRequestExtended (F)</term>
+ <listitem>
+ <variablelist>
+ <varlistentry>
+ <term>Int32(16)</term>
+ <listitem>
+ <para>
+ Length of message contents in bytes, including self.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>Int32(80877101)</term>
+ <listitem>
+ <para>
+ The cancel request code. The value is chosen to contain
+ <literal>1234</literal> in the most significant 16 bits,
+ and <literal>5677</literal> in the least significant 16 bits. (To
+ avoid confusion, this code must not be the same as any protocol
+ version number.)
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term>Int32</term>
+ <listitem>
+ <para>
+ The process ID of the target backend.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>Int16</term>
+ <listitem>
+ <para>
+ Length of the secret key.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>Byten</term>
<listitem>
<para>
The secret key for the target backend.
@@ -4097,6 +4191,10 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
</listitem>
</varlistentry>
</variablelist>
+ <para>
+ This message was added in protocol version 3.1. In protocol version 3.0,
+ use the CancelRequest message instead.
+ </para>
</listitem>
</varlistentry>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 1eefe652e8e..97f4372130e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -420,7 +420,8 @@ static int ServerLoop(void);
static int BackendStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done);
static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
-static void processCancelRequest(Port *port, void *pkt);
+static void processExtendedCancelRequestPacket(Port *port, void *pkt, int pktlen);
+static void processCancelRequestPacket(Port *port, void *pkt, int pktlen);
static void report_fork_failure_to_client(Port *port, int errnum);
static CAC_state canAcceptConnections(int backend_type);
static void signal_child(pid_t pid, int signal);
@@ -2005,16 +2006,15 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
*/
port->proto = proto = pg_ntoh32(*((ProtocolVersion *) buf));
+ if (proto == EXTENDED_CANCEL_REQUEST_CODE)
+ {
+ processExtendedCancelRequestPacket(port, buf, len);
+ /* Not really an error, but we don't want to proceed further */
+ return STATUS_ERROR;
+ }
if (proto == CANCEL_REQUEST_CODE)
{
- if (len != sizeof(CancelRequestPacket))
- {
- ereport(COMMERROR,
- (errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg("invalid length of startup packet")));
- return STATUS_ERROR;
- }
- processCancelRequest(port, buf);
+ processCancelRequestPacket(port, buf, len);
/* Not really an error, but we don't want to proceed further */
return STATUS_ERROR;
}
@@ -2311,21 +2311,54 @@ SendNegotiateProtocolVersion(List *unrecognized_protocol_options)
}
/*
- * The client has sent a cancel request packet, not a normal
- * start-a-new-connection packet. Perform the necessary processing.
- * Nothing is sent back to the client.
+ * The client has sent an ExtendedCancelRequest cancel request packet, not a normal
+ * start-a-new-connection packet. Perform the necessary processing. Nothing
+ * is sent back to the client.
*/
static void
-processCancelRequest(Port *port, void *pkt)
+processExtendedCancelRequestPacket(Port *port, void *pkt, int pktlen)
{
- CancelRequestPacket *canc = (CancelRequestPacket *) pkt;
- int backendPID;
- int32 cancelAuthCode;
+ ExtendedCancelRequestPacket *canc;
+ int len;
- backendPID = (int) pg_ntoh32(canc->backendPID);
- cancelAuthCode = (int32) pg_ntoh32(canc->cancelAuthCode);
+ if (pktlen < offsetof(ExtendedCancelRequestPacket, cancelAuthCode))
+ {
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("invalid length of extended query cancel packet")));
+ return;
+ }
+ canc = (ExtendedCancelRequestPacket *) pkt;
+ len = pg_ntoh16(canc->cancelAuthCodeLen);
+ if (pktlen != offsetof(ExtendedCancelRequestPacket, cancelAuthCode) + len)
+ {
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("invalid length of extended cancel request packet")));
+ return;
+ }
+
+ ProcessCancelRequest(pg_ntoh32(canc->backendPID), canc->cancelAuthCode, len);
+}
- SendCancelRequest(backendPID, cancelAuthCode);
+/*
+ * like processExtendedCancelRequestPacket, but for the old protocol version
+ * 3.0 CancelRequest message
+ */
+static void
+processCancelRequestPacket(Port *port, void *pkt, int pktlen)
+{
+ CancelRequestPacket *canc;
+
+ if (pktlen != sizeof(CancelRequestPacket))
+ {
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("invalid length of query cancel packet")));
+ return;
+ }
+ canc = (CancelRequestPacket *) pkt;
+ ProcessCancelRequest(pg_ntoh32(canc->backendPID), canc->cancelAuthCode, 4);
}
/*
diff --git a/src/backend/storage/ipc/pmsignal.c b/src/backend/storage/ipc/pmsignal.c
index 9516b869cb2..cac4de5be06 100644
--- a/src/backend/storage/ipc/pmsignal.c
+++ b/src/backend/storage/ipc/pmsignal.c
@@ -72,7 +72,8 @@ typedef struct ChildSlotData
pg_atomic_uint32 state;
int pid;
- int32 cancel_key;
+ int cancel_key_len;
+ char cancel_key[MAX_CANCEL_KEY_LENGTH];
} ChildSlotData;
/* "typedef struct PMSignalData PMSignalData" appears in pmsignal.h */
@@ -328,16 +329,18 @@ IsPostmasterChildWalSender(int slot)
* actively using shared memory. This is called in the child process.
*/
void
-MarkPostmasterChildActive(int pid, int32 cancelAuthCode)
+MarkPostmasterChildActive(int pid, char *cancelKey, int len)
{
int slot = MyPMChildSlot;
+ Assert(len <= MAX_CANCEL_KEY_LENGTH);
Assert(slot > 0 && slot <= PMSignalState->num_child_slots);
slot--;
Assert(pg_atomic_read_u32(&PMSignalState->child_slots[slot].state) == PM_CHILD_ASSIGNED);
PMSignalState->child_slots[slot].pid = pid;
- PMSignalState->child_slots[slot].cancel_key = cancelAuthCode;
- pg_memory_barrier();
+ memcpy(PMSignalState->child_slots[slot].cancel_key, cancelKey, len);
+ PMSignalState->child_slots[slot].cancel_key_len = len;
+ pg_write_barrier();
pg_atomic_write_u32(&PMSignalState->child_slots[slot].state, PM_CHILD_ACTIVE);
}
@@ -478,8 +481,22 @@ PostmasterDeathSignalInit(void)
#endif /* USE_POSTMASTER_DEATH_SIGNAL */
}
+static int
+pg_const_time_memcmp(const void *a, const void *b, size_t len)
+{
+ /*
+ * FIXME: need a constant time implementation. Implement one somewhere in
+ * src/port.
+ */
+ return memcmp(a, b, len);
+}
+
+/*
+ * Find the backend with given PID, and send SIGINT to it if the cancel key
+ * matches.
+ */
void
-SendCancelRequest(int backendPID, int32 cancelAuthCode)
+ProcessCancelRequest(int backendPID, char *cancelKey, int len)
{
/*
* See if we have a matching backend. In the EXEC_BACKEND case, we can no
@@ -497,18 +514,23 @@ SendCancelRequest(int backendPID, int32 cancelAuthCode)
pg_read_barrier();
if (slot->pid == backendPID)
{
- if (slot->cancel_key == cancelAuthCode)
+ /*
+ * Use pg_const_time_memcmp() to prevent an attacker from using
+ * timing to reveal the cancel key.
+ */
+ if (len == slot->cancel_key_len && pg_const_time_memcmp(slot->cancel_key, cancelKey, len) == 0)
{
/* Found a match; signal that backend to cancel current op */
ereport(DEBUG2,
(errmsg_internal("processing cancel request: sending SIGINT to process %d",
backendPID)));
- /*
- * FIXME: we used to use signal_child. I believe kill() is
- * maybe even more correct, but verify that.
- */
+ /* If we have setsid(), signal the backend's whole process group */
+#ifdef HAVE_SETSID
+ kill(-backendPID, SIGINT);
+#else
kill(backendPID, SIGINT);
+#endif
}
else
/* Right PID, wrong key: no way, Jose */
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index a30bba1499c..5ce86417216 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -36,6 +36,7 @@
#include "access/transam.h"
#include "access/twophase.h"
#include "access/xlogutils.h"
+#include "libpq/libpq-be.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/autovacuum.h"
@@ -65,6 +66,9 @@ bool log_lock_waits = false;
/* Pointer to this process's PGPROC struct, if any */
PGPROC *MyProc = NULL;
+char MyCancelKey[MAX_CANCEL_KEY_LENGTH];
+int MyCancelKeyLength;
+
/*
* This spinlock protects the freelist of recycled PGPROC structures.
* We cannot use an LWLock because the LWLock manager depends on already
@@ -314,7 +318,9 @@ InitProcess(void)
* better have something random in the field to prevent unfriendly people
* from sending cancels to them.
*/
- if (!pg_strong_random(&MyCancelKey, sizeof(int32)))
+ MyCancelKeyLength = (MyProcPort == NULL || MyProcPort->proto >= PG_PROTOCOL(3,1))
+ ? MAX_CANCEL_KEY_LENGTH : 4;
+ if (!pg_strong_random(&MyCancelKey, MyCancelKeyLength))
{
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
@@ -384,7 +390,7 @@ InitProcess(void)
*/
if (IsUnderPostmaster && !AmAutoVacuumLauncherProcess() &&
!AmLogicalSlotSyncWorkerProcess())
- MarkPostmasterChildActive(MyProcPid, MyCancelKey);
+ MarkPostmasterChildActive(MyProcPid, MyCancelKey, MyCancelKeyLength);
/*
* Initialize all fields of MyProc, except for those previously
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index aec1b194424..9c7b63ef4f8 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4274,7 +4274,17 @@ PostgresMain(const char *dbname, const char *username)
pq_beginmessage(&buf, PqMsg_BackendKeyData);
pq_sendint32(&buf, (int32) MyProcPid);
- pq_sendint32(&buf, (int32) MyCancelKey);
+
+ if (MyProcPort->proto >= PG_PROTOCOL(3, 1))
+ {
+ pq_sendint16(&buf, MyCancelKeyLength);
+ pq_sendbytes(&buf, MyCancelKey, MyCancelKeyLength);
+ }
+ else
+ {
+ Assert(MyCancelKeyLength == 4);
+ pq_sendbytes(&buf, MyCancelKey, 4);
+ }
pq_endmessage(&buf);
/* Need not flush since ReadyForQuery will do it. */
}
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 5b536ac50d1..fe702af4cfb 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -46,7 +46,6 @@ int MyProcPid;
pg_time_t MyStartTime;
TimestampTz MyStartTimestamp;
struct Port *MyProcPort;
-int32 MyCancelKey;
int MyPMChildSlot;
/*
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 9ae469c86c4..d1cf43f12d0 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -94,7 +94,7 @@ is_unixsock_path(const char *path)
*/
#define PG_PROTOCOL_EARLIEST PG_PROTOCOL(3,0)
-#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,1)
typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
@@ -129,6 +129,18 @@ typedef uint32 AuthRequest;
* The cancel request code must not match any protocol version number
* we're ever likely to use. This random choice should do.
*/
+#define EXTENDED_CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5677)
+
+typedef struct ExtendedCancelRequestPacket
+{
+ /* Note that each field is stored in network byte order! */
+ MsgType cancelRequestCode; /* code to identify a cancel request */
+ uint32 backendPID; /* PID of client's backend */
+ uint16 cancelAuthCodeLen; /* length of cancelAuthCode */
+ char cancelAuthCode[FLEXIBLE_ARRAY_MEMBER]; /* secret key to
+ * authorize cancel */
+} ExtendedCancelRequestPacket;
+
#define CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5678)
typedef struct CancelRequestPacket
@@ -136,7 +148,7 @@ typedef struct CancelRequestPacket
/* Note that each field is stored in network byte order! */
MsgType cancelRequestCode; /* code to identify a cancel request */
uint32 backendPID; /* PID of client's backend */
- uint32 cancelAuthCode; /* secret key to authorize cancel */
+ char cancelAuthCode[4]; /* secret key to authorize cancel */
} CancelRequestPacket;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index f900da61573..ae4abc0a43b 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -191,7 +191,8 @@ extern PGDLLIMPORT pg_time_t MyStartTime;
extern PGDLLIMPORT TimestampTz MyStartTimestamp;
extern PGDLLIMPORT struct Port *MyProcPort;
extern PGDLLIMPORT struct Latch *MyLatch;
-extern PGDLLIMPORT int32 MyCancelKey;
+extern PGDLLIMPORT char MyCancelKey[];
+extern PGDLLIMPORT int MyCancelKeyLength;
extern PGDLLIMPORT int MyPMChildSlot;
extern PGDLLIMPORT char OutputFileName[];
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 8dff47e86e4..2573c051b0b 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -57,6 +57,9 @@ typedef enum
/* PMSignalData is an opaque struct, details known only within pmsignal.c */
typedef struct PMSignalData PMSignalData;
+/* Length of query cancel keys generated. */
+#define MAX_CANCEL_KEY_LENGTH 32
+
/*
* prototypes for functions in pmsignal.c
*/
@@ -69,12 +72,12 @@ extern QuitSignalReason GetQuitSignalReason(void);
extern int AssignPostmasterChildSlot(void);
extern bool ReleasePostmasterChildSlot(int slot);
extern bool IsPostmasterChildWalSender(int slot);
-extern void MarkPostmasterChildActive(int pid, int32 cancelAuthCode);
+extern void MarkPostmasterChildActive(int pid, char *cancelKey, int len);
extern void MarkPostmasterChildInactive(void);
extern void MarkPostmasterChildWalSender(void);
extern bool PostmasterIsAliveInternal(void);
extern void PostmasterDeathSignalInit(void);
-extern void SendCancelRequest(int backendPID, int32 cancelAuthCode);
+extern void ProcessCancelRequest(int backendPID, char *cancelKey, int len);
/*
diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index 51f8d8a78c4..608038cf2be 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -34,6 +34,8 @@ PGcancel *
PQgetCancel(PGconn *conn)
{
PGcancel *cancel;
+ int cancel_req_len;
+ bool use_extended;
if (!conn)
return NULL;
@@ -41,13 +43,20 @@ PQgetCancel(PGconn *conn)
if (conn->sock == PGINVALID_SOCKET)
return NULL;
- cancel = malloc(sizeof(PGcancel));
+ use_extended = conn->pversion >= PG_PROTOCOL(3, 1);
+ if (use_extended)
+ cancel_req_len = offsetof(ExtendedCancelRequestPacket, cancelAuthCode) + conn->be_cancel_key_len;
+ else
+ {
+ Assert(conn->be_cancel_key_len == 4);
+ cancel_req_len = sizeof(CancelRequestPacket);
+ }
+ cancel = malloc(offsetof(PGcancel, cancel_req) + cancel_req_len);
if (cancel == NULL)
return NULL;
memcpy(&cancel->raddr, &conn->raddr, sizeof(SockAddr));
- cancel->be_pid = conn->be_pid;
- cancel->be_key = conn->be_key;
+
/* We use -1 to indicate an unset connection option */
cancel->pgtcp_user_timeout = -1;
cancel->keepalives = -1;
@@ -90,6 +99,28 @@ PQgetCancel(PGconn *conn)
goto fail;
}
+ if (use_extended)
+ {
+ ExtendedCancelRequestPacket *req;
+
+ req = (ExtendedCancelRequestPacket *) &cancel->cancel_req;
+ req->cancelRequestCode = (MsgType) pg_hton32(EXTENDED_CANCEL_REQUEST_CODE);
+ req->backendPID = pg_hton32(conn->be_pid);
+ req->cancelAuthCodeLen = pg_hton16(conn->be_cancel_key_len);
+ memcpy(req->cancelAuthCode, conn->be_cancel_key, conn->be_cancel_key_len);
+ }
+ else
+ {
+ CancelRequestPacket *req;
+
+ req = (CancelRequestPacket *) &cancel->cancel_req;
+ req->cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ req->backendPID = pg_hton32(conn->be_pid);
+ memcpy(req->cancelAuthCode, conn->be_cancel_key, 4);
+ }
+ /* include the length field itself in the length */
+ cancel->cancel_pkt_len = pg_hton32(cancel_req_len + 4);
+
return cancel;
fail:
@@ -150,11 +181,8 @@ PQcancel(PGcancel *cancel, char *errbuf, int errbufsize)
int save_errno = SOCK_ERRNO;
pgsocket tmpsock = PGINVALID_SOCKET;
int maxlen;
- struct
- {
- uint32 packetlen;
- CancelRequestPacket cp;
- } crp;
+ char recvbuf;
+ int cancel_pkt_len;
if (!cancel)
{
@@ -256,15 +284,14 @@ retry3:
goto cancel_errReturn;
}
- /* Create and send the cancel request packet. */
-
- crp.packetlen = pg_hton32((uint32) sizeof(crp));
- crp.cp.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
- crp.cp.backendPID = pg_hton32(cancel->be_pid);
- crp.cp.cancelAuthCode = pg_hton32(cancel->be_key);
+ cancel_pkt_len = pg_ntoh32(cancel->cancel_pkt_len);
retry4:
- if (send(tmpsock, (char *) &crp, sizeof(crp), 0) != (int) sizeof(crp))
+ /*
+ * Send the cancel request packet. It starts with the message length at
+ * cancel_pkt_len, followed by the actual packet.
+ */
+ if (send(tmpsock, (char *) &cancel->cancel_pkt_len, cancel_pkt_len, 0) != cancel_pkt_len)
{
if (SOCK_ERRNO == EINTR)
/* Interrupted system call - we'll just try again */
@@ -281,7 +308,7 @@ retry4:
* read to obtain any data, we are just waiting for EOF to be signaled.
*/
retry5:
- if (recv(tmpsock, (char *) &crp, 1, 0) < 0)
+ if (recv(tmpsock, &recvbuf, 1, 0) < 0)
{
if (SOCK_ERRNO == EINTR)
/* Interrupted system call - we'll just try again */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d4e10a0c4f3..e47b933e27e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -617,7 +617,12 @@ pqDropServerData(PGconn *conn)
free(conn->write_err_msg);
conn->write_err_msg = NULL;
conn->be_pid = 0;
- conn->be_key = 0;
+ if (conn->be_cancel_key != NULL)
+ {
+ free(conn->be_cancel_key);
+ conn->be_cancel_key = NULL;
+ }
+ conn->be_cancel_key_len = 0;
}
@@ -2724,7 +2729,7 @@ keep_going: /* We will come back to here until there is
* must persist across individual connection attempts, but we must
* reset them when we start to consider a new server.
*/
- conn->pversion = PG_PROTOCOL(3, 0);
+ conn->pversion = PG_PROTOCOL(3, 1);
conn->send_appname = true;
#ifdef USE_SSL
/* initialize these values based on SSL mode */
@@ -3724,14 +3729,25 @@ keep_going: /* We will come back to here until there is
}
else if (beresp == PqMsg_NegotiateProtocolVersion)
{
- if (pqGetNegotiateProtocolVersion3(conn))
+ switch (pqGetNegotiateProtocolVersion3(conn))
{
- libpq_append_conn_error(conn, "received invalid protocol negotiation message");
- goto error_return;
+ case 0:
+ /* OK, we read the message; mark data consumed */
+ conn->inStart = conn->inCursor;
+ /* Stay in the CONNECTION_AWAITING_RESPONSE state */
+ goto keep_going;
+ case 1:
+ /*
+ * Negotiation failed. The error message was
+ * filled in already.
+ */
+ conn->inStart = conn->inCursor;
+ goto error_return;
+ case EOF:
+ /* We'll come back when there is more data */
+ libpq_append_conn_error(conn, "received invalid protocol negotiation message");
+ goto error_return;
}
- /* OK, we read the message; mark data consumed */
- conn->inStart = conn->inCursor;
- goto error_return;
}
/* It is an authentication request. */
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 701d58e1087..877ba69c4a8 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -48,6 +48,7 @@ static int getRowDescriptions(PGconn *conn, int msgLength);
static int getParamDescriptions(PGconn *conn, int msgLength);
static int getAnotherTuple(PGconn *conn, int msgLength);
static int getParameterStatus(PGconn *conn);
+static int getBackendKeyData(PGconn *conn);
static int getNotify(PGconn *conn);
static int getCopyStart(PGconn *conn, ExecStatusType copytype);
static int getReadyForQuery(PGconn *conn);
@@ -308,9 +309,7 @@ pqParseInput3(PGconn *conn)
* just as easy to handle it as part of the main loop.
* Save the data and continue processing.
*/
- if (pqGetInt(&(conn->be_pid), 4, conn))
- return;
- if (pqGetInt(&(conn->be_key), 4, conn))
+ if (getBackendKeyData(conn))
return;
break;
case PqMsg_RowDescription:
@@ -1404,7 +1403,8 @@ reportErrorPosition(PQExpBuffer msg, const char *query, int loc, int encoding)
/*
* Attempt to read a NegotiateProtocolVersion message.
* Entry: 'v' message type and length have already been consumed.
- * Exit: returns 0 if successfully consumed message.
+ * Exit: returns 0 if successfully consumed message and the negotiation succeeded.
+ * returns 1 if successfully consumed message and the negotiation failed.
* returns EOF if not enough data.
*/
int
@@ -1435,10 +1435,13 @@ pqGetNegotiateProtocolVersion3(PGconn *conn)
appendPQExpBufferStr(&buf, conn->workBuffer.data);
}
- if (their_version < conn->pversion)
- libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u",
+ if (their_version < PG_PROTOCOL(3, 0))
+ {
+ libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u", /* XXX */
PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion),
PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version));
+ return 1;
+ }
if (num > 0)
{
appendPQExpBuffer(&conn->errorMessage,
@@ -1450,7 +1453,12 @@ pqGetNegotiateProtocolVersion3(PGconn *conn)
/* neither -- server shouldn't have sent it */
if (!(their_version < conn->pversion) && !(num > 0))
+ {
libpq_append_conn_error(conn, "invalid %s message", "NegotiateProtocolVersion");
+ return 1;
+ }
+
+ conn->pversion = their_version;
termPQExpBuffer(&buf);
return 0;
@@ -1485,6 +1493,52 @@ getParameterStatus(PGconn *conn)
return 0;
}
+/*
+ * parseInput subroutine to read a BackendKeyData message.
+ * Entry: 'v' message type and length have already been consumed.
+ * Exit: returns 0 if successfully consumed message.
+ * returns EOF if not enough data.
+ */
+static int
+getBackendKeyData(PGconn *conn)
+{
+ int cancel_key_len;
+
+ if (conn->be_cancel_key)
+ {
+ free(conn->be_cancel_key);
+ conn->be_cancel_key = NULL;
+ conn->be_cancel_key_len = 0;
+ }
+
+ if (pqGetInt(&(conn->be_pid), 4, conn))
+ return EOF;
+
+ if (conn->pversion >= PG_PROTOCOL(3, 1))
+ {
+ if (pqGetInt(&cancel_key_len, 2, conn))
+ return EOF;
+ }
+ else
+ cancel_key_len = 4;
+
+ conn->be_cancel_key = malloc(cancel_key_len);
+ if (conn->be_cancel_key == NULL)
+ {
+ libpq_append_conn_error(conn, "out of memory");
+ /* discard the message */
+ return EOF;
+ }
+ if (pqGetnchar(conn->be_cancel_key, cancel_key_len, conn))
+ {
+ free(conn->be_cancel_key);
+ conn->be_cancel_key = NULL;
+ return EOF;
+ }
+ conn->be_cancel_key_len = cancel_key_len;
+ return 0;
+}
+
/*
* Attempt to read a Notify response message.
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 82c18f870d2..3b46a760d23 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -492,7 +492,8 @@ struct pg_conn
/* Miscellaneous stuff */
int be_pid; /* PID of backend --- needed for cancels */
- int be_key; /* key of backend --- needed for cancels */
+ char *be_cancel_key;
+ uint16 be_cancel_key_len;
pgParameterStatus *pstatus; /* ParameterStatus data */
int client_encoding; /* encoding id */
bool std_strings; /* standard_conforming_strings */
@@ -629,7 +630,6 @@ struct pg_cancel
{
SockAddr raddr; /* Remote address */
int be_pid; /* PID of backend --- needed for cancels */
- int be_key; /* key of backend --- needed for cancels */
int pgtcp_user_timeout; /* tcp user timeout */
int keepalives; /* use TCP keepalives? */
int keepalives_idle; /* time between TCP keepalives */
@@ -637,6 +637,10 @@ struct pg_cancel
* retransmits */
int keepalives_count; /* maximum number of TCP keepalive
* retransmits */
+
+ /* Pre-constructed cancel request packet starts here */
+ int32 cancel_pkt_len; /* in network-byte-order */
+ char cancel_req[FLEXIBLE_ARRAY_MEMBER]; /* CancelRequestPacket */
};
--
2.39.2
