Skip to content

Commit b1aebbb

Browse files
sfrostsfrost
committed
Various Coverity-spotted fixes
A number of issues were identified by the Coverity scanner and are addressed in this patch. None of these appear to be security issues and many are mostly cosmetic changes. Short comments for each of the changes follows. Correct the semi-colon placement in be-secure.c regarding SSL retries. Remove a useless comparison-to-NULL in proc.c (value is dereferenced prior to this check and therefore can't be NULL). Add checking of chmod() return values to initdb. Fix a couple minor memory leaks in initdb. Fix memory leak in pg_ctl- involves free'ing the config file contents. Use an int to capture fgetc() return instead of an enum in pg_dump. Fix minor memory leaks in pg_dump. (note minor change to convertOperatorReference()'s API) Check fclose()/remove() return codes in psql. Check fstat(), find_my_exec() return codes in psql. Various ECPG memory leak fixes. Check find_my_exec() return in ECPG. Explicitly ignore pqFlush return in libpq error-path. Change PQfnumber() to avoid doing an strdup() when no changes required. Remove a few useless check-against-NULL's (value deref'd beforehand). Check rmtree(), malloc() results in pg_regress. Also check get_alternative_expectfile() return in pg_regress.
1 parent 9662143 commit b1aebbb

File tree

16 files changed

+290
-73
lines changed

16 files changed

+290
-73
lines changed

‎src/backend/libpq/be-secure.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,7 @@ secure_write(Port *port, void *ptr, size_t len)
371371
* A handshake can fail, so be prepared to retry it, but only
372372
* a few times.
373373
*/
374-
for (retries = 0; retries++;)
374+
for (retries = 0;; retries++)
375375
{
376376
if (SSL_do_handshake(port->ssl) > 0)
377377
break; /* done */

‎src/backend/storage/lmgr/proc.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,8 +1158,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
11581158
* Only do it if the worker is not working to protect against Xid
11591159
* wraparound.
11601160
*/
1161-
if ((autovac != NULL) &&
1162-
(autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
1161+
if ((autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
11631162
!(autovac_pgxact->vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))
11641163
{
11651164
int pid = autovac->pid;

‎src/bin/initdb/initdb.c

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,7 +1293,12 @@ setup_config(void)
12931293
snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
12941294

12951295
writefile(path, conflines);
1296-
chmod(path, S_IRUSR | S_IWUSR);
1296+
if (chmod(path, S_IRUSR | S_IWUSR) != 0)
1297+
{
1298+
fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
1299+
progname, path, strerror(errno));
1300+
exit_nicely();
1301+
}
12971302

12981303
/*
12991304
* create the automatic configuration file to store the configuration
@@ -1308,7 +1313,12 @@ setup_config(void)
13081313
sprintf(path, "%s/%s", pg_data, PG_AUTOCONF_FILENAME);
13091314

13101315
writefile(path, autoconflines);
1311-
chmod(path, S_IRUSR | S_IWUSR);
1316+
if (chmod(path, S_IRUSR | S_IWUSR) != 0)
1317+
{
1318+
fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
1319+
progname, path, strerror(errno));
1320+
exit_nicely();
1321+
}
13121322

13131323
free(conflines);
13141324

@@ -1387,7 +1397,12 @@ setup_config(void)
13871397
snprintf(path, sizeof(path), "%s/pg_hba.conf", pg_data);
13881398

13891399
writefile(path, conflines);
1390-
chmod(path, S_IRUSR | S_IWUSR);
1400+
if (chmod(path, S_IRUSR | S_IWUSR) != 0)
1401+
{
1402+
fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
1403+
progname, path, strerror(errno));
1404+
exit_nicely();
1405+
}
13911406

13921407
free(conflines);
13931408

@@ -1398,7 +1413,12 @@ setup_config(void)
13981413
snprintf(path, sizeof(path), "%s/pg_ident.conf", pg_data);
13991414

14001415
writefile(path, conflines);
1401-
chmod(path, S_IRUSR | S_IWUSR);
1416+
if (chmod(path, S_IRUSR | S_IWUSR) != 0)
1417+
{
1418+
fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
1419+
progname, path, strerror(errno));
1420+
exit_nicely();
1421+
}
14021422

14031423
free(conflines);
14041424

@@ -1957,8 +1977,14 @@ setup_collation(void)
19571977
* only, so this doesn't clash with "en_US" for LATIN1, say.
19581978
*/
19591979
if (normalize_locale_name(alias, localebuf))
1980+
{
1981+
char *quoted_alias = escape_quotes(alias);
1982+
19601983
PG_CMD_PRINTF3("INSERT INTO tmp_pg_collation VALUES (E'%s', E'%s', %d);\n",
1961-
escape_quotes(alias), quoted_locale, enc);
1984+
quoted_alias, quoted_locale, enc);
1985+
free(quoted_alias);
1986+
}
1987+
free(quoted_locale);
19621988
}
19631989

19641990
/* Add an SQL-standard name */

‎src/bin/pg_ctl/pg_ctl.c

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo,
154154

155155
static pgpid_t get_pgpid(void);
156156
static char **readfile(const char *path);
157+
static void free_readfile(char **optlines);
157158
static int start_postmaster(void);
158159
static void read_post_opts(void);
159160

@@ -369,6 +370,24 @@ readfile(const char *path)
369370
}
370371

371372

373+
/*
374+
* Free memory allocated for optlines through readfile()
375+
*/
376+
void
377+
free_readfile(char **optlines)
378+
{
379+
int i = 0;
380+
381+
if (!optlines)
382+
return;
383+
384+
while (optlines[i++])
385+
free(optlines[i]);
386+
387+
free(optlines);
388+
389+
return;
390+
}
372391

373392
/*
374393
* start/test/stop routines
@@ -572,6 +591,13 @@ test_postmaster_connection(bool do_checkpoint)
572591
}
573592
}
574593
}
594+
595+
/*
596+
* Free the results of readfile.
597+
*
598+
* This is safe to call even if optlines is NULL.
599+
*/
600+
free_readfile(optlines);
575601
}
576602

577603
/* If we have a connection string, ping the server */
@@ -708,6 +734,9 @@ read_post_opts(void)
708734
if (exec_path == NULL)
709735
exec_path = optline;
710736
}
737+
738+
/* Free the results of readfile. */
739+
free_readfile(optlines);
711740
}
712741
}
713742
}
@@ -1201,8 +1230,13 @@ do_status(void)
12011230

