On Mon, May 06, 2024 at 10:03:37AM +0900, Michael Paquier wrote: > On Thu, May 02, 2024 at 12:35:55PM -0700, Noah Misch wrote: > > I should have given a simpler example: > > > > s1: local-attach to POINT > > s2: enter InjectionPointRun(POINT), yield CPU just before > > injection_callback() > > s1: exit > > s2: wake up and run POINT as though it had been non-local
Here's how I've patched it locally. It does avoid changing the backend-side, which has some attraction. Shall I just push this? > Hmm. Even if you were to emulate that in a controlled manner, you > would need a second injection point that does a wait in s2, which is > something that would happen before injection_callback() and before > scanning the local entry. This relies on the fact that we're holding > CPU in s2 between the backend shmem hash table lookup and the callback > being called. Right. We would need "second-level injection points" to write a test for that race in the injection point system.
Author: Noah Misch <n...@leadboat.com> Commit: Noah Misch <n...@leadboat.com> Fix race condition in backend exit after injection_points_set_local(). Symptoms were like those prompting commit f587338dec87d3c35b025e131c5977930ac69077. That is, under installcheck, backends from unrelated tests could run the injection points. Reviewed by FIXME. Discussion: https://postgr.es/m/FIXME diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index ace386d..75466d3 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -37,7 +37,13 @@ PG_MODULE_MAGIC; /* * Conditions related to injection points. This tracks in shared memory the - * runtime conditions under which an injection point is allowed to run. + * runtime conditions under which an injection point is allowed to run. Once + * set, a name is never changed. That avoids a race condition: + * + * s1: local-attach to POINT + * s2: yield CPU before InjectionPointRun(POINT) calls injection_callback() + * s1: exit() + * s2: run POINT as though it had been non-local * * If more types of runtime conditions need to be tracked, this structure * should be expanded. @@ -49,6 +55,9 @@ typedef struct InjectionPointCondition /* ID of the process where the injection point is allowed to run */ int pid; + + /* Should "pid" run this injection point, or not? */ + bool valid; } InjectionPointCondition; /* Shared state information for injection points. */ @@ -133,7 +142,7 @@ injection_point_allowed(const char *name) { InjectionPointCondition *condition = &inj_state->conditions[i]; - if (strcmp(condition->name, name) == 0) + if (condition->valid && strcmp(condition->name, name) == 0) { /* * Check if this injection point is allowed to run in this @@ -168,15 +177,16 @@ injection_points_cleanup(int code, Datum arg) { InjectionPointCondition *condition = &inj_state->conditions[i]; - if (condition->name[0] == '\0') + if (!condition->valid) continue; + Assert(condition->name[0] != '\0'); if (condition->pid != MyProcPid) continue; /* Detach the injection point and unregister condition */ InjectionPointDetach(condition->name); - condition->name[0] = '\0'; + condition->valid = false; condition->pid = 0; } SpinLockRelease(&inj_state->lock); @@ -299,11 +309,13 @@ injection_points_attach(PG_FUNCTION_ARGS) { InjectionPointCondition *condition = &inj_state->conditions[i]; - if (condition->name[0] == '\0') + if (strcmp(condition->name, name) == 0 || + condition->name[0] == '\0') { index = i; strlcpy(condition->name, name, INJ_NAME_MAXLEN); condition->pid = MyProcPid; + condition->valid = true; break; } } @@ -416,8 +428,8 @@ injection_points_detach(PG_FUNCTION_ARGS) if (strcmp(condition->name, name) == 0) { + condition->valid = false; condition->pid = 0; - condition->name[0] = '\0'; } } SpinLockRelease(&inj_state->lock);