New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fd handling issues in start_job... #1559

Closed
michaelrsweet opened this Issue Apr 12, 2006 · 3 comments

Comments

Projects
None yet
1 participant
@michaelrsweet
Collaborator

michaelrsweet commented Apr 12, 2006

Version: 1.2-current
CUPS.org User: jlovell

There seems to be two fd handling issues in start_job:

  • status fds need to be kept in cupsd_job_t since they are reused in multi-document jobs (and 'statusfds[1]' should only be closed after starting the last document).
  • 'filterfds[slot]' should only be closed after starting the last document since they are copies of 'job->print_pipes'

These bugs manifest themselves by eventually causing select() to return EBADF.

This code is slightly tricky... I believe the attached patch is correct but please check.

Thanks!

@michaelrsweet

This comment has been minimized.

Collaborator

michaelrsweet commented Apr 14, 2006

CUPS.org User: mike

Looks pretty good, although we need to FD_CLR the status FD before calling cupsdClosePipe(), since that function resets the file descriptors to -1...

Also, the change introduced a file descriptor leak, which I'm tracking down now...

@michaelrsweet

This comment has been minimized.

Collaborator

michaelrsweet commented Apr 14, 2006

CUPS.org User: mike

Fixed in Subversion repository.

@michaelrsweet

This comment has been minimized.

Collaborator

michaelrsweet commented Apr 14, 2006

"start_job.patch":

Index: scheduler/job.c

--- scheduler/job.c (revision 5390)
+++ scheduler/job.c (working copy)
@@ -127,6 +127,8 @@
job->back_pipes[1] = -1;
job->print_pipes[0] = -1;
job->print_pipes[1] = -1;

  • job->status_pipes[0]= -1;
  • job->status_pipes[1]= -1;

cupsdSetString(&job->dest, dest);

@@ -442,15 +444,17 @@
*/

 cupsdLogMessage(CUPSD_LOG_DEBUG2,
  •       "cupsdFinishJob: Closing status pipes [ %d %d ]...",
    
  •       job->status_pipes[0], job->status_pipes[1]);
    
  • cupsdClosePipe(job->status_pipes);
  • cupsdLogMessage(CUPSD_LOG_DEBUG2,
    "cupsdFinishJob: Removing fd %d from InputSet...",
    job->status_buffer->fd);

FD_CLR(job->status_buffer->fd, InputSet);

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,
  •                "cupsdFinishJob: Closing status input pipe %d...",
    

- job->status_buffer->fd);

 cupsdStatBufDelete(job->status_buffer);

 job->status_buffer = NULL;

@@ -1532,6 +1536,12 @@

cupsdClosePipe(job->back_pipes);

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,
  •              "cupsdStopJob: Closing status pipes [ %d %d ]...",
    
  •              job->status_pipes[0], job->status_pipes[1]);
    
  • cupsdClosePipe(job->status_pipes);

if (job->status_buffer)
{
/*
@@ -1544,10 +1554,6 @@

FD_CLR(job->status_buffer->fd, InputSet);

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,
  •                "cupsdStopJob: Closing status input pipe %d...",
    

- job->status_buffer->fd);

 cupsdStatBufDelete(job->status_buffer);

 job->status_buffer = NULL;

@@ -1979,6 +1985,8 @@
job->back_pipes[1] = -1;
job->print_pipes[0] = -1;
job->print_pipes[1] = -1;

  •  job->status_pipes[0]= -1;
    
  •  job->status_pipes[1]= -1;
    

    cupsdLogMessage(CUPSD_LOG_DEBUG, "Loading job %d from cache...", job->id);
    }
    @@ -2227,6 +2235,8 @@
    job->back_pipes[1] = -1;
    job->print_pipes[0] = -1;
    job->print_pipes[1] = -1;

  •  job->status_pipes[0]= -1;
    
  •  job->status_pipes[1]= -1;
    

    if (job->id >= NextJobId)
    NextJobId = job->id + 1;
    @@ -2344,8 +2354,7 @@
    int backroot; /* Run backend as root? /
    int pid; /
    Process ID of new filter process /
    int banner_page; /
    1 if banner page, 0 otherwise */

  • int statusfds[2], /* Pipes used with the scheduler */

  •       filterfds[2][2];/\* Pipes used between filters */
    
  • int filterfds[2][2];/* Pipes used between filters /
    int envc; /
    Number of environment variables _/
    char *_argv, /* Filter command-line arguments /
    sani_uri[1024], /
    Sanitized DEVICE_URI env var */
    @@ -2992,32 +3001,31 @@
    filterfds[1][0] = -1;
    filterfds[1][1] = -1;

  • if (cupsdOpenPipe(statusfds))

  • if (job->status_pipes[0] == -1)
    {

  • cupsdLogMessage(CUPSD_LOG_ERROR, "Unable to create job status pipes - %s.",

  •           strerror(errno));
    
  • snprintf(printer->state_message, sizeof(printer->state_message),

- "Unable to create status pipes - %s.", strerror(errno));

- cupsdAddPrinterHistory(printer);

  • cupsdAddEvent(CUPSD_EVENT_JOB_COMPLETED, job->printer, job,
  •              "Job canceled because the server could not create the job "
    

- "status pipes.");

  • goto abort_job;

  • if (cupsdOpenPipe(job->status_pipes))

  • {

  •  cupsdLogMessage(CUPSD_LOG_ERROR, "Unable to create job status pipes - %s.",
    
  •         strerror(errno));
    
  •  snprintf(printer->state_message, sizeof(printer->state_message),
    
  •      "Unable to create status pipes - %s.", strerror(errno));
    
  •  cupsdAddPrinterHistory(printer);
    
  •  cupsdAddEvent(CUPSD_EVENT_JOB_COMPLETED, job->printer, job,
    
  •       "Job canceled because the server could not create the job "
    
  •       "status pipes.");
    
  •  goto abort_job;
    
  • }

  • cupsdLogMessage(CUPSD_LOG_DEBUG2, "start_job: status_pipes = [ %d %d ]",

  •       job->status_pipes[0], job->status_pipes[1]);
    
  • job->status_buffer = cupsdStatBufNew(job->status_pipes[0], "[Job %d]",

  •                    job->id);
    

    }

  • cupsdLogMessage(CUPSD_LOG_DEBUG2, "start_job: statusfds = [ %d %d ]",

- statusfds[0], statusfds[1]);

-#ifdef FD_CLOEXEC

  • fcntl(statusfds[0], F_SETFD, FD_CLOEXEC);
  • fcntl(statusfds[1], F_SETFD, FD_CLOEXEC);
    -#endif /* FD_CLOEXEC */
  • job->status_buffer = cupsdStatBufNew(statusfds[0], "[Job %d]",
  •                                       job->id);
    
    job->status = 0;
    memset(job->filters, 0, sizeof(job->filters));

@@ -3144,7 +3152,7 @@
slot, filterfds[slot][0], filterfds[slot][1]);

 pid = cupsdStartProcess(command, argv, envp, filterfds[!slot][0],
  •                        filterfds[slot][1], statusfds[1],
    
  •                        filterfds[slot][1], job->status_pipes[1],
                job->back_pipes[0], 0, job->filters + i);
    

    cupsdLogMessage(CUPSD_LOG_DEBUG2,
    @@ -3232,7 +3240,7 @@
    slot, filterfds[slot][0], filterfds[slot][1]);

    pid = cupsdStartProcess(command, argv, envp, filterfds[!slot][0],

  •             filterfds[slot][1], statusfds[1],
    
  •             filterfds[slot][1], job->status_pipes[1],
              job->back_pipes[1], backroot,
              &(job->backend));
    

