On Thu, Feb 11, 2016 at 11:34:15AM +0200, Pekka Paalanen wrote: > On Thu, 11 Feb 2016 09:47:29 +0800 > Jonas Ådahl <[email protected]> wrote: > > > On Wed, Feb 10, 2016 at 12:37:43PM -0600, Derek Foreman wrote: > > > On 10/02/16 09:35 AM, Jonas Ådahl wrote: > > > > When we are adding padding bytes making our wl_buffer buffer content 4 > > > > byte aligned, we are just moving the pointer. Since the buffer is > > > > allocated using plain malloc(), this means our padding bytes are > > > > effectively uninitialized data, which could be anything previously > > > > allocated in the server process. As we'll be sharing this buffer > > > > content with arbitrary clients, we are effectively sharing private > > > > memory with every client, and even though a well behaving client will > > > > discard any such memory, a malicious client may not. > > > > > > > > Therefor, to avoid any potential missuse of the uninitialized padding > > > > memory shared between the server and client, initialize the buffer > > > > content to 0, making the padding bytes always 0. > > > > > > Cool - is this the cause of the valgrind complaints I've never bothered > > > to track down? :) > > > > Yes. Should have mentioned that in the commit message, as well as the > > bug (https://bugs.freedesktop.org/show_bug.cgi?id=94071 ) that made me > > actually dig out what memory was uninitialized. I'll add those two > > things to the commit message. > > > > > > > > > Signed-off-by: Jonas Ådahl <[email protected]> > > > > --- > > > > src/connection.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/connection.c b/src/connection.c > > > > index 65b64e9..c0e322f 100644 > > > > --- a/src/connection.c > > > > +++ b/src/connection.c > > > > @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, > > > > struct wl_connection *connection) > > > > return -1; > > > > > > > > buffer_size = buffer_size_for_closure(closure); > > > > - buffer = malloc(buffer_size * sizeof buffer[0]); > > > > + buffer = zalloc(buffer_size * sizeof buffer[0]); > > > > > > Oh, took me a couple of reads to realize the padding is between certain > > > potential members in the closure. (and not trivially at the start or end > > > of the buffer) > > Hi, > > just to see I understood right too, you mean the string and array types > in serialize_closure() which use DIV_ROUNDUP() and nothing else? > > If that is the case, then have my > Reviewed-by: Pekka Paalanen <[email protected]> > > and with that I also nominate this patch to be landed for 1.10: it is > trivial and obviously correct, and if it in the unlikely event causes > any problems, those problems have existed before. It might even be > considered as a security fix by some far stretch.
Yep, good simple fix: Reviewed-by: Bryce Harrington <[email protected]> and landed for 1.10: To ssh://git.freedesktop.org/git/wayland/wayland 1906a90..bf34ac7 master -> master Bryce > > > I guess we could zero just the pad bytes in serialize_closure, I don't > > > know whether that falls under "premature optimization" or not, though. > > > Looks pretty easy to do... > > > > We could do that, but is calloc() really any noticably slower than > > malloc() in the way we are using it? > > I wouldn't think there is any observable performance penalty here. And > I like the simplicity of the patch and the catch-all nature of the fix. > > Thanks, > pq > > > It is fairly easy (I did it at first just to make sure it was those > > bytes only that triggered the warning), but s/malloc/zalloc/ seemed like > > the cleaner solution to me. > > > > > > Jonas > > > > > > > > In any case, > > > Reviewed-by: Derek Foreman <[email protected]> > > > > > > Thanks, > > > Derek > > > > > > > if (buffer == NULL) > > > > return -1; > > > > > > > > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2 > > iQIVAwUBVrxVlyNf5bQRqqqnAQgpyhAAkBMiboYyTiE7MRPuNc3LfDxnfx9NXJdB > SY4miIA4VUu4XsZWSaXE6/48HFnFqlDaNhKuPpHCap8aInMrG0T1ZDw2McYGLM8Y > g3U48ZyQNvzyHE+PzOWcdPfmGsKGkwsmA+Epo16DEAqbbzWV+pu66Ny0gILWHPmX > g7i206GJ3eetYdQPtj0HdxPZe7zFTNMI9dDLhEFPNFDwSm+I+RdJICXoMCFYyL7z > OSXj0X7OWAJk0/oFqfV3DH2xXi6elW629GJmUgNVfF5GqP6sSzaGerccr6sYjXI5 > kTyL6CP1C6kNt+ylL0FG/80Oy3ET7/7+FnUQ/auic9UWGAwRz5hbpdj9rKcmpqf7 > zcz2SmGPGH74O/ZFK5NEkfU9TkDrwFUt/jtZ9Bwak4WyrmG0XhyuTz3vvP6fEBsj > dCzZy/W9LE8Fj5Yp1QM9tLsaITpgMjeHyE3kdcOBMq1Dg5rcKsTCn++YarQ7NY6X > zMF9Dw469GHmJP1+HdDxmDvH5tdWPm0nCXozx/7sYS/So4DElPHALj0QkY0tNI7U > X47SxYrBklJIglxvwdnh8MfmBQN8n5gMK3KjIxHST+iIgK0W9jUnjma5XoKXdpVm > SmK3AsJmCegyXmpUsQjCnS+wlI8Lv6NLI2chxpztVL1uBLKN69Bs7155/LLtQE8x > BX38/Od7YBc= > =YXcL > -----END PGP SIGNATURE----- _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
