This problem is related to file descriptor passing in Unix systems. It affects all versions since this functionality was introduced. In source file 'dbus/dbus-message.c', 'load_message' function, if we have more than one file descriptors inside the loader and they are from different messages, when we are extracting them to compose the actual message, a misbehaviour uprises. The problem is with this line (dbus/dbus-message.c:4207) of 1.8.X branch: memmove(loader->unix_fds + n_unix_fds, loader->unix_fds, loader->n_unix_fds); It pretends to move the array of FD's after extracting the current message ones, but the command has the source and destination arguments swapped. Suppose we have two FD's in the loader: (35,36) loader->n_unix_fds = 2 The message we are composing uses one FD (the 35). We assign it to the message, and after executing the previous command, we end with this info in the loader: (35,35) loader->n_unix_fds = 1 We should have ended with: (36, ) loader->n_unix_fds = 1 The problem is that we are duplicating the id of the first file descriptor in the position of the second one. Fix it is simple, we only need to interchange them: memmove(loader->unix_fds, loader->unix_fds + n_unix_fds, loader->n_unix_fds); This patch has worked succesfully for me. I have tested it on a Suse 11, and my problems with duplicated file descriptors (because two different messages where sharing the same file descriptor) are finally gone :).
(In reply to comment #0) > Suppose we have two FD's in the loader: I think this is a potentially dangerous oversimplification, so I'm going to work through the logic with larger numbers. Suppose we have 5 fds: the first message is meant to contain 2 and the second 3: n_unix_fds = 2 message has 0 fds {} loader has 5 fds { 11, 12, 21, 22, 23 } We copy the first 2 fds into the message: n_unix_fds = 2 message has 2 fds { 11, 12 } loader has 5 fds { 11, 12, 21, 22, 23 } and what we want to do is to shift unix_fds[2]..unix_fds[4], inclusive, down to unix_fds[0]..unix_fds[2], inclusive. There are actually *two* bugs here. One is that we're trying to shift [0]..[2] upwards, not the other way round; so you would expect we would get { 11, 12, 21, 11, 12, [heap buffer overflow] }. The other bug, which is what prevents this from being a buffer overflow, is that we're incorrectly copying loader->n_unix_fds *bytes*, not loader->n_unix_fds *ints*. (The buffer overflows if n_unix_fds * 4 + loader->n_unix_fds > max_message_unix_fds * 4 but since we know that n_unix_fds + loader->n_unix_fds <= max_message_unix_fds, that's impossible. Strictly speaking that "4" should be sizeof(int), but the same relationships remain true.) > memmove(loader->unix_fds, loader->unix_fds + n_unix_fds, loader->n_unix_fds); > > This patch has worked succesfully for me. I think that's only because you happen to be on a little-endian platform, fds are small integers (< 256, so the first byte of little-endian representation is enough), and you're only moving one fd.
Created attachment 100875 [details] [review] If the loader contains two messages with fds, don't corrupt the second There were two bugs here: we would previously overwrite the unused fds with the already-used fds instead of the other way round, and we would copy n bytes where we should have copied n ints. Where's GArray when you need it?
(In reply to comment #2) > Created attachment 100875 [details] [review] [review] > If the loader contains two messages with fds, don't corrupt the second > > There were two bugs here: we would previously overwrite the unused > fds with the already-used fds instead of the other way round, and > we would copy n bytes where we should have copied n ints. > > Where's GArray when you need it? The patch looks good to me. I reproduced the bad behaviour when testing your scenario described in comment #1: > Suppose we have 5 fds: the first message is meant to contain 2 and the > second 3: And the issue is gone with your patch. I tested the patch applied on top of the Debian package dbus-1.8.4-1.
Fixed in dbus 1.6.22, 1.8.6. This bug also caused a security vulnerability, although it's a denial of service, not the buffer overflow I was initially worried about. See Bug #80469 for that.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.