Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Jonas Ådahl]

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 wayland] resource-test: Use wl_seat instead of wl_display for testing

[Marek Chalupa]

Hi,

added reviewed-by to the whole series in some previous e-mail, so just 
adding it here again, so that it can be seen in the patchwork


Reviewed-by: Marek Chalupa 

Cheers,
Marek

On 01/14/16 21:32, Derek Foreman wrote:

We're creating resources with versions up to 4.  wl_display isn't version 4,
so this is technically verifying that we can do something we shouldn't.

wl_seat already has versions this high, so switch to that.

Signed-off-by: Derek Foreman 
---
  tests/resources-test.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/resources-test.c b/tests/resources-test.c
index ea88afb..337f9f9 100644
--- a/tests/resources-test.c
+++ b/tests/resources-test.c
@@ -45,7 +45,7 @@ TEST(create_resource_tst)
client = wl_client_create(display, s[0]);
assert(client);

-   res = wl_resource_create(client, _display_interface, 4, 0);
+   res = wl_resource_create(client, _seat_interface, 4, 0);
assert(res);

/* setters/getters */
@@ -105,7 +105,7 @@ TEST(destroy_res_tst)
client = wl_client_create(display, s[0]);
assert(client);

-   res = wl_resource_create(client, _display_interface, 4, 0);
+   res = wl_resource_create(client, _seat_interface, 4, 0);
assert(res);
wl_resource_set_implementation(res, NULL, , res_destroy_func);
wl_resource_add_destroy_listener(res, _listener);
@@ -119,7 +119,7 @@ TEST(destroy_res_tst)
assert(notify_called); /* check if signal was emitted */
assert(wl_client_get_object(client, id) == NULL);

-   res = wl_resource_create(client, _display_interface, 2, 0);
+   res = wl_resource_create(client, _seat_interface, 2, 0);
assert(res);
destroyed = 0;
notify_called = 0;
@@ -149,13 +149,13 @@ TEST(create_resource_with_same_id)
client = wl_client_create(display, s[0]);
assert(client);

-   res = wl_resource_create(client, _display_interface, 2, 0);
+   res = wl_resource_create(client, _seat_interface, 2, 0);
assert(res);
id = wl_resource_get_id(res);
assert(wl_client_get_object(client, id) == res);

/* this one should replace the old one */
-   res2 = wl_resource_create(client, _display_interface, 1, id);
+   res2 = wl_resource_create(client, _seat_interface, 1, id);
assert(res2 != NULL);
assert(wl_client_get_object(client, id) == res2);



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RFC weston 4/5] drm: Move backend to libweston

[Benoit Gschwind]

Hello,

Le 09/02/2016 16:14, Quentin Glidic a écrit :

From: Quentin Glidic 

Signed-off-by: Quentin Glidic 
---
  Makefile.am   | 10 ++---
  {src => lib}/compositor-drm.c | 87 +++
  {src => lib}/libbacklight.c   |  0
  {src => lib}/libbacklight.h   |  0
  lib/libweston.c   | 23 
  lib/libweston.h   |  1 +
  src/main.c|  3 ++
  7 files changed, 71 insertions(+), 53 deletions(-)
  rename {src => lib}/compositor-drm.c (97%)
  rename {src => lib}/libbacklight.c (100%)
  rename {src => lib}/libbacklight.h (100%)

diff --git a/Makefile.am b/Makefile.am
index 36078ba..68997d8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -287,12 +287,12 @@ drm_backend_la_CFLAGS =   \
$(DRM_COMPOSITOR_CFLAGS)\
$(AM_CFLAGS)
  drm_backend_la_SOURCES =  \
-   src/compositor-drm.c\
+   lib/compositor-drm.c\
$(INPUT_BACKEND_SOURCES)\
shared/helpers.h\
shared/timespec-util.h  \
-   src/libbacklight.c  \
-   src/libbacklight.h
+   lib/libbacklight.c  \
+   lib/libbacklight.h

  if ENABLE_VAAPI_RECORDER
  drm_backend_la_SOURCES += src/vaapi-recorder.c src/vaapi-recorder.h
@@ -1325,8 +1325,8 @@ if BUILD_SETBACKLIGHT
  noinst_PROGRAMS += setbacklight
  setbacklight_SOURCES =\
tests/setbacklight.c\
-   src/libbacklight.c  \
-   src/libbacklight.h
+   lib/libbacklight.c  \
+   lib/libbacklight.h
  setbacklight_CFLAGS = $(AM_CFLAGS) $(SETBACKLIGHT_CFLAGS)
  setbacklight_LDADD = $(SETBACKLIGHT_LIBS)
  endif
diff --git a/src/compositor-drm.c b/lib/compositor-drm.c
similarity index 97%
rename from src/compositor-drm.c
rename to lib/compositor-drm.c
index 04de95e..5d3cd0c 100644
--- a/src/compositor-drm.c
+++ b/lib/compositor-drm.c
@@ -46,7 +46,7 @@
  #include 
  #include 

-#include "libweston.h"
+#include "libweston-internal.h"
  #include "shared/helpers.h"
  #include "shared/timespec-util.h"
  #include "libbacklight.h"
@@ -88,6 +88,7 @@ enum output_config {

  struct drm_backend {
struct weston_backend base;
+   struct libweston_context *context;
struct weston_compositor *compositor;

struct udev *udev;
@@ -2144,15 +2145,15 @@ setup_output_seat_constraint(struct drm_backend *b,
  }

  static int
-get_gbm_format_from_section(struct weston_config_section *section,
-   uint32_t default_value,
-   uint32_t *format)
+get_gbm_format_from_config(struct libweston_context *context,
+  const char *section,
+  uint32_t default_value,
+  uint32_t *format)
  {
char *s;
int ret = 0;

-   weston_config_section_get_string(section,
-"gbm-format", , NULL);
+   s = context->backend_config.string_getter(section, "gbm-format", NULL, 
context->backend_config.user_data);

if (s == NULL)
*format = default_value;
@@ -2296,7 +2297,7 @@ create_output_for_connector(struct drm_backend *b,
struct drm_output *output;
struct drm_mode *drm_mode, *next, *current;
struct weston_mode *m;


[REF#1]


-   struct weston_config_section *section;
+   char section[39];
drmModeModeInfo crtc_mode, modeline;
int i, width, height, scale;
char *s;
@@ -2320,9 +2321,9 @@ create_output_for_connector(struct drm_backend *b,
output->base.serial_number = "unknown";
wl_list_init(>base.mode_list);



The _following changes_ seems wrong, in previous version, the function 
weston_config_get_section will try to find a section with a 'name' key 
and where the value of this 'name' equals to output->base.name. You 
replaced it with b->context->backend_config.string_getter, where first 
section is undefined see [REF#1]:

 - will probably always fallback to default values when not segfault,
 - even if section were fine, this approach does not handle severals 
outputs.


This case show that this approach is not very good, In that case the 
developer must give a list of unbound outputs setup and key/value is not 
well suited for that case. (there is some workaround, like using key 
pattern).


This case also leave me to add that section/key/value is not a good 
choice and you should stick to key/value.


I still prefer the opaque configuration structure with function to set 
the structure content.


Best regards.


-   section = weston_config_get_section(b->compositor->config, "output", 
"name",
-   output->base.name);
-