Fix assorted bugs in ecpg's macro mechanism.

The code associated with EXEC SQL DEFINE was unreadable and full of
bugs, notably:

* It'd attempt to free a non-malloced string if the ecpg program
tries to redefine a macro that was defined on the command line.

* Possible memory stomp if user writes "-D=foo".

* Undef'ing or redefining a macro defined on the command line would
change the state visible to the next file, when multiple files are
specified on the command line.  (While possibly that could have been
an intentional choice, the code clearly intends to revert to the
original macro state; it's just failing to consider this interaction.)

* Missing "break" in defining a new macro meant that redefinition
of an existing name would cause an extra entry to be added to the
definition list.  While not immediately harmful, a subsequent undef
would result in the prior entry becoming visible again.

* The interactions with input buffering are subtle and were entirely
undocumented.

It's not that surprising that we hadn't noticed these bugs,
because there was no test coverage at all of either the -D
command line switch or multiple input files.  This patch adds
such coverage (in a rather hacky way I guess).

In addition to the code bugs, the user documentation was confused
about whether the -D switch defines a C macro or an ecpg one, and
it failed to mention that you can write "-Dsymbol=value".

These problems are old, so back-patch to all supported branches.

Discussion: https://postgr.es/m/998011.1713217712@sss.pgh.pa.us

1 files changed, 46 insertions, 33 deletions

diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index 93e66fc60f0..73c37631acc 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -82,35 +82,46 @@ add_include_path(char *path)
 	}
 }
 
+/*
+ * Process a command line -D switch
+ */
 static void
 add_preprocessor_define(char *define)
 {
-	struct _defines *pd = defines;
-	char	   *ptr,
-			   *define_copy = mm_strdup(define);
+	/* copy the argument to avoid relying on argv storage */
+	char	   *define_copy = mm_strdup(define);
+	char	   *ptr;
+	struct _defines *newdef;
 
-	defines = mm_alloc(sizeof(struct _defines));
+	newdef = mm_alloc(sizeof(struct _defines));
 
 	/* look for = sign */
 	ptr = strchr(define_copy, '=');
 	if (ptr != NULL)
 	{
+		/* symbol has a value */
 		char	   *tmp;
 
-		/* symbol has a value */
-		for (tmp = ptr - 1; *tmp == ' '; tmp--);
+		/* strip any spaces between name and '=' */
+		for (tmp = ptr - 1; tmp >= define_copy && *tmp == ' '; tmp--);
 		tmp[1] = '\0';
-		defines->olddef = define_copy;
-		defines->newdef = ptr + 1;
+
+		/*
+		 * Note we don't bother to separately malloc cmdvalue; it will never
+		 * be freed so that's not necessary.
+		 */
+		newdef->cmdvalue = ptr + 1;
 	}
 	else
 	{
-		defines->olddef = define_copy;
-		defines->newdef = mm_strdup("1");
+		/* define it as "1"; again no need to malloc it */
+		newdef->cmdvalue = "1";
 	}
-	defines->pertinent = true;
-	defines->used = NULL;
-	defines->next = pd;
+	newdef->name = define_copy;
+	newdef->value = mm_strdup(newdef->cmdvalue);
+	newdef->used = NULL;
+	newdef->next = defines;
+	defines = newdef;
 }
 
 #define ECPG_GETOPT_LONG_REGRESSION		1
@@ -348,6 +359,8 @@ main(int argc, char *const argv[])
 			{
 				struct cursor *ptr;
 				struct _defines *defptr;
+				struct _defines *prevdefptr;
+				struct _defines *nextdefptr;
 				struct typedefs *typeptr;
 				struct declared_list *list;
 
@@ -385,28 +398,28 @@ main(int argc, char *const argv[])
 					free(this);
 				}
 
-				/* remove non-pertinent old defines as well */
-				while (defines && !defines->pertinent)
+				/* restore defines to their command-line state */
+				prevdefptr = NULL;
+				for (defptr = defines; defptr != NULL; defptr = nextdefptr)
 				{
-					defptr = defines;
-					defines = defines->next;
-
-					free(defptr->newdef);
-					free(defptr->olddef);
-					free(defptr);
-				}
-
-				for (defptr = defines; defptr != NULL; defptr = defptr->next)
-				{
-					struct _defines *this = defptr->next;
-
-					if (this && !this->pertinent)
+					nextdefptr = defptr->next;
+					if (defptr->cmdvalue != NULL)
 					{
-						defptr->next = this->next;
-
-						free(this->newdef);
-						free(this->olddef);
-						free(this);
+						/* keep it, resetting the value */
+						free(defptr->value);
+						defptr->value = mm_strdup(defptr->cmdvalue);
+						prevdefptr = defptr;
+					}
+					else
+					{
+						/* remove it */
+						if (prevdefptr != NULL)
+							prevdefptr->next = nextdefptr;
+						else
+							defines = nextdefptr;
+						free(defptr->name);
+						free(defptr->value);
+						free(defptr);
 					}
 				}