popen requires check for fdopen failure

[rtpbusr_gmail.com]

Summary

The current implementation of popen() assumes fdopen() does not fail. We get into situations where fdopen() actually fails and leads to
problems. Attached in Test Plan, please find a simple test program to create the fdopen() failure condition in popen().

Test Plan

/* p1.c -- test popen() for fdopen failure */

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>

/* SYNOPSIS: ./p1 [-e] 1|2|3 - argument -e injects error opening max. fdopen() */

int main(int argc, const char *argv[])
{

  FILE  *pipe;
  char  line[2048];
  int  func, n = 0;

  if (argc < 2) return 0;

  if (argc > 2 && *argv[1] == '-' && argv[1][1] == 'e') { /* inject error in system */
        int     fd;
        FILE    *iop;

        argv++; argc--;

        for (n = 0; (fd = dup(STDIN_FILENO)) >= 0; n++) {
                        /* open max streams to exhaust open streams */
                if ((iop = fdopen(fd,"r")) == NULL) {
			perror("fdopen ERROR\n");
                        break;
                }
        }
        printf("Total files opened: %d\n",n);
        n = 0;
  }

  func = atoi(argv[1]);

  if (func == 1) {
    puts("func 1; read from pipe");
    fflush(stdout);
    pipe = popen("echo hi there", "r");
    if (pipe == NULL) { puts("popen FAILURE\n"); abort(); }
    setbuf(pipe, NULL);
    while(fgets(line, sizeof(line), pipe) != NULL && strlen(line) > 1) {
      fprintf(stdout, "%3d %s", ++n, line);
      fflush(stdout);
    }  
    pclose(pipe);
  } else if (func == 2) {
    puts("func 2; write to pipe");
    fflush(stdout);
    pipe = popen("echo hi there", "w");
    if (pipe == NULL) { puts("popen FAILURE\n"); abort(); }
    setbuf(pipe, NULL);
    while(fgets(line, sizeof(line), stdin) != NULL && strlen(line) > 1) {
      fprintf(pipe, "%3d %s", ++n, line);
      fflush(pipe);
    }  
    pclose(pipe);
  } else if (func == 3) {
    puts("func 3; read and write pipe");
    fflush(stdout);
    pipe = popen("echo hi there", "r+");
    if (pipe == NULL) { puts("popen FAILURE\n"); abort(); }
    setbuf(pipe, NULL);
    while(fgets(line, sizeof(line), pipe) != NULL && strlen(line) > 1) {
      fprintf(pipe, "%3d %s", ++n, line);
      fflush(pipe);
    }  
    pclose(pipe);
  }  
  return 0;

}

/* p1.c end */

[rtpbusr_gmail.com]

Check is added for fdopen() returning NULL and corresponding error message emitted through perror() followed by abort() exception generation.

[mjg]

Are there any issues with doing fdopen /prior/ to execve? Just aborting is definitely not the way to go here.

[rtpbusr_gmail.com]

Hi mjg,
Here we are hitting the case of resource limits getting exhausted (max file streams opened hits the ceiling). Following are the options we can take to solve the problem:

1. It is conventional practice to generate exception like abort() in software indicating there is something critically wrong so that the issue can be debugged at the earliest.
2. Check for failures, undo things already done in the function path (close files, free memory, etc.,.) and return error. In such cases, the system

may end up with side-effect problems or exit indicating error. It may not be possible to undo all things. This increases complexity.
Option 1 is preferable since it keeps things simple.

Regarding issues of not checking fdopen() returning NULL and adding file stream in pidlist leads to problems, following code segement depicts the problem:

for (p = pidlist; p; p = p->next) {
        (void)_close(fileno(p->fp));
}

Thanks
rtpbusr

[rtpbusr_gmail.com]

Modifications focus on minimum resources and apply to a couple of lines in parent.

[rtpbusr_gmail.com]

popen() attempts to allocate resources (memory, file stream) before child creation.

[rtpbusr_gmail.com]

Hi mjg,
Diff2 ID:10314 and Diff 3 ID:10315 provide alternative solutions to eliminate abort() in popen() along the lines of your thought.

[jilles]

lib/libc/gen/popen.c

128

This comment seems unneeded.

[rpokala-panasas.com]

lib/libc/gen/popen.c

128

I was the one who asked for that - it wasn't clear why one pdes was being _close()d, while the other was being fclose()d.

[rpokala]

This was committed in r291114, and the commit message referenced this review, but this review was not marked as closed by r291114...?

[allanjude]

Committed as rS291114