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);

Reply via email to