Possible problems found by static analysis of code of cups-1.6.1 #4242

Closed
michaelrsweet opened this Issue Dec 5, 2012 · 4 comments

Comments

Projects
None yet
2 participants
Collaborator

michaelrsweet commented Dec 5, 2012

Version: 1.6-current
CUPS.org User: jpopelka

Hello,

we analyzed the cups-1.6.1 code with Coverity - tool for
static analysis.

I'm attaching a tarball of patches.
Each patch contains a relevant snippet from analysis log.
I hope the log line numbers are ok, I changed them to sync with svn repo head.

Collaborator

michaelrsweet commented Dec 13, 2012

CUPS.org User: mike

0001: OK
0002: OK
0003: OK
0004: Also the same fix for the mDNSResponder version of http_resolve_cb...
0005: Simpler to compare < 3+sizeof
0006: The values are already type-checked in the big option switch.
0007: This appears to be the correct fix, thanks.
0008: The missing break is on purpose; I've added a comment to this effect...

Final patch attached.

Collaborator

michaelrsweet commented Dec 13, 2012 edited

"str4242.patch":

# Index: backend/usb-libusb.c

--- backend/usb-libusb.c    (revision 10756)
+++ backend/usb-libusb.c    (working copy)
@@ -672,10 +672,10 @@
        \* If it didn't exit abort the pending read and wait an additional
        \* second...

         */

+
       if (!g.read_thread_done)
       {
-   fputs("DEBUG: Read thread still active, aborting the pending read...\n", 
-   fputs("DEBUG: Read thread still active, aborting the pending read...\n",
        stderr);
  
  g.wait_eof = 0;
  @@ -683,7 +683,7 @@
  gettimeofday(&tv, NULL);
  cond_timeout.tv_sec  = tv.tv_sec + 1;
  cond_timeout.tv_nsec = tv.tv_usec \* 1000;
- +
  while (!g.read_thread_done)
  {
    if (pthread_cond_timedwait(&g.read_thread_cond, &g.read_thread_mutex,
  @@ -696,9 +696,6 @@
   pthread_mutex_unlock(&g.read_thread_mutex);
  }
-  if (print_fd)

 \-    close(print_fd);

  /*
- Close the connection and input file and general clean up...
  */
  @@ -758,7 +755,7 @@
     */
     if (printer->origconf > 0 && printer->origconf != number2)
     {
  -   fprintf(stderr, "DEBUG: Restoring USB device configuration: %d -> %d\n", 
  -   fprintf(stderr, "DEBUG: Restoring USB device configuration: %d -> %d\n",
    number2, printer->origconf);
    if ((errcode = libusb_set_configuration(printer->handle,
                    printer->origconf)) < 0)
    @@ -919,7 +916,7 @@
    */
    
    if (((altptr->bInterfaceClass != LIBUSB_CLASS_PRINTER ||
  -         altptr->bInterfaceSubClass != 1) && 
  -         altptr->bInterfaceSubClass != 1) &&
     ((printer.quirks & USBLP_QUIRK_BAD_CLASS) == 0)) ||
    (altptr->bInterfaceProtocol != 1 && /\* Unidirectional _/
     altptr->bInterfaceProtocol != 2) ||    /_ Bidirectional */
    @@ -997,7 +994,7 @@
                    bEndpointAddress;
    }
    else
  -         fprintf(stderr, "DEBUG: Uni-directional USB communication " 
  -         fprintf(stderr, "DEBUG: Uni-directional USB communication "
          "only!\n");
    printer.write_endp = confptr->interface[printer.iface].
                   altsetting[printer.altset].
    @@ -1376,7 +1373,7 @@
  
  printer->origconf = current;
-  if ((errcode = 
-  if ((errcode =
      libusb_get_config_descriptor (printer->device, printer->conf, &confptr))
     < 0)
  {
  @@ -1388,7 +1385,7 @@
  
  if (number1 != current)
  {
-    fprintf(stderr, "DEBUG: Switching USB device configuration: %d -> %d\n", 
-    fprintf(stderr, "DEBUG: Switching USB device configuration: %d -> %d\n",
      current, number1);
   if ((errcode = libusb_set_configuration(printer->handle, number1)) < 0)
   {
  
  # Index: cups/ipp.c
  
  --- cups/ipp.c  (revision 10756)
  +++ cups/ipp.c  (working copy)
  @@ -3239,6 +3239,13 @@
        ipp->prev = ipp->current;
  
  attr = ipp->current = ipp_add_attr(ipp, NULL, ipp->curtag, IPP_TAG_ZERO, 1);

-       if (!attr)
-       {
-         _cupsSetHTTPError(HTTP_ERROR);
-         DEBUG_puts("1ippReadIO: unable to allocate attribute.");
-         _cupsBufferRelease((char *)buffer);
-         return (IPP_ERROR);
- 
    }
  
  DEBUG_printf(("2ippReadIO: membername, ipp->current=%p, ipp->prev=%p",
                ipp->current, ipp->prev));
  
  @@ -6364,6 +6371,7 @@
      _cupsStrFree(attr->values[0].string.language);
      attr->values[0].string.language = NULL;
    }
- 
  /* Fall through to other string values */
  
     case IPP_TAG_TEXT :
     case IPP_TAG_NAME :
  
  # Index: cups/dest-options.c
  
  --- cups/dest-options.c (revision 10756)
  +++ cups/dest-options.c (working copy)
  @@ -485,7 +485,7 @@
   active = NULL;
  }
-  if (tries >= 0)
-  if (tries >= 100)
  {
   DEBUG_puts("1cupsCopyDestConflicts: Unable to resolve after 100 tries.");
   have_conflicts = -1;
  Index: cups/http-support.c
  ===================================================================
  --- cups/http-support.c (revision 10756)
  +++ cups/http-support.c (working copy)
  @@ -2064,6 +2064,8 @@
          error));
  #endif /\* DEBUG */
       }
  +
-      httpAddrFreeList(addrlist);
   }
  }

@@ -2279,6 +2281,8 @@
            error));
 #endif /\* DEBUG */
       }
+
-      httpAddrFreeList(addrlist);
   }
  }

# Index: cups/ipp-support.c

--- cups/ipp-support.c  (revision 10756)
+++ cups/ipp-support.c  (working copy)
@@ -777,8 +777,8 @@

   if (!strcmp(attrname, "document-state") &&
       enumvalue >= 3 &&
-      enumvalue <= (3 + (int)(sizeof(ipp_document_states) /
-                              sizeof(ipp_document_states[0]))))
-      enumvalue < (3 + (int)(sizeof(ipp_document_states) /
-                sizeof(ipp_document_states[0]))))
   return (ipp_document_states[enumvalue - 3]);
  else if (!strcmp(attrname, "finishings") ||
     !strcmp(attrname, "finishings-actual") ||
  @@ -787,8 +787,8 @@
     !strcmp(attrname, "finishings-supported"))
  {
   if (enumvalue >= 3 &&
-        enumvalue <= (3 + (int)(sizeof(ipp_finishings) /
-                                sizeof(ipp_finishings[0]))))
-        enumvalue < (3 + (int)(sizeof(ipp_finishings) /
-                  sizeof(ipp_finishings[0]))))
     return (ipp_finishings[enumvalue - 3]);
   else if (enumvalue >= 0x40000000 &&
            enumvalue <= (0x40000000 + (int)(sizeof(ipp_finishings_vendor) /
  @@ -798,8 +798,8 @@
  else if ((!strcmp(attrname, "job-collation-type") ||
           !strcmp(attrname, "job-collation-type-actual")) &&
          enumvalue >= 3 &&
-           enumvalue <= (3 + (int)(sizeof(ipp_job_collation_types) /
-                                   sizeof(ipp_job_collation_types[0]))))
-           enumvalue < (3 + (int)(sizeof(ipp_job_collation_types) /
-                 sizeof(ipp_job_collation_types[0]))))
   return (ipp_job_collation_types[enumvalue - 3]);
  else if (!strcmp(attrname, "job-state") &&
     enumvalue >= IPP_JOB_PENDING && enumvalue <= IPP_JOB_COMPLETED)
  @@ -811,16 +811,16 @@
           !strcmp(attrname, "orientation-requested-default") ||
           !strcmp(attrname, "orientation-requested-supported")) &&
          enumvalue >= 3 &&
-           enumvalue <= (3 + (int)(sizeof(ipp_orientation_requesteds) /
-                                   sizeof(ipp_orientation_requesteds[0]))))
-           enumvalue < (3 + (int)(sizeof(ipp_orientation_requesteds) /
-                 sizeof(ipp_orientation_requesteds[0]))))
   return (ipp_orientation_requesteds[enumvalue - 3]);
  else if ((!strcmp(attrname, "print-quality") ||
           !strcmp(attrname, "print-quality-actual") ||
           !strcmp(attrname, "print-quality-default") ||
           !strcmp(attrname, "print-quality-supported")) &&
          enumvalue >= 3 &&
-           enumvalue <= (3 + (int)(sizeof(ipp_print_qualities) /
-                                   sizeof(ipp_print_qualities[0]))))
-           enumvalue < (3 + (int)(sizeof(ipp_print_qualities) /
-                 sizeof(ipp_print_qualities[0]))))
   return (ipp_print_qualities[enumvalue - 3]);
  else if (!strcmp(attrname, "printer-state") &&
          enumvalue >= IPP_PRINTER_IDLE && enumvalue <= IPP_PRINTER_STOPPED)
  Index: scheduler/job.c
  ===================================================================
  --- scheduler/job.c (revision 10757)
  +++ scheduler/job.c (working copy)
  @@ -4294,6 +4294,8 @@
  else
    unload_job(job);
     }
-      else
- 
     free(job);
  
   }
  
  cupsDirClose(dir);
  @@ -4385,7 +4387,7 @@
  
  if (!strcmp(name, "time-at-completed"))
  {
-    if (JobHistory < INT_MAX)
-    if (JobHistory < INT_MAX && attr)
     job->history_time = attr->values[0].integer + JobHistory;
   else
     job->history_time = INT_MAX;
  @@ -4393,7 +4395,7 @@
   if (job->history_time < JobHistoryUpdate || !JobHistoryUpdate)
     JobHistoryUpdate = job->history_time;
-    if (JobFiles < INT_MAX)
-    if (JobFiles < INT_MAX && attr)
     job->file_time = attr->values[0].integer + JobFiles;
   else
     job->file_time = INT_MAX;
  
  # Index: scheduler/cupsfilter.c
  
  --- scheduler/cupsfilter.c  (revision 10756)
  +++ scheduler/cupsfilter.c  (working copy)
  @@ -409,6 +409,7 @@
  
  if (srctype)
  {
-   /\* sscanf return value already checked above */
   sscanf(srctype, "%15[^/]/%255s", super, type);
   if ((src = mimeType(mime, super, type)) == NULL)
   {
  @@ -426,6 +427,7 @@
   return (1);
  }
- /\* sscanf return value already checked above */
  sscanf(dsttype, "%15[^/]/%255s", super, type);
  if (!_cups_strcasecmp(super, "printer"))
   dst = printer_type;

michaelrsweet added this to the Stable milestone Mar 17, 2016

Hi, Michael
Do you have str4242 patch file. I find that page http://www.cups.org/strfiles/4242/str4242.patch has been moved.
I need to update these issues. My version is 1.6.1

Collaborator

michaelrsweet commented Feb 13, 2017

Copy the patch from the bug comment above. Github offered no way for us to mass-upload the patch files from the old servers...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment