Re: [LOW] STR #4242: Possible problems found by static analysis of code of cups-1.6.1

Michael Sweet <[email protected]> Thu, 13 Dec 2012 13:08:39 -0800 (PST)
Newsgroups gmane.comp.printing.cups.bugs
Message-ID <[email protected]>
DO NOT REPLY TO THIS MESSAGE.  INSTEAD, POST ANY RESPONSES TO THE LINK BELOW.

[STR Closed w/Resolution]

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.

Link: https://www.cups.org/str.php?L4242
Version: 1.6-current
Fix Version: 1.7-current (r10766)

_______________________________________________
cups-bugs mailing list
[email protected]
http://lists.easysw.com/mailman/listinfo/cups-bugs
str4242.patch (text/plain, 8.3 KB)
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;