Bug 94071

Summary:	Two subsequent display roundtrips results sendmsg syscall with uninitialized bytes
Product:	Wayland	Reporter:	Jon <eyolfson>
Component:	wayland	Assignee:	Wayland bug list <wayland-bugs>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	jadahl
Version:	unspecified
Hardware:	x86-64 (AMD64)
OS:	Linux (All)
Whiteboard:
i915 platform:		i915 features:
Attachments:	The bug triggers on the second roundtrip call.

Description Jon 2016-02-10 02:24:51 UTC

Created attachment 121636 [details]
The bug triggers on the second roundtrip call.

This occurs in wayland version 1.9.0 (not selectable in Bugzilla).

I'm on Linux 4.4.1 using gcc 5.3.0 and valgrind 3.11.0.

I can compile the attachment like:

> gcc wayland-test.c -lwayland-client -g

Then run valgrind on the resulting executable:

> valgrind --track-origins=yes ./a.out 
==14980== Memcheck, a memory error detector
==14980== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==14980== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==14980== Command: ./a.out
==14980== 
==14980== Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
==14980==    at 0x512C1E0: __sendmsg_nocancel (in /usr/lib/libc-2.22.so)
==14980==    by 0x4E3AF30: ??? (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E3A738: wl_display_dispatch_queue (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E3AA6E: wl_display_roundtrip_queue (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x400B6A: main (wayland-test.c:47)
==14980==  Address 0x5d1324e is 4,158 bytes inside a block of size 16,424 alloc'd
==14980==    at 0x4C2A987: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14980==    by 0x4E3B061: ??? (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E3A246: wl_display_connect_to_fd (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E3A379: wl_display_connect (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x400B28: main (wayland-test.c:43)
==14980==  Uninitialised value was created by a heap allocation
==14980==    at 0x4C28C10: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14980==    by 0x4E3C8DF: ??? (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E39E71: wl_proxy_marshal_array_constructor (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E3A0F9: wl_proxy_marshal_constructor (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x400A2C: wl_registry_bind (wayland-client-protocol.h:288)
==14980==    by 0x400ABE: global (wayland-test.c:15)
==14980==    by 0x53EC1EF: ffi_call_unix64 (in /usr/lib/libffi.so.6.0.4)
==14980==    by 0x53EBC57: ffi_call (in /usr/lib/libffi.so.6.0.4)
==14980==    by 0x4E3C757: ??? (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E39A5F: ??? (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E39ADB: ??? (in /usr/lib/libwayland-client.so.0.3.0)
==14980==    by 0x4E3A7AE: wl_display_dispatch_queue (in /usr/lib/libwayland-client.so.0.3.0)
==14980== 
==14980== 
==14980== HEAP SUMMARY:
==14980==     in use at exit: 0 bytes in 0 blocks
==14980==   total heap usage: 45 allocs, 45 frees, 24,232 bytes allocated
==14980== 
==14980== All heap blocks were freed -- no leaks are possible
==14980== 
==14980== For counts of detected and suppressed errors, rerun with: -v
==14980== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Comment 1 Jon 2016-02-10 12:43:56 UTC

Note, a normal execution (not with valgrind) results in SIGSEGV (Address boundary error).

Comment 2 Jonas Ådahl 2016-02-10 14:32:20 UTC

(In reply to Jon from comment #1)
> Note, a normal execution (not with valgrind) results in SIGSEGV (Address
> boundary error).

Are you sure there is a compositor running when you run your test case? I can only reproduce a SIGSEGV if there is no Wayland compositor to connect to. If it succeeds connecting, I only get the valgrind warning, no SIGSEGV.

Comment 3 Jon 2016-02-10 14:51:30 UTC

(In reply to Jonas Ådahl from comment #2)
> (In reply to Jon from comment #1)
> > Note, a normal execution (not with valgrind) results in SIGSEGV (Address
> > boundary error).
> 
> Are you sure there is a compositor running when you run your test case? I
> can only reproduce a SIGSEGV if there is no Wayland compositor to connect
> to. If it succeeds connecting, I only get the valgrind warning, no SIGSEGV.

Oops, sorry. Yeah the SIGSEGV only occurs if there's no compositor. The sendmsg valgrind error still occurs in either case though.

Comment 4 Jonas Ådahl 2016-02-10 15:36:07 UTC

(In reply to Jon from comment #3)
> (In reply to Jonas Ådahl from comment #2)
> > (In reply to Jon from comment #1)
> > > Note, a normal execution (not with valgrind) results in SIGSEGV (Address
> > > boundary error).
> > 
> > Are you sure there is a compositor running when you run your test case? I
> > can only reproduce a SIGSEGV if there is no Wayland compositor to connect
> > to. If it succeeds connecting, I only get the valgrind warning, no SIGSEGV.
> 
> Oops, sorry. Yeah the SIGSEGV only occurs if there's no compositor. The
> sendmsg valgrind error still occurs in either case though.

The reason is that we align the buffers by 4 bytes. So that when we put a string "ab" which is 3 bytes including '\0', we write:

0-3: [sender id]
4-5: [3 bytes long message]
6-7: [op code]
8-10: "ab\0"
11: [uninitialized]

The reader will read bytes 4-5 and then not read the uninitialized data.

This, is, more or less harmless, accept for the fact that we are sending uninitialized bytes from the one process's memory to another, which I would say is a security issue at least when the closure is sent from the server to the client. I'm sending a patch making the memory initialized to 0, so we avoid padding with uninitialized bytes.

Comment 5 Jonas Ådahl 2016-02-10 15:36:35 UTC

Here is the patch: https://lists.freedesktop.org/archives/wayland-devel/2016-February/026983.html

Comment 6 Jonas Ådahl 2016-02-12 10:12:46 UTC

Fixed by:

commit bf34ac75d0d61609296de1300196c843f4246e7c
Author: Jonas Ådahl <jadahl@gmail.com>
Date:   Wed Feb 10 23:35:44 2016 +0800

    connection: Don't add uninitialized memory as 4 byte alignment padding

which is now on master. It will be available in version 1.10.

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.