Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nKBxH-0002fu-Tr for pgsql-bugs@arkaria.postgresql.org; Wed, 16 Feb 2022 04:30:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1nKBxG-0002hY-Ie for pgsql-bugs@arkaria.postgresql.org; Wed, 16 Feb 2022 04:30:14 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nKBxG-0002hN-88 for pgsql-bugs@lists.postgresql.org; Wed, 16 Feb 2022 04:30:14 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nKBxD-0003Mf-HP for pgsql-bugs@lists.postgresql.org; Wed, 16 Feb 2022 04:30:13 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 21G4U72a3255494; Tue, 15 Feb 2022 23:30:07 -0500 From: Tom Lane To: Andres Freund cc: Daniel Gustafsson , wliang@stu.xidian.edu.cn, pgsql-bugs Subject: Re: Report a potential memory leak in setup_config() In-reply-to: <20220216041049.uy3vwtk6ry66g6um@alap3.anarazel.de> References: <64ee7543.20d.17efb1270ca.Coremail.wliang@stu.xidian.edu.cn> <3052163.1644942806@sss.pgh.pa.us> <20220216012953.6d7bzmsblqou3ru4@alap3.anarazel.de> <3239746.1644975946@sss.pgh.pa.us> <20220216022118.vvijtgd5rl2b6ucv@alap3.anarazel.de> <3243399.1644978358@sss.pgh.pa.us> <3244453.1644979091@sss.pgh.pa.us> <20220216041049.uy3vwtk6ry66g6um@alap3.anarazel.de> Comments: In-reply-to Andres Freund message dated "Tue, 15 Feb 2022 20:10:49 -0800" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3255375.1644985801.0@sss.pgh.pa.us> Date: Tue, 15 Feb 2022 23:30:07 -0500 Message-ID: <3255493.1644985807@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3255375.1644985801.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Andres Freund writes: > When I'd looked at this last I apparently (looking at git stash, I ended= up > discarding this) decided that the best way would be to change replace_to= ken's > API to one that just processes one line at a time, with an outer loop th= at > processes all tokens in a line. I'm not really sure why anymore. Hmm, I did it the other way, as attached. This gets it down to =3D=3D3254266=3D=3D LEAK SUMMARY: =3D=3D3254266=3D=3D definitely lost: 342 bytes in 17 blocks =3D=3D3254266=3D=3D indirectly lost: 152 bytes in 2 blocks =3D=3D3254266=3D=3D possibly lost: 0 bytes in 0 blocks =3D=3D3254266=3D=3D still reachable: 2,403 bytes in 22 blocks =3D=3D3254266=3D=3D suppressed: 0 bytes in 0 blocks It seems that actually the pointer arrays *are* a big chunk of the leakage, because the individual strings get freed in the output loops! That's a bit ugly but I feel no need to change it. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="alternate-initdb-cleanup.patch"; charset="us-ascii" Content-ID: <3255375.1644985801.2@sss.pgh.pa.us> Content-Description: alternate-initdb-cleanup.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 97f15971e2..76cb3a142e 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -237,11 +237,11 @@ static const char *const subdirs[] =3D { static char bin_path[MAXPGPATH]; static char backend_exec[MAXPGPATH]; = -static char **replace_token(char **lines, +static void replace_token(char **lines, const char *token, const char *replacement); = #ifndef HAVE_UNIX_SOCKETS -static char **filter_lines_with_token(char **lines, const char *token); +static void remove_lines_with_token(char **lines, const char *token); #endif static char **readfile(const char *path); static void writefile(char *path, char **lines); @@ -363,43 +363,33 @@ escape_quotes_bki(const char *src) } = /* - * make a copy of the array of lines, with token replaced by replacement + * Modify an array of malloc'd strings, replacing token by replacement * the first time it occurs on each line. * * This does most of what sed was used for in the shell script, but * doesn't need any regexp stuff. */ -static char ** +static void replace_token(char **lines, const char *token, const char *replacement) { - int numlines =3D 1; int i; - char **result; int toklen, replen, diff; = - for (i =3D 0; lines[i]; i++) - numlines++; - - result =3D (char **) pg_malloc(numlines * sizeof(char *)); - toklen =3D strlen(token); replen =3D strlen(replacement); diff =3D replen - toklen; = - for (i =3D 0; i < numlines; i++) + for (i =3D 0; lines[i]; i++) { char *where; char *newline; int pre; = - /* just copy pointer if NULL or no change needed */ - if (lines[i] =3D=3D NULL || (where =3D strstr(lines[i], token)) =3D=3D = NULL) - { - result[i] =3D lines[i]; + /* no work if no change needed */ + if ((where =3D strstr(lines[i], token)) =3D=3D NULL) continue; - } = /* if we get here a change is needed - set up new line */ = @@ -413,39 +403,31 @@ replace_token(char **lines, const char *token, const= char *replacement) = strcpy(newline + pre + replen, lines[i] + pre + toklen); = - result[i] =3D newline; + free(lines[i]); + lines[i] =3D newline; } - - return result; } = /* - * make a copy of lines without any that contain the token + * remove strings from lines[] if they contain the token * * a sort of poor man's grep -v */ #ifndef HAVE_UNIX_SOCKETS -static char ** -filter_lines_with_token(char **lines, const char *token) +static void +remove_lines_with_token(char **lines, const char *token) { - int numlines =3D 1; - int i, - src, + int src, dst; - char **result; - - for (i =3D 0; lines[i]; i++) - numlines++; - - result =3D (char **) pg_malloc(numlines * sizeof(char *)); = - for (src =3D 0, dst =3D 0; src < numlines; src++) + for (src =3D 0, dst =3D 0; lines[src]; src++) { - if (lines[src] =3D=3D NULL || strstr(lines[src], token) =3D=3D NULL) - result[dst++] =3D lines[src]; + if (strstr(lines[src], token) =3D=3D NULL) + lines[dst++] =3D lines[src]; + else + free(lines[src]); } - - return result; + lines[dst] =3D NULL; } #endif = @@ -1066,7 +1048,7 @@ setup_config(void) conflines =3D readfile(conf_file); = snprintf(repltok, sizeof(repltok), "max_connections =3D %d", n_connectio= ns); - conflines =3D replace_token(conflines, "#max_connections =3D 100", replt= ok); + replace_token(conflines, "#max_connections =3D 100", repltok); = if ((n_buffers * (BLCKSZ / 1024)) % 1024 =3D=3D 0) snprintf(repltok, sizeof(repltok), "shared_buffers =3D %dMB", @@ -1074,7 +1056,7 @@ setup_config(void) else snprintf(repltok, sizeof(repltok), "shared_buffers =3D %dkB", n_buffers * (BLCKSZ / 1024)); - conflines =3D replace_token(conflines, "#shared_buffers =3D 128MB", repl= tok); + replace_token(conflines, "#shared_buffers =3D 128MB", repltok); = #ifdef HAVE_UNIX_SOCKETS snprintf(repltok, sizeof(repltok), "#unix_socket_directories =3D '%s'", @@ -1082,38 +1064,38 @@ setup_config(void) #else snprintf(repltok, sizeof(repltok), "#unix_socket_directories =3D ''"); #endif - conflines =3D replace_token(conflines, "#unix_socket_directories =3D '/t= mp'", + replace_token(conflines, "#unix_socket_directories =3D '/tmp'", repltok); = #if DEF_PGPORT !=3D 5432 snprintf(repltok, sizeof(repltok), "#port =3D %d", DEF_PGPORT); - conflines =3D replace_token(conflines, "#port =3D 5432", repltok); + replace_token(conflines, "#port =3D 5432", repltok); #endif = /* set default max_wal_size and min_wal_size */ snprintf(repltok, sizeof(repltok), "min_wal_size =3D %s", pretty_wal_size(DEFAULT_MIN_WAL_SEGS)); - conflines =3D replace_token(conflines, "#min_wal_size =3D 80MB", repltok= ); + replace_token(conflines, "#min_wal_size =3D 80MB", repltok); = snprintf(repltok, sizeof(repltok), "max_wal_size =3D %s", pretty_wal_size(DEFAULT_MAX_WAL_SEGS)); - conflines =3D replace_token(conflines, "#max_wal_size =3D 1GB", repltok)= ; + replace_token(conflines, "#max_wal_size =3D 1GB", repltok); = snprintf(repltok, sizeof(repltok), "lc_messages =3D '%s'", escape_quotes(lc_messages)); - conflines =3D replace_token(conflines, "#lc_messages =3D 'C'", repltok); + replace_token(conflines, "#lc_messages =3D 'C'", repltok); = snprintf(repltok, sizeof(repltok), "lc_monetary =3D '%s'", escape_quotes(lc_monetary)); - conflines =3D replace_token(conflines, "#lc_monetary =3D 'C'", repltok); + replace_token(conflines, "#lc_monetary =3D 'C'", repltok); = snprintf(repltok, sizeof(repltok), "lc_numeric =3D '%s'", escape_quotes(lc_numeric)); - conflines =3D replace_token(conflines, "#lc_numeric =3D 'C'", repltok); + replace_token(conflines, "#lc_numeric =3D 'C'", repltok); = snprintf(repltok, sizeof(repltok), "lc_time =3D '%s'", escape_quotes(lc_time)); - conflines =3D replace_token(conflines, "#lc_time =3D 'C'", repltok); + replace_token(conflines, "#lc_time =3D 'C'", repltok); = switch (locale_date_order(lc_time)) { @@ -1128,12 +1110,12 @@ setup_config(void) strcpy(repltok, "datestyle =3D 'iso, mdy'"); break; } - conflines =3D replace_token(conflines, "#datestyle =3D 'iso, mdy'", repl= tok); + replace_token(conflines, "#datestyle =3D 'iso, mdy'", repltok); = snprintf(repltok, sizeof(repltok), "default_text_search_config =3D 'pg_catalog.%s'", escape_quotes(default_text_search_config)); - conflines =3D replace_token(conflines, + replace_token(conflines, "#default_text_search_config =3D 'pg_catalog.simple'", repltok); = @@ -1141,46 +1123,46 @@ setup_config(void) { snprintf(repltok, sizeof(repltok), "timezone =3D '%s'", escape_quotes(default_timezone)); - conflines =3D replace_token(conflines, "#timezone =3D 'GMT'", repltok); + replace_token(conflines, "#timezone =3D 'GMT'", repltok); snprintf(repltok, sizeof(repltok), "log_timezone =3D '%s'", escape_quotes(default_timezone)); - conflines =3D replace_token(conflines, "#log_timezone =3D 'GMT'", replt= ok); + replace_token(conflines, "#log_timezone =3D 'GMT'", repltok); } = snprintf(repltok, sizeof(repltok), "dynamic_shared_memory_type =3D %s", dynamic_shared_memory_type); - conflines =3D replace_token(conflines, "#dynamic_shared_memory_type =3D = posix", + replace_token(conflines, "#dynamic_shared_memory_type =3D posix", repltok); = #if DEFAULT_BACKEND_FLUSH_AFTER > 0 snprintf(repltok, sizeof(repltok), "#backend_flush_after =3D %dkB", DEFAULT_BACKEND_FLUSH_AFTER * (BLCKSZ / 1024)); - conflines =3D replace_token(conflines, "#backend_flush_after =3D 0", + replace_token(conflines, "#backend_flush_after =3D 0", repltok); #endif = #if DEFAULT_BGWRITER_FLUSH_AFTER > 0 snprintf(repltok, sizeof(repltok), "#bgwriter_flush_after =3D %dkB", DEFAULT_BGWRITER_FLUSH_AFTER * (BLCKSZ / 1024)); - conflines =3D replace_token(conflines, "#bgwriter_flush_after =3D 0", + replace_token(conflines, "#bgwriter_flush_after =3D 0", repltok); #endif = #if DEFAULT_CHECKPOINT_FLUSH_AFTER > 0 snprintf(repltok, sizeof(repltok), "#checkpoint_flush_after =3D %dkB", DEFAULT_CHECKPOINT_FLUSH_AFTER * (BLCKSZ / 1024)); - conflines =3D replace_token(conflines, "#checkpoint_flush_after =3D 0", + replace_token(conflines, "#checkpoint_flush_after =3D 0", repltok); #endif = #ifndef USE_PREFETCH - conflines =3D replace_token(conflines, + replace_token(conflines, "#effective_io_concurrency =3D 1", "#effective_io_concurrency =3D 0"); #endif = #ifdef WIN32 - conflines =3D replace_token(conflines, + replace_token(conflines, "#update_process_title =3D on", "#update_process_title =3D off"); #endif @@ -1194,7 +1176,7 @@ setup_config(void) (strcmp(authmethodhost, "md5") =3D=3D 0 && strcmp(authmethodlocal, "scram-sha-256") !=3D 0)) { - conflines =3D replace_token(conflines, + replace_token(conflines, "#password_encryption =3D scram-sha-256", "password_encryption =3D md5"); } @@ -1207,7 +1189,7 @@ setup_config(void) */ if (pg_dir_create_mode =3D=3D PG_DIR_MODE_GROUP) { - conflines =3D replace_token(conflines, + replace_token(conflines, "#log_file_mode =3D 0600", "log_file_mode =3D 0640"); } @@ -1248,9 +1230,9 @@ setup_config(void) conflines =3D readfile(hba_file); = #ifndef HAVE_UNIX_SOCKETS - conflines =3D filter_lines_with_token(conflines, "@remove-line-for-noloc= al@"); + remove_lines_with_token(conflines, "@remove-line-for-nolocal@"); #else - conflines =3D replace_token(conflines, "@remove-line-for-nolocal@", ""); + replace_token(conflines, "@remove-line-for-nolocal@", ""); #endif = #ifdef HAVE_IPV6 @@ -1287,33 +1269,33 @@ setup_config(void) if (err !=3D 0 || getaddrinfo("::1", NULL, &hints, &gai_result) !=3D 0) { - conflines =3D replace_token(conflines, + replace_token(conflines, "host all all ::1", "#host all all ::1"); - conflines =3D replace_token(conflines, + replace_token(conflines, "host replication all ::1", "#host replication all ::1"); } } #else /* !HAVE_IPV6 */ /* If we didn't compile IPV6 support at all, always comment it out */ - conflines =3D replace_token(conflines, + replace_token(conflines, "host all all ::1", "#host all all ::1"); - conflines =3D replace_token(conflines, + replace_token(conflines, "host replication all ::1", "#host replication all ::1"); #endif /* HAVE_IPV6 */ = /* Replace default authentication methods */ - conflines =3D replace_token(conflines, + replace_token(conflines, "@authmethodhost@", authmethodhost); - conflines =3D replace_token(conflines, + replace_token(conflines, "@authmethodlocal@", authmethodlocal); = - conflines =3D replace_token(conflines, + replace_token(conflines, "@authcomment@", (strcmp(authmethodlocal, "trust") =3D=3D 0 || strcmp(authmethodh= ost, "trust") =3D=3D 0) ? AUTHTRUST_WARNING : ""); = @@ -1382,27 +1364,27 @@ bootstrap_template1(void) /* Substitute for various symbols used in the BKI file */ = sprintf(buf, "%d", NAMEDATALEN); - bki_lines =3D replace_token(bki_lines, "NAMEDATALEN", buf); + replace_token(bki_lines, "NAMEDATALEN", buf); = sprintf(buf, "%d", (int) sizeof(Pointer)); - bki_lines =3D replace_token(bki_lines, "SIZEOF_POINTER", buf); + replace_token(bki_lines, "SIZEOF_POINTER", buf); = - bki_lines =3D replace_token(bki_lines, "ALIGNOF_POINTER", + replace_token(bki_lines, "ALIGNOF_POINTER", (sizeof(Pointer) =3D=3D 4) ? "i" : "d"); = - bki_lines =3D replace_token(bki_lines, "FLOAT8PASSBYVAL", + replace_token(bki_lines, "FLOAT8PASSBYVAL", FLOAT8PASSBYVAL ? "true" : "false"); = - bki_lines =3D replace_token(bki_lines, "POSTGRES", + replace_token(bki_lines, "POSTGRES", escape_quotes_bki(username)); = - bki_lines =3D replace_token(bki_lines, "ENCODING", + replace_token(bki_lines, "ENCODING", encodingid_to_string(encodingid)); = - bki_lines =3D replace_token(bki_lines, "LC_COLLATE", + replace_token(bki_lines, "LC_COLLATE", escape_quotes_bki(lc_collate)); = - bki_lines =3D replace_token(bki_lines, "LC_CTYPE", + replace_token(bki_lines, "LC_CTYPE", escape_quotes_bki(lc_ctype)); = /* Also ensure backend isn't confused by this environment var: */ @@ -1624,7 +1606,8 @@ setup_privileges(FILE *cmdfd) { char **line; char **priv_lines; - static char *privileges_setup[] =3D { + int i; + static const char * const privileges_setup[] =3D { "UPDATE pg_class " " SET relacl =3D (SELECT array_agg(a.acl) FROM " " (SELECT E'=3Dr/\"$POSTGRES_SUPERUSERNAME\"' as acl " @@ -1759,10 +1742,19 @@ setup_privileges(FILE *cmdfd) NULL }; = - priv_lines =3D replace_token(privileges_setup, "$POSTGRES_SUPERUSERNAME"= , + priv_lines =3D (char**) pg_malloc(lengthof(privileges_setup) * sizeof(ch= ar*)); + for (i =3D 0; privileges_setup[i]; i++) + priv_lines[i] =3D pg_strdup(privileges_setup[i]); + priv_lines[i] =3D NULL; + + replace_token(priv_lines, "$POSTGRES_SUPERUSERNAME", escape_quotes(username)); for (line =3D priv_lines; *line !=3D NULL; line++) + { PG_CMD_PUTS(*line); + free(*line); + } + free(priv_lines); } = /* ------- =_aaaaaaaaaa0--