dbusbind.c SIGSEGV fix and minor cleanup

dbusbind.c SIGSEGV fix and minor cleanup

[David Hansen]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

Hello,

emacs crashes (in `strcpy') when receiving dbus messages with some empty
content.

The attached patch fixes this.  Also there is no need to copy the
strings.  This is already be done by `build_string'.

David


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2794 bytes --]

*** dbusbind.c.~1.22.~	2008-02-17 01:40:28.000000000 +0100
--- dbusbind.c	2008-03-18 10:35:30.000000000 +0100
***************
*** 1140,1149 ****
    DBusMessageIter iter;
    unsigned int dtype;
    int mtype;
!   char uname[DBUS_MAXIMUM_NAME_LENGTH];
!   char path[DBUS_MAXIMUM_MATCH_RULE_LENGTH]; /* Unlimited in D-Bus spec.  */
!   char interface[DBUS_MAXIMUM_NAME_LENGTH];
!   char member[DBUS_MAXIMUM_NAME_LENGTH];
  
    /* Open a connection to the bus.  */
    connection = xd_initialize (bus);
--- 1140,1146 ----
    DBusMessageIter iter;
    unsigned int dtype;
    int mtype;
!   const char *uname, *path, *interface, *member;
  
    /* Open a connection to the bus.  */
    connection = xd_initialize (bus);
***************
*** 1175,1185 ****
  
    /* Read message type, unique name, object path, interface and member
       from the message.  */
!   mtype =            dbus_message_get_type (dmessage);
!   strcpy (uname,     dbus_message_get_sender (dmessage));
!   strcpy (path,      dbus_message_get_path (dmessage));
!   strcpy (interface, dbus_message_get_interface (dmessage));
!   strcpy (member,    dbus_message_get_member (dmessage));
  
    XD_DEBUG_MESSAGE ("Event received: %d %s %s %s %s %s",
  		    mtype, uname, path, interface, member,
--- 1172,1186 ----
  
    /* Read message type, unique name, object path, interface and member
       from the message.  */
!   mtype     = dbus_message_get_type (dmessage);
!   uname     = dbus_message_get_sender (dmessage);
!   path      = dbus_message_get_path (dmessage);
!   interface = dbus_message_get_interface (dmessage);
!   member    = dbus_message_get_member (dmessage);
! 
!   /* dbus-registered-functions-table requires non nil interface and member. */
!   if ((NULL == interface) || (NULL == member))
!     RETURN_UNGCPRO (Qnil);
  
    XD_DEBUG_MESSAGE ("Event received: %d %s %s %s %s %s",
  		    mtype, uname, path, interface, member,
***************
*** 1210,1220 ****
  			     args);
  
  	  /* Add uname, path, interface and member to the event.  */
! 	  event.arg = Fcons ((member == NULL ? Qnil : build_string (member)),
! 			     event.arg);
! 	  event.arg = Fcons ((interface == NULL
! 			      ? Qnil : build_string (interface)),
! 			     event.arg);
  	  event.arg = Fcons ((path == NULL ? Qnil : build_string (path)),
  			     event.arg);
  	  event.arg = Fcons ((uname == NULL ? Qnil : build_string (uname)),
--- 1211,1218 ----
  			     args);
  
  	  /* Add uname, path, interface and member to the event.  */
! 	  event.arg = Fcons (build_string (member), event.arg);
! 	  event.arg = Fcons (build_string (interface), event.arg);
  	  event.arg = Fcons ((path == NULL ? Qnil : build_string (path)),
  			     event.arg);
  	  event.arg = Fcons ((uname == NULL ? Qnil : build_string (uname)),

Re: dbusbind.c SIGSEGV fix and minor cleanup

[David Hansen]

On Tue, 18 Mar 2008 10:47:11 +0100 David Hansen wrote:

> emacs crashes (in `strcpy') when receiving dbus messages with some empty
> content.

Shall I write a more detailed description of the patch?  I mean,
something should be done, a SIGSEGV is definitively a bug.

David

Re: dbusbind.c SIGSEGV fix and minor cleanup

[Michael Albinus]

David Hansen <david.hansen@gmx.net> writes:

> Hello,

Hi,

> emacs crashes (in `strcpy') when receiving dbus messages with some empty
> content.
>
> The attached patch fixes this.  Also there is no need to copy the
> strings.  This is already be done by `build_string'.

The patch looks OK to me.

One could argue, that in case interface or member is NULL, this could be
regarded as wildcards, and all registered functions which match
otherwise shall be applied. But I agree with you, that only messages,
which are sent to a specified interface and its member, shall be taken.

One minor point: before "RETURN_UNGCPRO (Qnil)" you might call
"dbus_message_unref (dmessage)" for freeing dmessage. Or you simply jump
to the end, where it is done already.

Please commit the patch, or (in case you have no write access in CVS)
send the ChangeLog entry; I'll do it then.

> David

Thanks, and best regards, Michael.

Re: dbusbind.c SIGSEGV fix and minor cleanup

[David Hansen]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On Fri, 21 Mar 2008 17:27:07 +0100 Michael Albinus wrote:

> David Hansen <david.hansen@gmx.net> writes:
>
>> Hello,
>
> Hi,
>
>> emacs crashes (in `strcpy') when receiving dbus messages with some empty
>> content.
>>
>> The attached patch fixes this.  Also there is no need to copy the
>> strings.  This is already be done by `build_string'.
>
> One could argue, that in case interface or member is NULL, this could be
> regarded as wildcards, and all registered functions which match

At least with my dbus installation there is still an error coming to the actual
connection.  This empty message is hard to reproduce as it seem to happen more
or less randomly.  In case you wonder how I found out:

I debugged a program with gud that crashed after receiving a dbus message from
the same emacs process.  In some cases emacs just died with it.

> One minor point: before "RETURN_UNGCPRO (Qnil)" you might call
> "dbus_message_unref (dmessage)" for freeing dmessage. Or you simply jump
> to the end, where it is done.

Thanks for reviewing and pointing that out.

> Please commit the patch, or (in case you have no write access in CVS)
> send the ChangeLog entry; I'll do it then.

2008-03-22  David Hansen  <david.hansen@gmx.net>

dbusbind.c: (xd_read_message): Removed extra copying of message strings.  Check
for NULL `interface' or `member'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3091 bytes --]

*** dbusbind.c.~1.22.~	2008-02-17 01:40:28.000000000 +0100
--- dbusbind.c	2008-03-22 09:12:04.000000000 +0100
***************
*** 1140,1149 ****
    DBusMessageIter iter;
    unsigned int dtype;
    int mtype;
!   char uname[DBUS_MAXIMUM_NAME_LENGTH];
!   char path[DBUS_MAXIMUM_MATCH_RULE_LENGTH]; /* Unlimited in D-Bus spec.  */
!   char interface[DBUS_MAXIMUM_NAME_LENGTH];
!   char member[DBUS_MAXIMUM_NAME_LENGTH];
  
    /* Open a connection to the bus.  */
    connection = xd_initialize (bus);
--- 1140,1146 ----
    DBusMessageIter iter;
    unsigned int dtype;
    int mtype;
!   const char *uname, *path, *interface, *member;
  
    /* Open a connection to the bus.  */
    connection = xd_initialize (bus);
***************
*** 1175,1185 ****
  
    /* Read message type, unique name, object path, interface and member
       from the message.  */
!   mtype =            dbus_message_get_type (dmessage);
!   strcpy (uname,     dbus_message_get_sender (dmessage));
!   strcpy (path,      dbus_message_get_path (dmessage));
!   strcpy (interface, dbus_message_get_interface (dmessage));
!   strcpy (member,    dbus_message_get_member (dmessage));
  
    XD_DEBUG_MESSAGE ("Event received: %d %s %s %s %s %s",
  		    mtype, uname, path, interface, member,
--- 1172,1186 ----
  
    /* Read message type, unique name, object path, interface and member
       from the message.  */
!   mtype     = dbus_message_get_type (dmessage);
!   uname     = dbus_message_get_sender (dmessage);
!   path      = dbus_message_get_path (dmessage);
!   interface = dbus_message_get_interface (dmessage);
!   member    = dbus_message_get_member (dmessage);
! 
!   /* dbus-registered-functions-table requires non nil interface and member. */
!   if ((NULL == interface) || (NULL == member))
!     goto cleanup;
  
    XD_DEBUG_MESSAGE ("Event received: %d %s %s %s %s %s",
  		    mtype, uname, path, interface, member,
***************
*** 1210,1220 ****
  			     args);
  
  	  /* Add uname, path, interface and member to the event.  */
! 	  event.arg = Fcons ((member == NULL ? Qnil : build_string (member)),
! 			     event.arg);
! 	  event.arg = Fcons ((interface == NULL
! 			      ? Qnil : build_string (interface)),
! 			     event.arg);
  	  event.arg = Fcons ((path == NULL ? Qnil : build_string (path)),
  			     event.arg);
  	  event.arg = Fcons ((uname == NULL ? Qnil : build_string (uname)),
--- 1211,1218 ----
  			     args);
  
  	  /* Add uname, path, interface and member to the event.  */
! 	  event.arg = Fcons (build_string (member), event.arg);
! 	  event.arg = Fcons (build_string (interface), event.arg);
  	  event.arg = Fcons ((path == NULL ? Qnil : build_string (path)),
  			     event.arg);
  	  event.arg = Fcons ((uname == NULL ? Qnil : build_string (uname)),
***************
*** 1235,1241 ****
       value = CDR_SAFE (value);
      }
  
!   /* Cleanup.  */
    dbus_message_unref (dmessage);
    RETURN_UNGCPRO (Qnil);
  }
--- 1233,1239 ----
       value = CDR_SAFE (value);
      }
  
!  cleanup:
    dbus_message_unref (dmessage);
    RETURN_UNGCPRO (Qnil);
  }

Re: dbusbind.c SIGSEGV fix and minor cleanup

[Michael Albinus]

David Hansen <david.hansen@gmx.net> writes:

> At least with my dbus installation there is still an error coming to the actual
> connection.  This empty message is hard to reproduce as it seem to happen more
> or less randomly.  In case you wonder how I found out:
>
> I debugged a program with gud that crashed after receiving a dbus message from
> the same emacs process.  In some cases emacs just died with it.

Could you, please, show the message it did receive when crashing? You
could simplify the test by enabling traces in dbusbind.c. When you
compile Emacs with "make MYCPPFLAGS='-DDBUS_DEBUG'", all traces in
XD_DEBUG_MESSAGE are written to STDOUT.

Best regards, Michael.