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. > > 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; > > > > > >
pgpnMOJKjPqKr.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
