fix: a race condition in which a backend might segfault

[imor]

In the old code SetLatch was called on the shared_latch only after checking that it was not NULL. But there was a narrow race condition in which the shared_latch variable might be assigned to NULL after the if condition succeeded but before the SetLatch was called if the worker exited in that narrow window and net_on_exit function set the shared_latch to NULL. The fix is to make the shared_latch pointer volatile and then to load it once before checking it for NULL and calling SetLatch.

This PR also removes the pg_write_barrier calls because these are redundant: SetLatch and ConditionVariableBroadcast functions already have a pg_write_barrier call inside them.

[AndrewJackson2020]

Do you have a test/repro case that would trigger this race condition? I see worker_state->shared_latch = NULL; 2 times in the source code, once in net_shmem_startup and once in net_on_exit. Can this only happen as the app is starting up or shutting down?

[imor]

Do you have a test/repro case that would trigger this race condition? I see worker_state->shared_latch = NULL; 2 times in the source code, once in net_shmem_startup and once in net_on_exit. Can this only happen as the app is starting up or shutting down?

It will be hard to write a deterministic test for this precisely because it is a race condition and the window is pretty narrow (a couple of assembly instructions). But it works like this:

1. Let's say a backend (other than pg_net background worker) called worker_restart and it had executed the if condition in the following code: if (worker_state->shared_latch) SetLatch(worker_state->shared_latch); but has not yet executed the SetLatch call.
2. The pg_net background worker exits in this window (due to some other reason), the net_on_exit function sets the latch to NULL at the line worker_state->shared_latch = NULL;
3. Before calling the SetLatch function the worker_state->shared_latch is loaded from shared memory again, reading a NULL value and leading to segfault or other UB.

Yes, this can only happen while pg_net bg worker is shutting down. The fix closes that window between two loads of if condition and SetLatch by collapsing them into a single load.

[steve-chavez]

Do you have a test/repro case that would trigger this race condition?
It will be hard to write a deterministic test for this precisely because it is a race condition and the window is pretty narrow

Maybe we could inject some sleep via postgres injection points to trigger this failure and prove the fix? It would be a custom build but we could do it via some config in xpg.

[AndrewJackson2020]

Maybe we could inject some sleep via postgres injection points to trigger this failure and prove the fix? It would be a custom build but we could do it via some config in xpg.

Possibly this can be used in some way?

https://www.postgresql.org/docs/devel/xfunc-c.html#XFUNC-ADDIN-INJECTION-POINTS

[imor]

Maybe we could inject some sleep via postgres injection points to trigger this failure and prove the fix? It would be a custom build but we could do it via some config in xpg.

Possibly this can be used in some way?

https://www.postgresql.org/docs/devel/xfunc-c.html#XFUNC-ADDIN-INJECTION-POINTS

Possibly, with a few caveats. First, injection points are available in PG 17 and later only, so we won't be able to test it on older versions. Second, it needs support in xpg to add the --enable-injection-points command line arg.

I've let AI write a potential test using injection points in this commit on a separate branch (rs/segfault-repro-with-sleeps) but haven't run in locally due to missing --enable-injection-points support. What I have tested locally is this commit which reproduces the segfault but by adding sleeps without the injection points.

I was also thinking if this is a scalable testing strategy. I asked AI find more race conditions and it came up with the following. I've not verified them all, but assuming there are more race conditions would we want to add more injection points to test them all? My worry is we might litter the code with them hurting readability.

