Harden against X11 instability.

Avoid accessing null pointers when an X11 call fails.

Since EGL doesn't own the X11 window, we expect it to be valid
for the duration of the EGL surface. Fail hard if that's not the case.

Bug chromium:833229
Bug chromium:824522

Change-Id: Iba5e3832fe312fb50232a13e2163a022f5048a76
Reviewed-on: https://swiftshader-review.googlesource.com/19788
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>

diff --git a/src/Main/FrameBufferX11.cpp b/src/Main/FrameBufferX11.cpp
index ca0bb4b..ca56b59 100644 (file)
--- a/src/Main/FrameBufferX11.cpp
+++ b/src/Main/FrameBufferX11.cpp
@@ -21,6 +21,7 @@
 #include <sys/shm.h>
 #include <string.h>
 #include <assert.h>
+#include <stdlib.h>
 
 namespace sw
 {
@@ -46,8 +47,11 @@ namespace sw
                if(!x_display)
                {
                        x_display = libX11->XOpenDisplay(0);
+                       assert(x_display);
                }
 
+               validateWindow();
+
                int screen = DefaultScreen(x_display);
                x_gc = libX11->XDefaultGC(x_display, screen);
                int depth = libX11->XDefaultDepth(x_display, screen);
@@ -63,7 +67,7 @@ namespace sw
                        x_image = libX11->XShmCreateImage(x_display, visual, depth, ZPixmap, 0, &shminfo, width, height);
 
                        shminfo.shmid = shmget(IPC_PRIVATE, x_image->bytes_per_line * x_image->height, IPC_CREAT | SHM_R | SHM_W);
-                       shminfo.shmaddr = x_image->data = buffer = (char*)shmat(shminfo.shmid, 0, 0);
+                       shminfo.shmaddr = x_image->data = (char*)shmat(shminfo.shmid, 0, 0);
                        shminfo.readOnly = False;
 
                        PreviousXErrorHandler = libX11->XSetErrorHandler(XShmErrorHandler);
@@ -87,9 +91,16 @@ namespace sw
                {
                        int bytes_per_line = width * 4;
                        int bytes_per_image = height * bytes_per_line;
-                       buffer = new char[bytes_per_image];
+                       char *buffer = (char*)malloc(bytes_per_image);
                        memset(buffer, 0, bytes_per_image);
+
                        x_image = libX11->XCreateImage(x_display, visual, depth, ZPixmap, 0, buffer, width, height, 32, bytes_per_line);
+                       assert(x_image);
+
+                       if(!x_image)
+                       {
+                               free(buffer);
+                       }
                }
        }
 
@@ -97,11 +108,7 @@ namespace sw
        {
                if(!mit_shm)
                {
-                       x_image->data = 0;
                        XDestroyImage(x_image);
-
-                       delete[] buffer;
-                       buffer = 0;
                }
                else
                {
@@ -111,6 +118,9 @@ namespace sw
                        shmctl(shminfo.shmid, IPC_RMID, 0);
                }
 
+               // Last chance to check the window before we close the display.
+               validateWindow();
+
                if(ownX11)
                {
                        libX11->XCloseDisplay(x_display);
@@ -119,8 +129,11 @@ namespace sw
 
        void *FrameBufferX11::lock()
        {
-               stride = x_image->bytes_per_line;
-               framebuffer = buffer;
+               if(x_image)
+               {
+                       stride = x_image->bytes_per_line;
+                       framebuffer = x_image->data;
+               }
 
                return framebuffer;
        }
@@ -134,6 +147,8 @@ namespace sw
        {
                copy(source);
 
+               assert(validateWindow());
+
                if(!mit_shm)
                {
                        libX11->XPutImage(x_display, x_window, x_gc, x_image, 0, 0, 0, 0, width, height);
@@ -175,6 +190,21 @@ namespace sw
                        libX11->XDrawString(x_display, x_window, x_gc, 50, 50, string, strlen(string));
                }
        }
+
+       bool FrameBufferX11::validateWindow()
+       {
+               // Since we don't own the window, it is the external client code's responsibility
+               // to not destroy it until we're done with it. We help out by validating it.
+               XWindowAttributes windowAttributes;
+               Status status = libX11->XGetWindowAttributes(x_display, x_window, &windowAttributes);
+
+               if(status != True)
+               {
+                       abort();   // Fail hard if we can't obtain the window's attributes.
+               }
+
+               return true;
+       }
 }
 
 NO_SANITIZE_FUNCTION sw::FrameBuffer *createFrameBuffer(void *display, Window window, int width, int height)

diff --git a/src/Main/FrameBufferX11.hpp b/src/Main/FrameBufferX11.hpp
index f25487b..aa06276 100644 (file)
--- a/src/Main/FrameBufferX11.hpp
+++ b/src/Main/FrameBufferX11.hpp
@@ -38,6 +38,8 @@ namespace sw
                void unlock() override;
 
        private:
+               bool validateWindow();
+
                bool ownX11;
                Display *x_display;
                Window x_window;
@@ -47,8 +49,6 @@ namespace sw
 
                bool mit_shm;
                XShmSegmentInfo shminfo;
-
-               char *buffer;
        };
 }