Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 5c7038d

Browse files
Refactor pipe_read_line to return the full line
Commit 5b2f4af refactored find_other_exec() and in the process created pipe_read_line() into a static routine for reading a single line of output, aimed at reading version numbers. Commit a7e8ece later exposed it externally in order to read a postgresql.conf GUC using "postgres -C ..". Further, f06b1c5 also made use of it for reading a version string much like find_other_exec(). The internal variable remained "pgver", even when used for other purposes. Since the function requires passing a buffer and its size, and at most size - 1 bytes will be read via fgets(), there is a truncation risk when using this for reading GUCs (like how pg_rewind does, though the risk in this case is marginal). To keep this as generic functionality for reading a line from a pipe, this refactors pipe_read_line() into returning an allocated buffer containing all of the line to remove the risk of silent truncation. Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi> Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: John Naylor <johncnaylorls@gmail.com> Discussion: https://postgr.es/m/DEDF73CE-D528-49A3-9089-B3592FD671A9@yesql.se
1 parent c01f6ef commit 5c7038d

File tree

4 files changed

+36
-25
lines changed

4 files changed

+36
-25
lines changed

src/bin/pg_rewind/pg_rewind.c

+6-8
Original file line numberDiff line numberDiff line change
@@ -1055,8 +1055,7 @@ static void
10551055
getRestoreCommand(const char *argv0)
10561056
{
10571057
int rc;
1058-
char postgres_exec_path[MAXPGPATH],
1059-
cmd_output[MAXPGPATH];
1058+
char postgres_exec_path[MAXPGPATH];
10601059
PQExpBuffer postgres_cmd;
10611060

10621061
if (!restore_wal)
@@ -1105,16 +1104,15 @@ getRestoreCommand(const char *argv0)
11051104
/* add -C switch, for restore_command */
11061105
appendPQExpBufferStr(postgres_cmd, " -C restore_command");
11071106

1108-
if (!pipe_read_line(postgres_cmd->data, cmd_output, sizeof(cmd_output)))
1109-
exit(1);
1107+
restore_command = pipe_read_line(postgres_cmd->data);
1108+
if (restore_command == NULL)
1109+
pg_fatal("unable to read restore_command from target cluster");
11101110

1111-
(void) pg_strip_crlf(cmd_output);
1111+
(void) pg_strip_crlf(restore_command);
11121112

1113-
if (strcmp(cmd_output, "") == 0)
1113+
if (strcmp(restore_command, "") == 0)
11141114
pg_fatal("restore_command is not set in the target cluster");
11151115

1116-
restore_command = pg_strdup(cmd_output);
1117-
11181116
pg_log_debug("using for rewind restore_command = \'%s\'",
11191117
restore_command);
11201118

src/bin/pg_upgrade/exec.c

+4-2
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ static void
431431
check_exec(const char *dir, const char *program, bool check_version)
432432
{
433433
char path[MAXPGPATH];
434-
char line[MAXPGPATH];
434+
char *line;
435435
char cmd[MAXPGPATH];
436436
char versionstr[128];
437437

@@ -442,7 +442,7 @@ check_exec(const char *dir, const char *program, bool check_version)
442442

443443
snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);
444444

445-
if (!pipe_read_line(cmd, line, sizeof(line)))
445+
if ((line = pipe_read_line(cmd)) == NULL)
446446
pg_fatal("check for \"%s\" failed: cannot execute",
447447
path);
448448

@@ -456,4 +456,6 @@ check_exec(const char *dir, const char *program, bool check_version)
456456
pg_fatal("check for \"%s\" failed: incorrect version: found \"%s\", expected \"%s\"",
457457
path, line, versionstr);
458458
}
459+
460+
pg_free(line);
459461
}

src/common/exec.c

+25-14
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@
4242
#endif
4343
#endif
4444

45+
#include "common/string.h"
46+
4547
/* Inhibit mingw CRT's auto-globbing of command line arguments */
4648
#if defined(WIN32) && !defined(_MSC_VER)
4749
extern int _CRT_glob = 0; /* 0 turns off globbing; 1 turns it on */
@@ -328,7 +330,7 @@ find_other_exec(const char *argv0, const char *target,
328330
const char *versionstr, char *retpath)
329331
{
330332
char cmd[MAXPGPATH];
331-
char line[MAXPGPATH];
333+
char *line;
332334

333335
if (find_my_exec(argv0, retpath) < 0)
334336
return -1;
@@ -346,46 +348,55 @@ find_other_exec(const char *argv0, const char *target,
346348

347349
snprintf(cmd, sizeof(cmd), "\"%s\" -V", retpath);
348350

349-
if (!pipe_read_line(cmd, line, sizeof(line)))
351+
if ((line = pipe_read_line(cmd)) == NULL)
350352
return -1;
351353

352354
if (strcmp(line, versionstr) != 0)
355+
{
356+
pfree(line);
353357
return -2;
358+
}
354359

360+
pfree(line);
355361
return 0;
356362
}
357363

358364

359365
/*
360-
* Execute a command in a pipe and read the first line from it.
366+
* Execute a command in a pipe and read the first line from it. The returned
367+
* string is palloc'd (malloc'd in frontend code), the caller is responsible
368+
* for freeing.
361369
*/
362370
char *
363-
pipe_read_line(char *cmd, char *line, int maxsize)
371+
pipe_read_line(char *cmd)
364372
{
365-
FILE *pgver;
373+
FILE *pipe_cmd;
374+
char *line;
366375

367376
fflush(NULL);
368377

369378
errno = 0;
370-
if ((pgver = popen(cmd, "r")) == NULL)
379+
if ((pipe_cmd = popen(cmd, "r")) == NULL)
371380
{
372381
perror("popen failure");
373382
return NULL;
374383
}
375384

385+
/* Make sure popen() didn't change errno */
376386
errno = 0;
377-
if (fgets(line, maxsize, pgver) == NULL)
387+
line = pg_get_line(pipe_cmd, NULL);
388+
389+
if (line == NULL)
378390
{
379-
if (feof(pgver))
380-
fprintf(stderr, "no data was returned by command \"%s\"\n", cmd);
391+
if (ferror(pipe_cmd))
392+
log_error(errcode_for_file_access(),
393+
_("could not read from command \"%s\": %m"), cmd);
381394
else
382-
perror("fgets failure");
383-
pclose(pgver); /* no error checking */
384-
return NULL;
395+
log_error(errcode_for_file_access(),
396+
_("no data was returned by command \"%s\": %m"), cmd);
385397
}
386398

387-
if (pclose_check(pgver))
388-
return NULL;
399+
(void) pclose_check(pipe_cmd);
389400

390401
return line;
391402
}

src/include/port.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ extern int validate_exec(const char *path);
137137
extern int find_my_exec(const char *argv0, char *retpath);
138138
extern int find_other_exec(const char *argv0, const char *target,
139139
const char *versionstr, char *retpath);
140-
extern char *pipe_read_line(char *cmd, char *line, int maxsize);
140+
extern char *pipe_read_line(char *cmd);
141141

142142
/* Doesn't belong here, but this is used with find_other_exec(), so... */
143143
#define PG_BACKEND_VERSIONSTR "postgres (PostgreSQL) " PG_VERSION "\n"

0 commit comments

Comments
 (0)