inplace030-inj-exit-race-v1.patch

text/plain

Filename: inplace030-inj-exit-race-v1.patch
Type: text/plain
Part: 0
Message: Re: Weird test mixup

Patch

Format: unified
Series: patch v1
File+
src/test/modules/injection_points/injection_points.c 18 6
Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@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);