Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding
On Thu, Feb 11, 2016 at 11:34:15AM +0200, Pekka Paalanen wrote: > On Thu, 11 Feb 2016 09:47:29 +0800 > Jonas Ådahl 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 > > > > --- > > > > 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? Yes. An alternative solution I tested just for the sake of it was to memset the padding up to the next DIV_ROUNDUP(), and that also fixed the warning, but I as well think a malloc->zalloc was the cleaner approach. Jonas > > If that is the case, then have my > Reviewed-by: Pekka Paalanen > > 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. > > > > 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 > > > > > > Thanks, > > > Derek > > > > > > > if (buffer == NULL) > > > > return -1; > > > > > > > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding
On Thu, Feb 11, 2016 at 11:34:15AM +0200, Pekka Paalanen wrote: > On Thu, 11 Feb 2016 09:47:29 +0800 > Jonas Ådahl 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 > > > > --- > > > > 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 > > 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 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 > > > > > > 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 wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding
On Thu, 11 Feb 2016 09:47:29 +0800 Jonas Ådahl 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 > > > --- > > > 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 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. > > 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 > > > > Thanks, > > Derek > > > > > if (buffer == NULL) > > > return -1; > > > > > > pgpnMOJKjPqKr.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding
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 > > --- > > 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) > > 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? 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 > > Thanks, > Derek > > > if (buffer == NULL) > > return -1; > > > > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding
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? :) > Signed-off-by: Jonas Ådahl > --- > 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) 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... In any case, Reviewed-by: Derek Foreman Thanks, Derek > if (buffer == NULL) > return -1; > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel