GNU bug report logs - #8722
dbusbind.c fixes for integer issues, e.g., integer overflow

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8722 in the body.
You can then email your comments to 8722 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: dbusbind.c fixes for integer issues, e.g., integer overflow
Date: Mon, 23 May 2011 22:32:19 -0700

I found some integer overflow issues in dbusbind.c and came up
with the following patch which I'd like to install.  This doesn't
fix all the integer overflow problems in dbusbind.c, but it makes
some progress anyway.

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-05-24 01:20:04 +0000
+++ src/ChangeLog	2011-05-24 05:16:14 +0000
@@ -1,5 +1,23 @@
 2011-05-24  Paul Eggert  <eggert <at> cs.ucla.edu>
 
+	* dbusbind.c: Serial number integer overflow fixes.
+	(CHECK_DBUS_SERIAL_GET_SERIAL): New macro.
+	(xd_invalid_serial): New static function.
+	(Fdbus_call_method_asynchronously, xd_read_message_1): Use a float
+	to hold a serial number that is too large for a fixnum.
+	(Fdbus_method_return_internal, Fdbus_method_error_internal):
+	Check for serial numbers out of range.  Decode any serial number
+	that was so large that it became a float.
+
+	* dbusbind.c: Use XFASTINT rather than XUINT, and check for nonneg.
+	(Fdbus_call_method, Fdbus_call_method_asynchronously):
+	Use XFASTINT rather than XUINT when numbers are nonnegative.
+	(xd_append_arg, Fdbus_method_return_internal):
+	(Fdbus_method_error_internal): Likewise.  Also, for unsigned
+	arguments, check that Lisp number is nonnegative, rather than
+	silently wrapping negative numbers around.
+	(xd_read_message_1): Don't assume dbus_uint32_t can fit in int.
+
 	* data.c (arith_driver, Flsh): Avoid unnecessary casts to EMACS_UINT.
 
 2011-05-23  Paul Eggert  <eggert <at> cs.ucla.edu>

=== modified file 'src/dbusbind.c'
--- src/dbusbind.c	2011-05-06 22:12:31 +0000
+++ src/dbusbind.c	2011-05-24 05:16:14 +0000
@@ -242,6 +242,31 @@
 #define XD_NEXT_VALUE(object)						\
   ((XD_DBUS_TYPE_P (CAR_SAFE (object))) ? CDR_SAFE (object) : object)
 
+/* Check whether X is a valid dbus serial number.  If valid, set
+   SERIAL to its value.  Otherwise, signal an error. */
+#define CHECK_DBUS_SERIAL_GET_SERIAL(x, serial)				\
+  do									\
+    {									\
+      dbus_uint32_t DBUS_SERIAL_MAX = -1;				\
+      if (NATNUMP (x) && XINT (x) <= DBUS_SERIAL_MAX)			\
+	serial = XINT (x);						\
+      else if (MOST_POSITIVE_FIXNUM < DBUS_SERIAL_MAX			\
+	       && FLOATP (x)						\
+	       && 0 <= XFLOAT_DATA (x)					\
+	       && XFLOAT_DATA (x) <= DBUS_SERIAL_MAX)			\
+	serial = XFLOAT_DATA (x);					\
+      else								\
+	xd_invalid_serial (x);						\
+    }									\
+  while (0)
+
+static void xd_invalid_serial (Lisp_Object) NO_RETURN;
+static void
+xd_invalid_serial (Lisp_Object x)
+{
+  signal_error ("Invalid dbus serial", x);
+}
+
 /* Compute SIGNATURE of OBJECT.  It must have a form that it can be
    used in dbus_message_iter_open_container.  DTYPE is the DBusType
    the object is related to.  It is passed as argument, because it
@@ -431,9 +456,9 @@
     switch (dtype)
       {
       case DBUS_TYPE_BYTE:
-	CHECK_NUMBER (object);
+	CHECK_NATNUM (object);
 	{
-	  unsigned char val = XUINT (object) & 0xFF;
+	  unsigned char val = XFASTINT (object) & 0xFF;
 	  XD_DEBUG_MESSAGE ("%c %d", dtype, val);
 	  if (!dbus_message_iter_append_basic (iter, dtype, &val))
 	    XD_SIGNAL2 (build_string ("Unable to append argument"), object);
@@ -460,9 +485,9 @@
 	}
 
       case DBUS_TYPE_UINT16:
-	CHECK_NUMBER (object);
+	CHECK_NATNUM (object);
 	{
-	  dbus_uint16_t val = XUINT (object);
+	  dbus_uint16_t val = XFASTINT (object);
 	  XD_DEBUG_MESSAGE ("%c %u", dtype, (unsigned int) val);
 	  if (!dbus_message_iter_append_basic (iter, dtype, &val))
 	    XD_SIGNAL2 (build_string ("Unable to append argument"), object);
@@ -483,9 +508,9 @@
 #ifdef DBUS_TYPE_UNIX_FD
       case DBUS_TYPE_UNIX_FD:
 #endif
-	CHECK_NUMBER (object);
+	CHECK_NATNUM (object);
 	{
-	  dbus_uint32_t val = XUINT (object);
+	  dbus_uint32_t val = XFASTINT (object);
 	  XD_DEBUG_MESSAGE ("%c %u", dtype, val);
 	  if (!dbus_message_iter_append_basic (iter, dtype, &val))
 	    XD_SIGNAL2 (build_string ("Unable to append argument"), object);
@@ -503,10 +528,10 @@
 	}
 
       case DBUS_TYPE_UINT64:
-	CHECK_NUMBER (object);
+	CHECK_NATNUM (object);
 	{
-	  dbus_uint64_t val = XUINT (object);
-	  XD_DEBUG_MESSAGE ("%c %"pI"u", dtype, XUINT (object));
+	  dbus_uint64_t val = XFASTINT (object);
+	  XD_DEBUG_MESSAGE ("%c %"pI"d", dtype, XFASTINT (object));
 	  if (!dbus_message_iter_append_basic (iter, dtype, &val))
 	    XD_SIGNAL2 (build_string ("Unable to append argument"), object);
 	  return;
@@ -1110,7 +1135,7 @@
   if ((i+2 <= nargs) && (EQ ((args[i]), QCdbus_timeout)))
     {
       CHECK_NATNUM (args[i+1]);
-      timeout = XUINT (args[i+1]);
+      timeout = XFASTINT (args[i+1]);
       i = i+2;
     }
 
@@ -1186,7 +1211,7 @@
 
   /* Return the result.  If there is only one single Lisp object,
      return it as-it-is, otherwise return the reversed list.  */
-  if (XUINT (Flength (result)) == 1)
+  if (XFASTINT (Flength (result)) == 1)
     RETURN_UNGCPRO (CAR_SAFE (result));
   else
     RETURN_UNGCPRO (Fnreverse (result));
@@ -1251,6 +1276,7 @@
   DBusMessage *dmessage;
   DBusMessageIter iter;
   unsigned int dtype;
+  dbus_uint32_t serial;
   int timeout = -1;
   size_t i = 6;
   char signature[DBUS_MAXIMUM_SIGNATURE_LENGTH];
@@ -1292,7 +1318,7 @@
   if ((i+2 <= nargs) && (EQ ((args[i]), QCdbus_timeout)))
     {
       CHECK_NATNUM (args[i+1]);
-      timeout = XUINT (args[i+1]);
+      timeout = XFASTINT (args[i+1]);
       i = i+2;
     }
 
@@ -1335,7 +1361,8 @@
 	XD_SIGNAL1 (build_string ("Cannot send message"));
 
       /* The result is the key in Vdbus_registered_objects_table.  */
-      result = (list2 (bus, make_number (dbus_message_get_serial (dmessage))));
+      serial = dbus_message_get_serial (dmessage);
+      result = list2 (bus, make_fixnum_or_float (serial));
 
       /* Create a hash table entry.  */
       Fputhash (result, handler, Vdbus_registered_objects_table);
@@ -1368,25 +1395,26 @@
 usage: (dbus-method-return-internal BUS SERIAL SERVICE &rest ARGS)  */)
   (size_t nargs, register Lisp_Object *args)
 {
-  Lisp_Object bus, serial, service;
-  struct gcpro gcpro1, gcpro2, gcpro3;
+  Lisp_Object bus, service;
+  struct gcpro gcpro1, gcpro2;
   DBusConnection *connection;
   DBusMessage *dmessage;
   DBusMessageIter iter;
-  unsigned int dtype;
+  dbus_uint32_t serial;
+  unsigned int ui_serial, dtype;
   size_t i;
   char signature[DBUS_MAXIMUM_SIGNATURE_LENGTH];
 
   /* Check parameters.  */
   bus = args[0];
-  serial = args[1];
   service = args[2];
 
-  CHECK_NUMBER (serial);
+  CHECK_DBUS_SERIAL_GET_SERIAL (args[1], serial);
   CHECK_STRING (service);
-  GCPRO3 (bus, serial, service);
+  GCPRO2 (bus, service);
 
-  XD_DEBUG_MESSAGE ("%"pI"u %s ", XUINT (serial), SDATA (service));
+  ui_serial = serial;
+  XD_DEBUG_MESSAGE ("%u %s ", ui_serial, SSDATA (service));
 
   /* Open a connection to the bus.  */
   connection = xd_initialize (bus, TRUE);
@@ -1394,7 +1422,7 @@
   /* Create the message.  */
   dmessage = dbus_message_new (DBUS_MESSAGE_TYPE_METHOD_RETURN);
   if ((dmessage == NULL)
-      || (!dbus_message_set_reply_serial (dmessage, XUINT (serial)))
+      || (!dbus_message_set_reply_serial (dmessage, serial))
       || (!dbus_message_set_destination (dmessage, SSDATA (service))))
     {
       UNGCPRO;
@@ -1456,25 +1484,26 @@
 usage: (dbus-method-error-internal BUS SERIAL SERVICE &rest ARGS)  */)
   (size_t nargs, register Lisp_Object *args)
 {
-  Lisp_Object bus, serial, service;
-  struct gcpro gcpro1, gcpro2, gcpro3;
+  Lisp_Object bus, service;
+  struct gcpro gcpro1, gcpro2;
   DBusConnection *connection;
   DBusMessage *dmessage;
   DBusMessageIter iter;
-  unsigned int dtype;
+  dbus_uint32_t serial;
+  unsigned int ui_serial, dtype;
   size_t i;
   char signature[DBUS_MAXIMUM_SIGNATURE_LENGTH];
 
   /* Check parameters.  */
   bus = args[0];
-  serial = args[1];
   service = args[2];
 
-  CHECK_NUMBER (serial);
+  CHECK_DBUS_SERIAL_GET_SERIAL (args[1], serial);
   CHECK_STRING (service);
-  GCPRO3 (bus, serial, service);
+  GCPRO2 (bus, service);
 
-  XD_DEBUG_MESSAGE ("%"pI"u %s ", XUINT (serial), SDATA (service));
+  ui_serial = serial;
+  XD_DEBUG_MESSAGE ("%u %s ", ui_serial, SSDATA (service));
 
   /* Open a connection to the bus.  */
   connection = xd_initialize (bus, TRUE);
@@ -1483,7 +1512,7 @@
   dmessage = dbus_message_new (DBUS_MESSAGE_TYPE_ERROR);
   if ((dmessage == NULL)
       || (!dbus_message_set_error_name (dmessage, DBUS_ERROR_FAILED))
-      || (!dbus_message_set_reply_serial (dmessage, XUINT (serial)))
+      || (!dbus_message_set_reply_serial (dmessage, serial))
       || (!dbus_message_set_destination (dmessage, SSDATA (service))))
     {
       UNGCPRO;
@@ -1663,7 +1692,9 @@
   DBusMessage *dmessage;
   DBusMessageIter iter;
   unsigned int dtype;
-  int mtype, serial;
+  int mtype;
+  dbus_uint32_t serial;
+  unsigned int ui_serial;
   const char *uname, *path, *interface, *member;
 
   dmessage = dbus_connection_pop_message (connection);
@@ -1692,7 +1723,7 @@
   /* Read message type, message serial, unique name, object path,
      interface and member from the message.  */
   mtype = dbus_message_get_type (dmessage);
-  serial =
+  ui_serial = serial =
     ((mtype == DBUS_MESSAGE_TYPE_METHOD_RETURN)
      || (mtype == DBUS_MESSAGE_TYPE_ERROR))
     ? dbus_message_get_reply_serial (dmessage)
@@ -1702,7 +1733,7 @@
   interface = dbus_message_get_interface (dmessage);
   member = dbus_message_get_member (dmessage);
 
-  XD_DEBUG_MESSAGE ("Event received: %s %d %s %s %s %s %s",
+  XD_DEBUG_MESSAGE ("Event received: %s %u %s %s %s %s %s",
 		    (mtype == DBUS_MESSAGE_TYPE_INVALID)
 		    ? "DBUS_MESSAGE_TYPE_INVALID"
 		    : (mtype == DBUS_MESSAGE_TYPE_METHOD_CALL)
@@ -1712,14 +1743,14 @@
 		    : (mtype == DBUS_MESSAGE_TYPE_ERROR)
 		    ? "DBUS_MESSAGE_TYPE_ERROR"
 		    : "DBUS_MESSAGE_TYPE_SIGNAL",
-		    serial, uname, path, interface, member,
+		    ui_serial, uname, path, interface, member,
 		    SDATA (format2 ("%s", args, Qnil)));
 
   if ((mtype == DBUS_MESSAGE_TYPE_METHOD_RETURN)
       || (mtype == DBUS_MESSAGE_TYPE_ERROR))
     {
       /* Search for a registered function of the message.  */
-      key = list2 (bus, make_number (serial));
+      key = list2 (bus, make_fixnum_or_float (serial));
       value = Fgethash (key, Vdbus_registered_objects_table, Qnil);
 
       /* There shall be exactly one entry.  Construct an event.  */
@@ -1785,7 +1816,7 @@
 		     event.arg);
   event.arg = Fcons ((uname == NULL ? Qnil : build_string (uname)),
 		     event.arg);
-  event.arg = Fcons (make_number (serial), event.arg);
+  event.arg = Fcons (make_fixnum_or_float (serial), event.arg);
   event.arg = Fcons (make_number (mtype), event.arg);
 
   /* Add the bus symbol to the event.  */

Message #8 received at 8722 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8722 <at> debbugs.gnu.org
Subject: Re: bug#8722: dbusbind.c fixes for integer issues, e.g.,
	integer overflow
Date: Tue, 24 May 2011 09:04:49 +0200

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> I found some integer overflow issues in dbusbind.c and came up
> with the following patch which I'd like to install.  This doesn't
> fix all the integer overflow problems in dbusbind.c, but it makes
> some progress anyway.

On a raw look, it seems to be OK. Minor remark: Why do you need
xd_invalid_serial? You could call instead

  XD_SIGNAL2 (build_string ("Invalid dbus serial"), x);

which returns a `dbus-error', and which is more sensible when reading
DBus events.

Best regards, Michael.

Message #11 received at 8722 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 8722 <at> debbugs.gnu.org
Subject: Re: bug#8722: dbusbind.c fixes for integer issues, e.g., integer
	overflow
Date: Tue, 24 May 2011 00:42:22 -0700

On 05/24/11 00:04, Michael Albinus wrote:
> Why do you need xd_invalid_serial? You could call instead
> 
>   XD_SIGNAL2 (build_string ("Invalid dbus serial"), x);

Thanks, I didn't think of that.  I'll make that change.

Message #16 received at 8722-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 8722-done <at> debbugs.gnu.org, 8719-done <at> debbugs.gnu.org, 
	8668-done <at> debbugs.gnu.org
Subject: fixes committed to trunk
Date: Fri, 27 May 2011 13:40:10 -0700

I just committed bzr 104390 to the Emacs trunk.
It merges fixes for bugs 8668, 8719, and 8722 as
previously discussed.

For Bug#8719, although the patch should fix the problems
it may not be the best patch.  Kenichi Handa wrote that
he'll check it.  Kenichi, if there are problems with it,
please feel free to replace it with something better
or to send me email with suggestions and I'll work on
making it better.  Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.