From 95567f599fb64e7fc72e243286d917ca3bd9e5d0 Mon Sep 17 00:00:00 2001 From: Georgios Kokolatos Date: Mon, 31 Aug 2020 08:51:36 +0000 Subject: [PATCH v4] Use a stringInfo instead of a char for replace_string in pg_regress The exposed function replace_string() in pg_regress assumes that there exists enough space in the string it operates on. While this is documented and expected, the consequence can be subtle failures in the tests which might be hard to trace back to the root cause. Now replace_string() operates in StringInfo instead, growing the string as nessecary. Also, pg_regress takes advantage of the growing capability of StringInfo and consumes lines greater than the original buffer, guarding against cutting the replace string along consecutive reads. Refactor ecpg's pg_regress slightly to make use of the new replace_string or remove it all together where applicable. Co-authored-by: Asim R P --- src/interfaces/ecpg/test/pg_regress_ecpg.c | 53 +++++++++---------- src/test/regress/pg_regress.c | 59 +++++++++++++++------- src/test/regress/pg_regress.h | 5 +- 3 files changed, 68 insertions(+), 49 deletions(-) diff --git a/src/interfaces/ecpg/test/pg_regress_ecpg.c b/src/interfaces/ecpg/test/pg_regress_ecpg.c index 46b9e78fe5..d6aa9c16d8 100644 --- a/src/interfaces/ecpg/test/pg_regress_ecpg.c +++ b/src/interfaces/ecpg/test/pg_regress_ecpg.c @@ -19,6 +19,7 @@ #include "postgres_fe.h" #include "pg_regress.h" +#include "lib/stringinfo.h" #define LINEBUFSIZE 300 @@ -52,7 +53,6 @@ ecpg_filter(const char *sourcefile, const char *outfile) if (strstr(linebuf, "#line ") == linebuf) { char *p = strchr(linebuf, '"'); - char *n; int plen = 1; while (*p && (*(p + plen) == '.' || strchr(p + plen, '/') != NULL)) @@ -62,9 +62,7 @@ ecpg_filter(const char *sourcefile, const char *outfile) /* plen is one more than the number of . and / characters */ if (plen > 1) { - n = (char *) malloc(plen); - strlcpy(n, p + 1, plen); - replace_string(linebuf, n, ""); + memmove(p + 1, p + plen, strlen(p + plen) + 1); } } fputs(linebuf, t); @@ -84,43 +82,43 @@ ecpg_start_test(const char *testname, _stringlist **expectfiles, _stringlist **tags) { + StringInfoData testname_dash; PID_TYPE pid; char inprg[MAXPGPATH]; char insource[MAXPGPATH]; - char *outfile_stdout, + char outfile_stdout[MAXPGPATH], expectfile_stdout[MAXPGPATH]; - char *outfile_stderr, + char outfile_stderr[MAXPGPATH], expectfile_stderr[MAXPGPATH]; - char *outfile_source, + char outfile_source[MAXPGPATH], expectfile_source[MAXPGPATH]; char cmd[MAXPGPATH * 3]; - char *testname_dash; char *appnameenv; snprintf(inprg, sizeof(inprg), "%s/%s", inputdir, testname); - testname_dash = strdup(testname); - replace_string(testname_dash, "/", "-"); + initStringInfo(&testname_dash); + appendStringInfoString(&testname_dash, testname); + replace_string(&testname_dash, "/", "-"); snprintf(expectfile_stdout, sizeof(expectfile_stdout), "%s/expected/%s.stdout", - outputdir, testname_dash); + outputdir, testname_dash.data); snprintf(expectfile_stderr, sizeof(expectfile_stderr), "%s/expected/%s.stderr", - outputdir, testname_dash); + outputdir, testname_dash.data); snprintf(expectfile_source, sizeof(expectfile_source), "%s/expected/%s.c", - outputdir, testname_dash); - - /* - * We can use replace_string() here because the replacement string does - * not occupy more space than the replaced one. - */ - outfile_stdout = strdup(expectfile_stdout); - replace_string(outfile_stdout, "/expected/", "/results/"); - outfile_stderr = strdup(expectfile_stderr); - replace_string(outfile_stderr, "/expected/", "/results/"); - outfile_source = strdup(expectfile_source); - replace_string(outfile_source, "/expected/", "/results/"); + outputdir, testname_dash.data); + + snprintf(outfile_stdout, sizeof(outfile_stdout), + "%s/results/%s.stdout", + outputdir, testname_dash.data); + snprintf(outfile_stderr, sizeof(outfile_stderr), + "%s/results/%s.stderr", + outputdir, testname_dash.data); + snprintf(outfile_source, sizeof(outfile_source), + "%s/results/%s.c", + outputdir, testname_dash.data); add_stringlist_item(resultfiles, outfile_stdout); add_stringlist_item(expectfiles, expectfile_stdout); @@ -145,7 +143,7 @@ ecpg_start_test(const char *testname, outfile_stdout, outfile_stderr); - appnameenv = psprintf("PGAPPNAME=ecpg/%s", testname_dash); + appnameenv = psprintf("PGAPPNAME=ecpg/%s", testname_dash.data); putenv(appnameenv); pid = spawn_process(cmd); @@ -160,10 +158,7 @@ ecpg_start_test(const char *testname, unsetenv("PGAPPNAME"); free(appnameenv); - free(testname_dash); - free(outfile_stdout); - free(outfile_stderr); - free(outfile_source); + free(testname_dash.data); return pid; } diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index d82e0189dc..185dc20c00 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -33,6 +33,7 @@ #include "common/restricted_token.h" #include "common/username.h" #include "getopt_long.h" +#include "lib/stringinfo.h" #include "libpq/pqcomm.h" /* needed for UNIXSOCK_PATH() */ #include "pg_config_paths.h" #include "pg_regress.h" @@ -434,23 +435,22 @@ string_matches_pattern(const char *str, const char *pattern) return false; } -/* - * Replace all occurrences of a string in a string with a different string. - * NOTE: Assumes there is enough room in the target buffer! - */ + void -replace_string(char *string, const char *replace, const char *replacement) +replace_string(StringInfo string, const char *replace, const char *replacement) { char *ptr; - while ((ptr = strstr(string, replace)) != NULL) + while ((ptr = strstr(string->data, replace)) != NULL) { - char *dup = pg_strdup(string); + char *suffix = pnstrdup(ptr + strlen(replace), string->maxlen); + size_t pos = ptr - string->data; - strlcpy(string, dup, ptr - string + 1); - strcat(string, replacement); - strcat(string, dup + (ptr - string) + strlen(replace)); - free(dup); + string->len = pos; + appendStringInfoString(string, replacement); + appendStringInfoString(string, suffix); + + free(suffix); } } @@ -521,7 +521,7 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch char prefix[MAXPGPATH]; FILE *infile, *outfile; - char line[1024]; + StringInfoData line; /* reject filenames not finishing in ".source" */ if (strlen(*name) < 8) @@ -551,15 +551,36 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch progname, destfile, strerror(errno)); exit(2); } - while (fgets(line, sizeof(line), infile)) + + initStringInfo(&line); + while (fgets(line.data, line.maxlen, infile)) { - replace_string(line, "@abs_srcdir@", inputdir); - replace_string(line, "@abs_builddir@", outputdir); - replace_string(line, "@testtablespace@", testtablespace); - replace_string(line, "@libdir@", dlpath); - replace_string(line, "@DLSUFFIX@", DLSUFFIX); - fputs(line, outfile); + /* continue reading if line was cut short and there is more input */ + if ((strlen(line.data) == line.maxlen - 1) && + line.data[line.maxlen - 2] != '\n') + { + char rest[1024]; + char *unused pg_attribute_unused(); + + line.len = line.maxlen - 1; + do + { + unused = fgets(rest, sizeof(rest), infile); + appendStringInfoString(&line, rest); + } while ((strlen(rest) == sizeof(rest) - 1) && + rest[sizeof(rest) - 2] != '\n'); + } + + replace_string(&line, "@abs_srcdir@", inputdir); + replace_string(&line, "@abs_builddir@", outputdir); + replace_string(&line, "@testtablespace@", testtablespace); + replace_string(&line, "@libdir@", dlpath); + replace_string(&line, "@DLSUFFIX@", DLSUFFIX); + fputs(line.data, outfile); + + resetStringInfo(&line); } + pfree(line.data); fclose(infile); fclose(outfile); } diff --git a/src/test/regress/pg_regress.h b/src/test/regress/pg_regress.h index ee6e0d42f4..d4640e0e2f 100644 --- a/src/test/regress/pg_regress.h +++ b/src/test/regress/pg_regress.h @@ -18,6 +18,8 @@ #define INVALID_PID INVALID_HANDLE_VALUE #endif +struct StringInfoData; /* not to include stringinfo.h here */ + /* simple list of strings */ typedef struct _stringlist { @@ -49,5 +51,6 @@ int regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc); void add_stringlist_item(_stringlist **listhead, const char *str); PID_TYPE spawn_process(const char *cmdline); -void replace_string(char *string, const char *replace, const char *replacement); +void replace_string(struct StringInfoData *string, + const char *replace, const char *replacement); bool file_exists(const char *file); -- 2.25.1