Lost wake while the extension is locked. should_wake is cleared at [src/worker.c:325](vscode-webview://1p4fpsd399b3atmn45devue8s7k4h4a6lbcrqs241fg0ij4solfm/src/worker.c#L325) before is_extension_locked() runs. If another session holds a conflicting lock on the queue tables (CREATE, ALTER or DROP EXTENSION), the worker breaks out and waits again with the flag at 0. Requests queued before that point sit there until some later commit calls net.wake(). The fix is to re-arm should_wake on that path.

worker_restart() followed by wait_until_running() can return early. worker_restart() only sets a flag and a latch. status stays WS_RUNNING until the worker actually exits, and WS_EXITED is only published on the clean path ([src/worker.c:473](vscode-webview://1p4fpsd399b3atmn45devue8s7k4h4a6lbcrqs241fg0ij4solfm/src/worker.c#L473)). A caller doing worker_restart(); wait_until_running(); (the pattern in test/common.py:348) can see the stale WS_RUNNING and return before the restart has happened. After a crash the status isn't reset at all.

Stale got_restart flag. It is never cleared at worker startup. If worker_restart() runs while the worker is down (the latch is NULL), or lands in the shutdown window after the worker's last pg_atomic_exchange, the flag stays at 1. The next worker then restarts itself again after its first wait. The result is an extra restart cycle, not data loss.

TOCTOU in is_extension_locked(). It looks up the table OIDs by name and then locks them. A concurrent DROP EXTENSION between the lookup and the lock means it locks a dead OID, which ConditionalLockRelationOid accepts. The next SPI query then errors, the worker dies, and it restarts after 1s. The standard fix is to lock first and then re-check that the relation still exists.

Stale latch pointer after the fix (benign). A backend can load a non-NULL shared_latch just before the worker exits. By the time it calls SetLatch, the worker's PGPROC may be freed or reused. That gives a spurious wakeup to an unrelated process, which latches tolerate.

Unordered pointer publish (low risk). shared_latch is a plain volatile pointer, which gives no ordering. It is only safe because pointer stores are atomic on the supported platforms, and SetLatch has its own barrier. pg_atomic_uintptr or an explicit barrier would be more robust.

[AndrewJackson2020]

Tbh for this PR all I really need is a way to repro what ur talking about locally so I can validate the issue. Ill take a look at the above.

[utkarash2991]

Maybe we could inject some sleep via postgres injection points to trigger this failure and prove the fix? It would be a custom build but we could do it via some config in xpg.

Possibly this can be used in some way?
https://www.postgresql.org/docs/devel/xfunc-c.html#XFUNC-ADDIN-INJECTION-POINTS

Possibly, with a few caveats. First, injection points are available in PG 17 and later only, so we won't be able to test it on older versions. Second, it needs support in xpg to add the --enable-injection-points command line arg.

I've let AI write a potential test using injection points in this commit on a separate branch (rs/segfault-repro-with-sleeps) but haven't run in locally due to missing --enable-injection-points support. What I have tested locally is this commit which reproduces the segfault but by adding sleeps without the injection points.

I was also thinking if this is a scalable testing strategy. I asked AI find more race conditions and it came up with the following. I've not verified them all, but assuming there are more race conditions would we want to add more injection points to test them all? My worry is we might litter the code with them hurting readability.

Lost wake while the extension is locked. should_wake is cleared at [src/worker.c:325](vscode-webview://1p4fpsd399b3atmn45devue8s7k4h4a6lbcrqs241fg0ij4solfm/src/worker.c#L325) before is_extension_locked() runs. If another session holds a conflicting lock on the queue tables (CREATE, ALTER or DROP EXTENSION), the worker breaks out and waits again with the flag at 0. Requests queued before that point sit there until some later commit calls net.wake(). The fix is to re-arm should_wake on that path.

worker_restart() followed by wait_until_running() can return early. worker_restart() only sets a flag and a latch. status stays WS_RUNNING until the worker actually exits, and WS_EXITED is only published on the clean path ([src/worker.c:473](vscode-webview://1p4fpsd399b3atmn45devue8s7k4h4a6lbcrqs241fg0ij4solfm/src/worker.c#L473)). A caller doing worker_restart(); wait_until_running(); (the pattern in test/common.py:348) can see the stale WS_RUNNING and return before the restart has happened. After a crash the status isn't reset at all.

Stale got_restart flag. It is never cleared at worker startup. If worker_restart() runs while the worker is down (the latch is NULL), or lands in the shutdown window after the worker's last pg_atomic_exchange, the flag stays at 1. The next worker then restarts itself again after its first wait. The result is an extra restart cycle, not data loss.

TOCTOU in is_extension_locked(). It looks up the table OIDs by name and then locks them. A concurrent DROP EXTENSION between the lookup and the lock means it locks a dead OID, which ConditionalLockRelationOid accepts. The next SPI query then errors, the worker dies, and it restarts after 1s. The standard fix is to lock first and then re-check that the relation still exists.

Stale latch pointer after the fix (benign). A backend can load a non-NULL shared_latch just before the worker exits. By the time it calls SetLatch, the worker's PGPROC may be freed or reused. That gives a spurious wakeup to an unrelated process, which latches tolerate.

Unordered pointer publish (low risk). shared_latch is a plain volatile pointer, which gives no ordering. It is only safe because pointer stores are atomic on the supported platforms, and SetLatch has its own barrier. pg_atomic_uintptr or an explicit barrier would be more robust.

1. IMO using injection points is good as far as they dont make the code readibility worse. Your worry is correct. We need to be a bit frugal in using injection points else we will end up with littering of injection points in the code here and there. So my recommendation would be as far as injection points are used for critical paths/scenarios which is mission cirtical then its good. If we are using it for just sake of using it or testing simple scenarios then it doesn't make any sense.

2. If we are able to repro it locally using sleep as what @steve-chavez suggested earlier then we should be ok.

Just my 2 cents.