@@ -3271,6 +3279,13 @@
job->back_pipes[0], job->back_pipes[1]);

   cupsdClosePipe(job->back_pipes);
  •  cupsdLogMessage(CUPSD_LOG_DEBUG2,
    
  •         "start_job: Closing status output pipe %d...",
    
  •         job->status_pipes[1]);
    
  •  close(job->status_pipes[1]);
    
  •  job->status_pipes[1] = -1;
    

    }
    }
    else
    @@ -3278,13 +3293,6 @@
    filterfds[slot][0] = -1;
    filterfds[slot][1] = -1;

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,

  •                "start_job: Closing filter pipes for slot %d "
    
  •       "[ %d %d ]...",
    

- !slot, filterfds[!slot][0], filterfds[!slot][1]);

- cupsdClosePipe(filterfds[!slot]);

 if (job->current_file == job->num_files)
 {
   cupsdLogMessage(CUPSD_LOG_DEBUG2,

@@ -3292,17 +3300,21 @@
job->print_pipes[0], job->print_pipes[1]);

   cupsdClosePipe(job->print_pipes);
  •  cupsdLogMessage(CUPSD_LOG_DEBUG2,
    
  •         "start_job: Closing status output pipe %d...",
    
  •         job->status_pipes[1]);
    
  •  close(job->status_pipes[1]);
    
  •  job->status_pipes[1] = -1;
    

    }
    }

  • for (slot = 0; slot < 2; slot ++)

  • {

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,

  •                "start_job: Closing filter pipes for slot %d "
    
  •       "[ %d %d ]...",
    
  •                slot, filterfds[slot][0], filterfds[slot][1]);
    
  • cupsdClosePipe(filterfds[slot]);

  • }

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,

  •     "start_job: Closing filter pipes for slot %d "
    
  •     "[ %d %d ]...",
    
  •     slot, filterfds[slot][0], filterfds[slot][1]);
    
  • cupsdClosePipe(filterfds[slot]);

if (remote_job)
{
@@ -3313,12 +3325,6 @@
free(argv);

cupsdLogMessage(CUPSD_LOG_DEBUG2,

  •              "start_job: Closing status output pipe %d...",
    

- statusfds[1]);

- close(statusfds[1]);

  • cupsdLogMessage(CUPSD_LOG_DEBUG2,
    "start_job: Adding fd %d to InputSet...",
    job->status_buffer->fd);

@@ -3348,8 +3354,8 @@

cupsdLogMessage(CUPSD_LOG_DEBUG2,
"start_job: Closing status pipes [ %d %d ]...",

  •              statusfds[0], statusfds[1]);
    
  • cupsdClosePipe(statusfds);
  •              job->status_pipes[0], job->status_pipes[1]);
    
  • cupsdClosePipe(job->status_pipes);

cupsArrayDelete(filters);

Index: scheduler/job.h

--- scheduler/job.h (revision 5390)
+++ scheduler/job.h (working copy)
@@ -46,7 +46,8 @@
ipp_t attrs; / Job attributes /
cupsd_statbuf_t *status_buffer; /
Status buffer for this job /
int print_pipes[2], /
Print data pipes */

  •       back_pipes[2];  /\* Backchannel pipes */
    
  •       back_pipes[2],  /\* Backchannel pipes */
    
  •       status_pipes[2];/\* Status pipes _/
    
    int cost; /_ Filtering cost /
    int filters[MAX_FILTERS + 1];
    /
    Filter process IDs, 0 terminated */
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment