Skip to content

Commit

Permalink
bpipe-fd: stderr is merged with stdout, possible corruption.
Browse files Browse the repository at this point in the history 
When changing from using popen() to open_bpipe() it was overlooked that
popen() just ignores stderr but open_bpipe() dups both stdout and stderr
onto the read pipe used. This patch adds a flag to open_bpipe() which is
by default set to true to dup stderr or not. For now this fixes the
problem but a better solution could probably be coded by adding an extra
member to the BPIPE class with the name efd which is the error handle
just as we now have a write and read handle.

Fixes #632: fd-bpipe plugin merges stderr with stdout, which can result
            in corrupted backups.
  • Loading branch information
Marco van Wieringen committed Mar 11, 2016
1 parent fab6ead commit 747d34c
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 42 deletions.
109 changes: 69 additions & 40 deletions src/lib/bpipe.c
View file
Expand Up @@ -2,6 +2,7 @@
BAREOS® - Backup Archiving REcovery Open Sourced
Copyright (C) 2002-2011 Free Software Foundation Europe e.V.
Copyright (C) 2013-2016 Bareos GmbH & Co. KG
This program is Free Software; you can redistribute it and/or
modify it under the terms of version three of the GNU Affero General Public
Expand Down Expand Up @@ -42,19 +43,18 @@ int execvp_errors[] = {
};
int num_execvp_errors = (int)(sizeof(execvp_errors)/sizeof(int));


#define MAX_ARGV 100

#if !defined(HAVE_WIN32)
static void build_argc_argv(char *cmd, int *bargc, char *bargv[], int max_arg);

/*
* Run an external program. Optionally wait a specified number
* of seconds. Program killed if wait exceeded. We open
* a bi-directional pipe so that the user can read from and
* write to the program.
* of seconds. Program killed if wait exceeded. We open
* a bi-directional pipe so that the user can read from and
* write to the program.
*/
BPIPE *open_bpipe(char *prog, int wait, const char *mode)
BPIPE *open_bpipe(char *prog, int wait, const char *mode, bool dup_stderr)
{
char *bargv[MAX_ARGV];
int bargc, i;
Expand All @@ -68,18 +68,17 @@ BPIPE *open_bpipe(char *prog, int wait, const char *mode)
memset(bpipe, 0, sizeof(BPIPE));
mode_read = (mode[0] == 'r');
mode_write = (mode[0] == 'w' || mode[1] == 'w');
/* Build arguments for running program. */

/*
* Build arguments for running program.
*/
tprog = get_pool_memory(PM_FNAME);
pm_strcpy(tprog, prog);
build_argc_argv(tprog, &bargc, bargv, MAX_ARGV);
#ifdef xxxxxx
printf("argc=%d\n", bargc);
for (i=0; i<bargc; i++) {
printf("argc=%d argv=%s:\n", i, bargv[i]);
}
#endif

/* Each pipe is one way, write one end, read the other, so we need two */
/*
* Each pipe is one way, write one end, read the other, so we need two
*/
if (mode_write && pipe(writep) == -1) {
save_errno = errno;
free(bpipe);
Expand All @@ -98,7 +97,10 @@ BPIPE *open_bpipe(char *prog, int wait, const char *mode)
errno = save_errno;
return NULL;
}
/* Start worker process */

/*
* Start worker process
*/
switch (bpipe->worker_pid = fork()) {
case -1: /* error */
save_errno = errno;
Expand All @@ -123,10 +125,10 @@ BPIPE *open_bpipe(char *prog, int wait, const char *mode)
if (mode_read) {
close(readp[0]); /* Close unused child fds */
dup2(readp[1], 1); /* dup our read to his stdout */
dup2(readp[1], 2); /* and his stderr */
if (dup_stderr) {
dup2(readp[1], 2); /* and his stderr */
}
}
/* Note, the close log cause problems, see bug #1536 */
/* closelog(); close syslog if open */

#if defined(HAVE_FCNTL_F_CLOSEM)
/*
Expand All @@ -145,8 +147,11 @@ BPIPE *open_bpipe(char *prog, int wait, const char *mode)
#endif

execvp(bargv[0], bargv); /* call the program */
/* Convert errno into an exit code for later analysis */
for (i=0; i< num_execvp_errors; i++) {

/*
* Convert errno into an exit code for later analysis
*/
for (i = 0; i < num_execvp_errors; i++) {
if (execvp_errors[i] == errno) {
exit(200 + i); /* exit code => errno */
}
Expand All @@ -156,24 +161,32 @@ BPIPE *open_bpipe(char *prog, int wait, const char *mode)
default: /* parent */
break;
}

free_pool_memory(tprog);

if (mode_read) {
close(readp[1]); /* close unused parent fds */
bpipe->rfd = fdopen(readp[0], "r"); /* open file descriptor */
}

if (mode_write) {
close(writep[0]);
bpipe->wfd = fdopen(writep[1], "w");
}

bpipe->worker_stime = time(NULL);
bpipe->wait = wait;

if (wait > 0) {
bpipe->timer_id = start_child_timer(NULL, bpipe->worker_pid, wait);
}

return bpipe;
}

/* Close the write pipe only */
/*
* Close the write pipe only
*/
int close_wpipe(BPIPE *bpipe)
{
int status = 1;
Expand All @@ -191,8 +204,8 @@ int close_wpipe(BPIPE *bpipe)
/*
* Close both pipes and free resources
*
* Returns: 0 on success
* berrno on failure
* Returns: 0 on success
* berrno on failure
*/
int close_bpipe(BPIPE *bpipe)
{
Expand All @@ -203,11 +216,14 @@ int close_bpipe(BPIPE *bpipe)
pid_t wpid = 0;


/* Close pipes */
/*
* Close pipes
*/
if (bpipe->rfd) {
fclose(bpipe->rfd);
bpipe->rfd = NULL;
}

if (bpipe->wfd) {
fclose(bpipe->wfd);
bpipe->wfd = NULL;
Expand All @@ -220,7 +236,9 @@ int close_bpipe(BPIPE *bpipe)
}
remaining_wait = bpipe->wait;

/* wait for worker child to exit */
/*
* Wait for worker child to exit
*/
for ( ;; ) {
Dmsg2(800, "Wait for %d opt=%d\n", bpipe->worker_pid, wait_option);
do {
Expand All @@ -244,6 +262,7 @@ int close_bpipe(BPIPE *bpipe)
break; /* don't wait any longer */
}
}

if (wpid > 0) {
if (WIFEXITED(chldstatus)) { /* process exit()ed */
status = WEXITSTATUS(chldstatus);
Expand All @@ -262,11 +281,14 @@ int close_bpipe(BPIPE *bpipe)
status |= b_errno_signal; /* exit signal returned */
}
}

if (bpipe->timer_id) {
stop_child_timer(bpipe->timer_id);
}

free(bpipe);
Dmsg2(800, "returning status=%d,%d\n", status & ~(b_errno_exit|b_errno_signal), status);

return status;
}

Expand Down Expand Up @@ -320,16 +342,16 @@ static void build_argc_argv(char *cmd, int *bargc, char *bargv[], int max_argv)

/*
* Run an external program. Optionally wait a specified number
* of seconds. Program killed if wait exceeded. Optionally
* return the output from the program (normally a single line).
* of seconds. Program killed if wait exceeded. Optionally
* return the output from the program (normally a single line).
*
* If the watchdog kills the program, fgets returns, and ferror is set
* to 1 (=>SUCCESS), so we check if the watchdog killed the program.
* If the watchdog kills the program, fgets returns, and ferror is set
* to 1 (=>SUCCESS), so we check if the watchdog killed the program.
*
* Contrary to my normal calling conventions, this program
*
* Returns: 0 on success
* non-zero on error == berrno status
* Returns: 0 on success
* non-zero on error == berrno status
*/
int run_program(char *prog, int wait, POOLMEM *&results)
{
Expand All @@ -342,25 +364,30 @@ int run_program(char *prog, int wait, POOLMEM *&results)
if (!bpipe) {
return ENOENT;
}

results[0] = 0;
int len = sizeof_pool_memory(results) - 1;
fgets(results, len, bpipe->rfd);
results[len] = 0;

if (feof(bpipe->rfd)) {
stat1 = 0;
} else {
stat1 = ferror(bpipe->rfd);
}

if (stat1 < 0) {
berrno be;
Dmsg2(150, "Run program fgets stat=%d ERR=%s\n", stat1, be.bstrerror(errno));
} else if (stat1 != 0) {
Dmsg1(150, "Run program fgets stat=%d\n", stat1);
if (bpipe->timer_id) {
Dmsg1(150, "Run program fgets killed=%d\n", bpipe->timer_id->killed);
/* NB: I'm not sure it is really useful for run_program. Without the
/*
* NB: I'm not sure it is really useful for run_program. Without the
* following lines run_program would not detect if the program was killed
* by the watchdog. */
* by the watchdog.
*/
if (bpipe->timer_id->killed) {
stat1 = ETIME;
pm_strcpy(results, _("Program killed by BAREOS (timeout)\n"));
Expand All @@ -370,24 +397,24 @@ int run_program(char *prog, int wait, POOLMEM *&results)
stat2 = close_bpipe(bpipe);
stat1 = stat2 != 0 ? stat2 : stat1;
Dmsg1(150, "Run program returning %d\n", stat1);

return stat1;
}

/*
* Run an external program. Optionally wait a specified number
* of seconds. Program killed if wait exceeded (it is done by the
* watchdog, as fgets is a blocking function).
* of seconds. Program killed if wait exceeded (it is done by the
* watchdog, as fgets is a blocking function).
*
* If the watchdog kills the program, fgets returns, and ferror is set
* to 1 (=>SUCCESS), so we check if the watchdog killed the program.
* If the watchdog kills the program, fgets returns, and ferror is set
* to 1 (=>SUCCESS), so we check if the watchdog killed the program.
*
* Return the full output from the program (not only the first line).
* Return the full output from the program (not only the first line).
*
* Contrary to my normal calling conventions, this program
*
* Returns: 0 on success
* non-zero on error == berrno status
*
* Returns: 0 on success
* non-zero on error == berrno status
*/
int run_program_full_output(char *prog, int wait, POOLMEM *&results)
{
Expand Down Expand Up @@ -438,6 +465,7 @@ int run_program_full_output(char *prog, int wait, POOLMEM *&results)
}
}
}

/*
* We always check whether the timer killed the program. We would see
* an eof even when it does so we just have to trust the killed flag
Expand All @@ -449,6 +477,7 @@ int run_program_full_output(char *prog, int wait, POOLMEM *&results)
pm_strcpy(tmp, _("Program killed by BAREOS (timeout)\n"));
stat1 = ETIME;
}

pm_strcpy(results, tmp);
Dmsg3(1900, "resadr=0x%x reslen=%d res=%s\n", results, strlen(results), results);
stat2 = close_bpipe(bpipe);
Expand Down
3 changes: 2 additions & 1 deletion src/lib/protos.h
View file
Expand Up @@ -89,7 +89,8 @@ void bnet_thread_server_tcp(dlist *addr_list,
void bnet_stop_thread_server_tcp(pthread_t tid);

/* bpipe.c */
BPIPE *open_bpipe(char *prog, int wait, const char *mode);
BPIPE *open_bpipe(char *prog, int wait, const char *mode,
bool dup_stderr = true);
int close_wpipe(BPIPE *bpipe);
int close_bpipe(BPIPE *bpipe);

Expand Down
2 changes: 1 addition & 1 deletion src/plugins/filed/bpipe-fd.c
View file
Expand Up @@ -391,7 +391,7 @@ static bRC pluginIO(bpContext *ctx, struct io_pkt *io)
free(writer_codes);
}
} else {
p_ctx->pfd = open_bpipe(p_ctx->reader, 0, "r");
p_ctx->pfd = open_bpipe(p_ctx->reader, 0, "r", false);
Dmsg(ctx, dbglvl, "bpipe-fd: IO_OPEN fd=%p reader=%s\n", p_ctx->pfd, p_ctx->reader);
if (!p_ctx->pfd) {
io->io_errno = errno;
Expand Down

0 comments on commit 747d34c

Please sign in to comment.