pg_regress/pg_isolation_regress: Fix possible nullptr dereference.

Started by Xing Guoabout 3 years ago2 messages
#1Xing Guo
higuoxing@gmail.com
1 attachment(s)

Hi hackers,

While playing with pg_regress and pg_isolation_regress, I noticed that
there's a potential nullptr deference in both of them.

How to reproduce:

Specify the `--dbname=` option without providing any database name.

<path>/<to>/pg_regress --dbname= foo
<path>/<to>/pg_isolation_regress --dbname= foo

Patch is attached.

--
Best Regards,
Xing

Attachments:

patch.difftext/plain; charset=US-ASCII; name=patch.diffDownload
diff --git a/src/test/isolation/isolation_main.c b/src/test/isolation/isolation_main.c
index 31a0e6b709..a88e9da255 100644
--- a/src/test/isolation/isolation_main.c
+++ b/src/test/isolation/isolation_main.c
@@ -89,7 +89,7 @@ isolation_start_test(const char *testname,
 	offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
 					   "\"%s\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1",
 					   isolation_exec,
-					   dblist->str,
+					   dblist ? dblist->str : "",
 					   infile,
 					   outfile);
 	if (offset >= sizeof(psql_cmd))
diff --git a/src/test/regress/pg_regress_main.c b/src/test/regress/pg_regress_main.c
index a4b354c9e6..4e089b0f83 100644
--- a/src/test/regress/pg_regress_main.c
+++ b/src/test/regress/pg_regress_main.c
@@ -81,7 +81,7 @@ psql_start_test(const char *testname,
 					   "\"%s%spsql\" -X -a -q -d \"%s\" %s < \"%s\" > \"%s\" 2>&1",
 					   bindir ? bindir : "",
 					   bindir ? "/" : "",
-					   dblist->str,
+					   dblist ? dblist->str : "",
 					   "-v HIDE_TABLEAM=on -v HIDE_TOAST_COMPRESSION=on",
 					   infile,
 					   outfile);
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Xing Guo (#1)
Re: pg_regress/pg_isolation_regress: Fix possible nullptr dereference.

Xing Guo <higuoxing@gmail.com> writes:

While playing with pg_regress and pg_isolation_regress, I noticed that
there's a potential nullptr deference in both of them.
How to reproduce:
Specify the `--dbname=` option without providing any database name.

Hmm, yeah, I see that too.

Patch is attached.

This patch seems like a band-aid, though. The reason nobody's
noticed this for decades is that it doesn't make a lot of sense
to allow tests to run in your default database: the odds of them
screwing up something valuable are high, and the odds that they'll
fail if started in a nonempty database are even higher.

I think the right answer is to treat it as an error if we end up
with an empty dblist (or even a zero-length name).

regards, tom lane