12021231
optlines = readfile(postopts_file);
12031232
if (optlines != NULL)
1233+
{
12041234
for (; *optlines != NULL; optlines++)
12051235
fputs(*optlines, stdout);
1236+
1237+
/* Free the results of readfile */
1238+
free_readfile(optlines);
1239+
}
12061240
return;
12071241
}
12081242
}

‎src/bin/pg_dump/pg_backup_archiver.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1947,8 +1947,10 @@ _discoverArchiveFormat(ArchiveHandle *AH)
19471947
else
19481948
AH->offSize = AH->intSize;
19491949

1950-
if ((AH->format = fgetc(fh)) == EOF)
1950+
if ((byteread = fgetc(fh)) == EOF)
19511951
exit_horribly(modulename, "could not read input file: %s\n", strerror(errno));
1952+
1953+
AH->format = byteread;
19521954
AH->lookahead[AH->lookaheadLen++] = AH->format;
19531955
}
19541956
else

‎src/bin/pg_dump/pg_dump.c

Lines changed: 54 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -234,9 +234,9 @@ static char *format_function_arguments_old(Archive *fout,
234234
char **argnames);
235235
static char *format_function_signature(Archive *fout,
236236
FuncInfo *finfo, bool honor_quotes);
237-
static const char *convertRegProcReference(Archive *fout,
237+
static char *convertRegProcReference(Archive *fout,
238238
const char *proc);
239-
static const char *convertOperatorReference(Archive *fout, const char *opr);
239+
static char *convertOperatorReference(Archive *fout, const char *opr);
240240
static const char *convertTSFunction(Archive *fout, Oid funcOid);
241241
static Oid findLastBuiltinOid_V71(Archive *fout, const char *);
242242
static Oid findLastBuiltinOid_V70(Archive *fout);
@@ -10246,6 +10246,8 @@ dumpOpr(Archive *fout, OprInfo *oprinfo)
1024610246
char *oprjoin;
1024710247
char *oprcanmerge;
1024810248
char *oprcanhash;
10249+
char *oprregproc;
10250+
char *oprref;
1024910251

1025010252
/* Skip if not to be dumped */
1025110253
if (!oprinfo->dobj.dump || dataOnly)
@@ -10352,8 +10354,12 @@ dumpOpr(Archive *fout, OprInfo *oprinfo)
1035210354
oprcanmerge = PQgetvalue(res, 0, i_oprcanmerge);
1035310355
oprcanhash = PQgetvalue(res, 0, i_oprcanhash);
1035410356

10355-
appendPQExpBuffer(details, " PROCEDURE = %s",
10356-
convertRegProcReference(fout, oprcode));
10357+
oprregproc = convertRegProcReference(fout, oprcode);
10358+
if (oprregproc)
10359+
{
10360+
appendPQExpBuffer(details, " PROCEDURE = %s", oprregproc);
10361+
free(oprregproc);
10362+
}
1035710363

1035810364
appendPQExpBuffer(oprid, "%s (",
1035910365
oprinfo->dobj.name);
@@ -10388,27 +10394,39 @@ dumpOpr(Archive *fout, OprInfo *oprinfo)
1038810394
else
1038910395
appendPQExpBufferStr(oprid, ", NONE)");
1039010396

10391-
name = convertOperatorReference(fout, oprcom);
10392-
if (name)
10393-
appendPQExpBuffer(details, ",\n COMMUTATOR = %s", name);
10397+
oprref = convertOperatorReference(fout, oprcom);
10398+
if (oprref)
10399+
{
10400+
appendPQExpBuffer(details, ",\n COMMUTATOR = %s", oprref);
10401+
free(oprref);
10402+
}
1039410403

10395-
name = convertOperatorReference(fout, oprnegate);
10396-
if (name)
10397-
appendPQExpBuffer(details, ",\n NEGATOR = %s", name);
10404+
oprref = convertOperatorReference(fout, oprnegate);
10405+
if (oprref)
10406+
{
10407+
appendPQExpBuffer(details, ",\n NEGATOR = %s", oprref);
10408+
free(oprref);
10409+
}
1039810410

1039910411
if (strcmp(oprcanmerge, "t") == 0)
1040010412
appendPQExpBufferStr(details, ",\n MERGES");
1040110413

1040210414
if (strcmp(oprcanhash, "t") == 0)
1040310415
appendPQExpBufferStr(details, ",\n HASHES");
1040410416

10405-
name = convertRegProcReference(fout, oprrest);
10406-
if (name)
10407-
appendPQExpBuffer(details, ",\n RESTRICT = %s", name);
10417+
oprregproc = convertRegProcReference(fout, oprrest);
10418+
if (oprregproc)
10419+
{
10420+
appendPQExpBuffer(details, ",\n RESTRICT = %s", oprregproc);
10421+
free(oprregproc);
10422+
}
1040810423

10409-
name = convertRegProcReference(fout, oprjoin);
10410-
if (name)
10411-
appendPQExpBuffer(details, ",\n JOIN = %s", name);
10424+
oprregproc = convertRegProcReference(fout, oprjoin);
10425+
if (oprregproc)
10426+
{
10427+
appendPQExpBuffer(details, ",\n JOIN = %s", oprregproc);
10428+
free(oprregproc);
10429+
}
1041210430

1041310431
/*
1041410432
* DROP must be fully qualified in case same name appears in pg_catalog
@@ -10453,12 +10471,13 @@ dumpOpr(Archive *fout, OprInfo *oprinfo)
1045310471
/*
1045410472
* Convert a function reference obtained from pg_operator
1045510473
*
10456-
* Returns what to print, or NULL if function references is InvalidOid
10474+
* Returns allocated string of what to print, or NULL if function references
10475+
* is InvalidOid. Returned string is expected to be free'd by the caller.
1045710476
*
1045810477
* In 7.3 the input is a REGPROCEDURE display; we have to strip the
1045910478
* argument-types part. In prior versions, the input is a REGPROC display.
1046010479
*/
10461-
static const char *
10480+
static char *
1046210481
convertRegProcReference(Archive *fout, const char *proc)
1046310482
{
1046410483
/* In all cases "-" means a null reference */
@@ -10488,20 +10507,21 @@ convertRegProcReference(Archive *fout, const char *proc)
1048810507
}
1048910508

1049010509
/* REGPROC before 7.3 does not quote its result */
10491-
return fmtId(proc);
10510+
return pg_strdup(fmtId(proc));
1049210511
}
1049310512

