Merge lp:~3v1n0/compiz/ignore-invalid-sized-textures into lp:compiz/0.9.11

[Marco Trevisan (Treviño)]

Opengl, Texture: don't try to create a texture of invalid (empty or negative) size

This seem to cause also a crash when using software rendering.

There's no reason to create textures of 0x0 size, we can just return an empty list in this case.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3833
http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-ci/38/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-amd64-ci/38/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-armhf-ci/38/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-i386-ci/38/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/compiz-0.9.11-ci/38/rebuild

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

Ouch that's nasty.

One thing to consider is that I don't think it is possible for a
Drawable to have a zero or negative size. Taken from the manpage for
XCreatePixmap for instance:

"The width and height arguments must be nonzero, or a BadValue error results."

As such, if bindPixmapToTexture is called with a zero or negative
size, it is almost certainly programmer error. I would think that a
warning should be printed here or that an assertion should be fired.
Just returning an empty list runs the danger of this programmer error
(which can have quite serious results) being silently ignored, which
might cause more subtle problems later down the line.

(I'd also have a look at unity::UnityWindow::DrawWindowDecoration,
because it seems like the bad value is coming from there).

On Mon, Mar 3, 2014 at 8:54 PM, Marco Trevisan (Treviño) <[e-mail]> wrote:
> Marco Trevisan (Treviño) has proposed merging lp:~3v1n0/compiz/ignore-invalid-sized-textures into lp:compiz.
>
> Commit message:
> Opengl, Texture: don't try to create a texture of invalid (empty or negative) size
>
> This seem to cause also a crash when using software rendering.
>
> Requested reviews:
> Compiz Maintainers (compiz-team)
> Related bugs:
> Bug #1055166 in Compiz: "compiz crashed with SIGSEGV in memmove() from drisw_update_tex_buffer() from dri_set_tex_buffer2() from operator() from compiz::opengl::bindTexImageGLX() from ... from unity::UnityWindow::DrawWindowDecoration"
> https://bugs.launchpad.net/compiz/+bug/1055166
>
> For more details, see:
> https://code.launchpad.net/~3v1n0/compiz/ignore-invalid-sized-textures/+merge/209061
>
> There's no reason to create textures of 0x0 size, we can just return an empty list in this case.
> --
> https://code.launchpad.net/~3v1n0/compiz/ignore-invalid-sized-textures/+merge/209061
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~3v1n0/compiz/ignore-invalid-sized-textures into lp:compiz.
>
> === modified file 'plugins/opengl/src/texture.cpp'
> --- plugins/opengl/src/texture.cpp 2012-09-19 12:18:34 +0000
> +++ plugins/opengl/src/texture.cpp 2014-03-03 12:53:46 +0000
> @@ -433,6 +433,12 @@
> int depth,
> compiz::opengl::PixmapSource source)
> {
> + if (!GL::textureFromPixmap || width <= 0 || height <= 0 ||
> + width > GL::maxTextureSize || height > GL::maxTextureSize)
> + {
> + return GLTexture::List ();
> + }
> +
> GLTexture::List rv;
>
> foreach (BindPixmapProc &proc, GLScreen::get (screen)->priv->bindPixmap)
> @@ -473,10 +479,6 @@
> int depth,
> compiz::opengl::PixmapSource source)
> {
> - if ((int) width > GL::maxTextureSize || (int) height > GL::maxTextureSize ||
> - !GL::textureFromPixmap)
> - return GLTexture::List ();
> -
> GLTexture::List rv (1);
> EglTexture *tex = NULL;
> EGLImageKHR eglImage = NULL;
> @@ -644,10 +646,6 @@
> int depth,
> cgl::Pixmap...

[lp:~3v1n0/compiz/ignore-invalid-sized-textures]

3834. By Marco Trevisan (Treviño) on 2014-03-03

debian/patches/100_workaround_virtualbox_hang.patch: refresh to apply in current trunk

[Marco Trevisan (Treviño) (3v1n0)]

> As such, if bindPixmapToTexture is called with a zero or negative
> size, it is almost certainly programmer error. I would think that a
> warning should be printed here or that an assertion should be fired.
> Just returning an empty list runs the danger of this programmer error
> (which can have quite serious results) being silently ignored, which
> might cause more subtle problems later down the line.

Right.

> (I'd also have a look at unity::UnityWindow::DrawWindowDecoration,
> because it seems like the bad value is coming from there).

Yeah, that's already done, but I'd also prefer to avoid this to happen.

[Sam Spilsbury (smspillaz)]

On Mon, Mar 3, 2014 at 10:36 PM, Marco Trevisan (Treviño)
<[e-mail]> wrote:
>> As such, if bindPixmapToTexture is called with a zero or negative
>> size, it is almost certainly programmer error. I would think that a
>> warning should be printed here or that an assertion should be fired.
>> Just returning an empty list runs the danger of this programmer error
>> (which can have quite serious results) being silently ignored, which
>> might cause more subtle problems later down the line.
>
> Right.
>
>> (I'd also have a look at unity::UnityWindow::DrawWindowDecoration,
>> because it seems like the bad value is coming from there).
>
> Yeah, that's already done, but I'd also prefer to avoid this to happen.

Yep, sounds good. Not crashing with explanation and a warning is
definitely better than crashing without explanation where a runtime
contract exists.

> --
> https://code.launchpad.net/~3v1n0/compiz/ignore-invalid-sized-textures/+merge/209061
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~3v1n0/compiz/ignore-invalid-sized-textures into lp:compiz.

--
Sam Spilsbury

[lp:~3v1n0/compiz/ignore-invalid-sized-textures]

3835. By Marco Trevisan (Treviño) on 2014-03-03

OpenGL, Texture: log about errors in case texture binding is wrong

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3834
http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-ci/39/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-amd64-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-armhf-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-i386-ci/39

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/compiz-0.9.11-ci/39/rebuild

review: Approve (continuous-integration)

[lp:~3v1n0/compiz/ignore-invalid-sized-textures]

3836. By Marco Trevisan (Treviño) on 2014-03-03

OpenGL, Texture: fix indentation change

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3835
http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-ci/40/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-amd64-ci/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-armhf-ci/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-i386-ci/40

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/compiz-0.9.11-ci/40/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3836
http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-ci/41/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-amd64-ci/41
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-armhf-ci/41
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.11-trusty-i386-ci/41

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/compiz-0.9.11-ci/41/rebuild

review: Approve (continuous-integration)

[Christopher Townsend (townsend)]

LGTM

review: Approve

=== modified file 'debian/patches/100_workaround_virtualbox_hang.patch'
--- debian/patches/100_workaround_virtualbox_hang.patch	2013-05-10 16:10:55 +0000
+++ debian/patches/100_workaround_virtualbox_hang.patch	2014-03-03 16:20:39 +0000
@@ -1,6 +1,8 @@
---- a/plugins/opengl/include/opengl/opengl.h
-+++ b/plugins/opengl/include/opengl/opengl.h
-@@ -582,6 +582,17 @@
+Index: compiz/plugins/opengl/include/opengl/opengl.h
+===================================================================
+--- compiz.orig/plugins/opengl/include/opengl/opengl.h	2014-03-03 15:29:29.898235681 +0100
++++ compiz/plugins/opengl/include/opengl/opengl.h	2014-03-03 15:29:29.894235669 +0100
+@@ -590,6 +590,17 @@
  
  extern GLScreenPaintAttrib defaultScreenPaintAttrib;
  
@@ -18,7 +20,7 @@
  class GLScreen;
  class GLFramebufferObject;
  class GLScreenInterface;
-@@ -774,6 +785,13 @@
+@@ -787,6 +798,13 @@
  
  	bool glInitContext (XVisualInfo *);
  
@@ -32,8 +34,10 @@
  	WRAPABLE_HND (0, GLScreenInterface, bool, glPaintOutput,
  		      const GLScreenPaintAttrib &, const GLMatrix &,
  		      const CompRegion &, CompOutput *, unsigned int);
---- a/plugins/opengl/src/privates.h
-+++ b/plugins/opengl/src/privates.h
+Index: compiz/plugins/opengl/src/privates.h
+===================================================================
+--- compiz.orig/plugins/opengl/src/privates.h	2014-03-03 15:29:29.898235681 +0100
++++ compiz/plugins/opengl/src/privates.h	2014-03-03 15:29:29.894235669 +0100
 @@ -47,6 +47,24 @@
  
  extern CompOutput *targetOutput;
@@ -59,15 +63,15 @@
  class GLDoubleBuffer :
      public compiz::opengl::DoubleBuffer
  {
-@@ -123,6 +141,7 @@
- class PrivateGLScreen :
+@@ -141,6 +159,7 @@
      public ScreenInterface,
+     public CompositeScreenInterface,
      public compiz::composite::PaintHandler,
 +    public compiz::opengl::internal::DriverWorkaroundQuery,
      public OpenglOptions
  {
      public:
-@@ -207,6 +226,7 @@
+@@ -232,6 +251,7 @@
  	std::vector<GLTexture::BindPixmapProc> bindPixmap;
  	bool hasCompositing;
  	bool commonFrontbuffer;
@@ -75,8 +79,8 @@
  	bool incorrectRefreshRate; // hack for NVIDIA specifying an incorrect
  				   // refresh rate, causing us to miss vblanks
  
-@@ -226,6 +246,10 @@
- 
+@@ -253,6 +273,10 @@
+ 	bool postprocessingRequired;
  	mutable CompString prevRegex;
  	mutable bool       prevBlacklisted;
 +
@@ -86,8 +90,10 @@
  };
  
  class PrivateGLWindow :
---- a/plugins/opengl/src/screen.cpp
-+++ b/plugins/opengl/src/screen.cpp
+Index: compiz/plugins/opengl/src/screen.cpp
+===================================================================
+--- compiz.orig/plugins/opengl/src/screen.cpp	2014-03-03 15:29:29.898235681 +0100
++++ compiz/plugins/opengl/src/screen.cpp	2014-03-03 15:29:29.894235669 +0100
 @@ -67,6 +67,7 @@
  
  
@@ -96,8 +102,8 @@
  
  namespace GL {
      #ifdef USE_GLES
-@@ -359,6 +360,18 @@
- #endif
+@@ -537,6 +538,18 @@
+ };
  
  bool
 +PrivateGLScreen::unsafeForExternalBinds () const
@@ -115,7 +121,7 @@
  GLScreen::glInitContext (XVisualInfo *visinfo)
  {
  #ifndef USE_GLES
-@@ -660,6 +673,7 @@
+@@ -855,6 +868,7 @@
  
      priv->commonFrontbuffer = true;
      priv->incorrectRefreshRate = false;
@@ -123,7 +129,7 @@
      if (glRenderer != NULL && strstr (glRenderer, "on llvmpipe"))
      {
  	/*
-@@ -680,6 +694,18 @@
+@@ -875,6 +889,18 @@
  	priv->incorrectRefreshRate = true;
      }
  
@@ -142,8 +148,10 @@
      if (strstr (glExtensions, "GL_ARB_texture_non_power_of_two"))
  	GL::textureNonPowerOfTwo = true;
      GL::textureNonPowerOfTwoMipmap = GL::textureNonPowerOfTwo;
---- a/plugins/opengl/src/texture.cpp
-+++ b/plugins/opengl/src/texture.cpp
+Index: compiz/plugins/opengl/src/texture.cpp
+===================================================================
+--- compiz.orig/plugins/opengl/src/texture.cpp	2014-03-03 15:29:29.898235681 +0100
++++ compiz/plugins/opengl/src/texture.cpp	2014-03-03 15:31:45.974666737 +0100
 @@ -41,6 +41,7 @@
  #include "glx-tfp-bind.h"
  
@@ -152,15 +160,14 @@
  
  #ifdef USE_GLES
  std::map<Damage, EglTexture*> boundPixmapTex;
-@@ -648,6 +649,13 @@
-         !GL::textureFromPixmap)
- 	return GLTexture::List ();
- 
+@@ -646,6 +647,12 @@
+ 				 int               depth,
+ 				 cgl::PixmapSource source)
+ {
 +    GLScreen *gs = GLScreen::get (screen);
 +    const cglint::DriverWorkaroundQuery &query (gs->fetchDriverWorkarounds ());
 +
-+    if (query.unsafeForExternalBinds () &&
-+	source == cgl::ExternallyManaged)
++    if (query.unsafeForExternalBinds () && source == cgl::ExternallyManaged)
 +	return GLTexture::List ();
 +
      GLTexture::List   rv (1);

=== modified file 'plugins/opengl/src/texture.cpp'
--- plugins/opengl/src/texture.cpp	2012-09-19 12:18:34 +0000
+++ plugins/opengl/src/texture.cpp	2014-03-03 16:20:39 +0000
@@ -433,6 +433,28 @@
 				int                          depth,
 				compiz::opengl::PixmapSource source)
 {
+    if (!GL::textureFromPixmap)
+    {
+	compLogMessage("opengl", CompLogLevelError,
+		       "GL::textureFromPixmap is not supported.");
+    }
+
+    if (width <= 0 || height <= 0)
+    {
+	compLogMessage("opengl", CompLogLevelError,
+		       "Couldn't bind 0-sized pixmap to texture: "
+		       "the width and height arguments must be nonzero.");
+	return GLTexture::List ();
+    }
+
+    if (width > GL::maxTextureSize || height > GL::maxTextureSize)
+    {
+	compLogMessage("opengl", CompLogLevelError,
+		       "Impossible to bind a pixmap bigger than %dx%d to texture.",
+		       GL::maxTextureSize, GL::maxTextureSize);
+	return GLTexture::List ();
+    }
+
     GLTexture::List rv;
 
     foreach (BindPixmapProc &proc, GLScreen::get (screen)->priv->bindPixmap)
@@ -473,10 +495,6 @@
 				 int                          depth,
 				 compiz::opengl::PixmapSource source)
 {
-    if ((int) width > GL::maxTextureSize || (int) height > GL::maxTextureSize ||
-        !GL::textureFromPixmap)
-	return GLTexture::List ();
-
     GLTexture::List   rv (1);
     EglTexture        *tex = NULL;
     EGLImageKHR       eglImage = NULL;
@@ -644,10 +662,6 @@
 				 int               depth,
 				 cgl::PixmapSource source)
 {
-    if ((int) width > GL::maxTextureSize || (int) height > GL::maxTextureSize ||
-        !GL::textureFromPixmap)
-	return GLTexture::List ();
-
     GLTexture::List   rv (1);
     TfpTexture        *tex = NULL;
     unsigned int      target = 0;