1049410513
/*
1049510514
* Convert an operator cross-reference obtained from pg_operator
1049610515
*
10497-
* Returns what to print, or NULL to print nothing
10516+
* Returns an allocated string of what to print, or NULL to print nothing.
10517+
* Caller is responsible for free'ing result string.
1049810518
*
1049910519
* In 7.3 and up the input is a REGOPERATOR display; we have to strip the
1050010520
* argument-types part, and add OPERATOR() decoration if the name is
1050110521
* schema-qualified. In older versions, the input is just a numeric OID,
1050210522
* which we search our operator list for.
1050310523
*/
10504-
static const char *
10524+
static char *
1050510525
convertOperatorReference(Archive *fout, const char *opr)
1050610526
{
1050710527
OprInfo *oprInfo;
@@ -10549,7 +10569,7 @@ convertOperatorReference(Archive *fout, const char *opr)
1054910569
opr);
1055010570
return NULL;
1055110571
}
10552-
return oprInfo->dobj.name;
10572+
return pg_strdup(oprInfo->dobj.name);
1055310573
}
1055410574

1055510575
/*
@@ -11522,6 +11542,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
1152211542
const char *aggtransfn;
1152311543
const char *aggfinalfn;
1152411544
const char *aggsortop;
11545+
char *aggsortconvop;
1152511546
bool hypothetical;
1152611547
const char *aggtranstype;
1152711548
const char *aggtransspace;
@@ -11665,6 +11686,12 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
1166511686
{
1166611687
write_msg(NULL, "WARNING: aggregate function %s could not be dumped correctly for this database version; ignored\n",
1166711688
aggsig);
11689+
11690+
if (aggfullsig)
11691+
free(aggfullsig);
11692+
11693+
free(aggsig);
11694+
1166811695
return;
1166911696
}
1167011697

@@ -11709,11 +11736,12 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
1170911736
aggfinalfn);
1171011737
}
1171111738

11712-
aggsortop = convertOperatorReference(fout, aggsortop);
11713-
if (aggsortop)
11739+
aggsortconvop = convertOperatorReference(fout, aggsortop);
11740+
if (aggsortconvop)
1171411741
{
1171511742
appendPQExpBuffer(details, ",\n SORTOP = %s",
11716-
aggsortop);
11743+
aggsortconvop);
11744+
free(aggsortconvop);
1171711745
}
1171811746

1171911747
if (hypothetical)
@@ -12413,6 +12441,7 @@ dumpUserMappings(Archive *fout,
1241312441

1241412442
destroyPQExpBuffer(query);
1241512443
destroyPQExpBuffer(delq);
12444+
destroyPQExpBuffer(tag);
1241612445
destroyPQExpBuffer(q);
1241712446
}
1241812447

‎src/bin/psql/command.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2027,14 +2027,20 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
20272027
if (fwrite(query_buf->data, 1, ql, stream) != ql)
20282028
{
20292029
psql_error("%s: %s\n", fname, strerror(errno));
2030-
fclose(stream);
2031-
remove(fname);
2030+
2031+
if (fclose(stream) != 0)
2032+
psql_error("%s: %s\n", fname, strerror(errno));
2033+
2034+
if (remove(fname) != 0)
2035+
psql_error("%s: %s\n", fname, strerror(errno));
2036+
20322037
error = true;
20332038
}
20342039
else if (fclose(stream) != 0)
20352040
{
20362041
psql_error("%s: %s\n", fname, strerror(errno));
2037-
remove(fname);
2042+
if (remove(fname) != 0)
2043+
psql_error("%s: %s\n", fname, strerror(errno));
20382044
error = true;
20392045
}
20402046
}

0 commit comments

Comments
 (0)