Merge lp:~afrantzis/mir/drm-device-fd-close-on-exec into lp:mir

[Alexandros Frantzis]

mesa: Ensure we close drm device file descriptors on exec

mesa: Ensure we close drm device file descriptors on exec

This is needed for https://bugs.launchpad.net/mir/+bug/1284081 , but it's not a complete fix, because Mesa internally dup()s our drm fd without setting the close-on-exec flag.

[Alan Griffiths (alan-griffiths)]

Looks sensible

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1427
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/945/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1049
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1047
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/641
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/677
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/677/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/682
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/682/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/642
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/642/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/616
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4316

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/945/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

This looks like a potential mismatch:

3 parameters mocked:
8 + MOCK_METHOD3(open, int(char const* path, int flags, mode_t mode));

2 parameters used:
35 + tmp_fd = open(device.devnode(), O_RDWR | O_CLOEXEC);

Maybe it mocks correctly still? I guess the expectations would fail otherwise. The real function is:
/usr/include/fcntl.h:extern int open (const char *__file, int __oflag, ...) __nonnull ((1));

Seems to work.

review: Approve

=== modified file 'include/test/mir_test_doubles/mock_drm.h'
--- include/test/mir_test_doubles/mock_drm.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/mock_drm.h	2014-02-26 12:30:46 +0000
@@ -88,6 +88,7 @@
     MockDRM();
     ~MockDRM() noexcept;
 
+    MOCK_METHOD3(open, int(char const* path, int flags, mode_t mode));
     MOCK_METHOD2(drmOpen, int(const char *name, const char *busid));
     MOCK_METHOD1(drmClose, int(int fd));
     MOCK_METHOD3(drmIoctl, int(int fd, unsigned long request, void *arg));
 
=== modified file 'src/platform/graphics/mesa/display_helpers.cpp'
--- src/platform/graphics/mesa/display_helpers.cpp	2014-01-22 08:32:55 +0000
+++ src/platform/graphics/mesa/display_helpers.cpp	2014-02-26 12:30:46 +0000
@@ -65,6 +65,13 @@
         BOOST_THROW_EXCEPTION(
             std::runtime_error("Failed to open DRM device for authenticated fd"));
 
+    if (fcntl(auth_fd, F_SETFD, fcntl(auth_fd, F_GETFD) | FD_CLOEXEC) == -1)
+    {
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to set FD_CLOEXEC for authenticated drm fd")));
+    }
+
     drm_magic_t magic;
     int ret = -1;
     if ((ret = drmGetMagic(auth_fd, &magic)) < 0)
@@ -198,7 +205,7 @@
             continue;
 
         // If directly opening the DRM device is good enough for X it's good enough for us!
-        tmp_fd = open(device.devnode(), O_RDWR, O_CLOEXEC);
+        tmp_fd = open(device.devnode(), O_RDWR | O_CLOEXEC);
         if (tmp_fd < 0)
         {
             error = errno;
 
=== modified file 'tests/mir_test_doubles/mock_drm.cpp'
--- tests/mir_test_doubles/mock_drm.cpp	2014-01-13 06:12:33 +0000
+++ tests/mir_test_doubles/mock_drm.cpp	2014-02-26 12:30:46 +0000
@@ -229,6 +229,9 @@
 
     global_mock = this;
 
+    ON_CALL(*this, open(_,_,_))
+    .WillByDefault(Return(fake_drm.fd()));
+
     ON_CALL(*this, drmOpen(_,_))
     .WillByDefault(Return(fake_drm.fd()));
 
@@ -405,7 +408,7 @@
 {
     char const* drm_prefix = "/dev/dri/";
     if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
-        return global_mock->drmOpen("i915", NULL);
+        return global_mock->open(path, flags, mode);
 
     int (*real_open)(char const *path, int flags, mode_t mode);
     *(void **)(&real_open) = dlsym(RTLD_NEXT, "open");
@@ -417,7 +420,7 @@
 {
     char const* drm_prefix = "/dev/dri/";
     if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
-        return global_mock->drmOpen("i915", NULL);
+        return global_mock->open(path, flags, mode);
 
     int (*real_open64)(char const *path, int flags, mode_t mode);
     *(void **)(&real_open64) = dlsym(RTLD_NEXT, "open64");
@@ -429,7 +432,7 @@
 {
     char const* drm_prefix = "/dev/dri/";
     if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
-        return global_mock->drmOpen("i915", NULL);
+        return global_mock->open(path, flags, mode);
 
     int (*real_open)(char const *path, int flags, mode_t mode);
     *(void **)(&real_open) = dlsym(RTLD_NEXT, "__open");
@@ -441,7 +444,7 @@
 {
     char const* drm_prefix = "/dev/dri/";
     if (!strncmp(path, drm_prefix, strlen(drm_prefix)))
-        return global_mock->drmOpen("i915", NULL);
+        return global_mock->open(path, flags, mode);
 
     int (*real_open64)(char const *path, int flags, mode_t mode);
     *(void **)(&real_open64) = dlsym(RTLD_NEXT, "__open64");
 
=== modified file 'tests/unit-tests/graphics/mesa/CMakeLists.txt'
--- tests/unit-tests/graphics/mesa/CMakeLists.txt	2014-01-22 08:32:55 +0000
+++ tests/unit-tests/graphics/mesa/CMakeLists.txt	2014-02-26 12:30:46 +0000
@@ -17,6 +17,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_native_platform.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_anonymous_shm_file.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_shm_buffer.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_drm_helper.cpp
 )
 
 set(UNIT_TEST_SOURCES ${UNIT_TEST_SOURCES} PARENT_SCOPE)
 
=== modified file 'tests/unit-tests/graphics/mesa/test_display.cpp'
--- tests/unit-tests/graphics/mesa/test_display.cpp	2014-02-21 15:32:30 +0000
+++ tests/unit-tests/graphics/mesa/test_display.cpp	2014-02-26 12:30:46 +0000
@@ -332,7 +332,7 @@
 {
     using namespace testing;
 
-    EXPECT_CALL(mock_drm, drmOpen(_,_))
+    EXPECT_CALL(mock_drm, open(_,_,_))
         .Times(AtLeast(1))
         .WillRepeatedly(Return(-1));
 
 
=== added file 'tests/unit-tests/graphics/mesa/test_drm_helper.cpp'
--- tests/unit-tests/graphics/mesa/test_drm_helper.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/graphics/mesa/test_drm_helper.cpp	2014-02-26 12:30:46 +0000
@@ -0,0 +1,65 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "src/platform/graphics/mesa/display_helpers.h"
+#include "mir/udev/wrapper.h"
+
+#include "mir_test_framework/udev_environment.h"
+#include "mir_test_doubles/mock_drm.h"
+
+#include <fcntl.h>
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace mgm = mir::graphics::mesa;
+namespace mtf = mir::mir_test_framework;
+namespace mtd = mir::test::doubles;
+                          
+namespace
+{
+
+MATCHER_P(FlagSet, flag, "")
+{
+    return (arg & flag);
+}
+
+class DRMHelperTest : public ::testing::Test
+{              
+public:        
+    DRMHelperTest()
+    {          
+        fake_devices.add_standard_device("standard-drm-devices");
+    }          
+               
+protected:       
+    ::testing::NiceMock<mtd::MockDRM> mock_drm;
+    mtf::UdevEnvironment fake_devices;
+};
+
+}
+
+TEST_F(DRMHelperTest, closes_drm_fd_on_exec)
+{   
+    using namespace testing;
+
+    EXPECT_CALL(mock_drm, open(_, FlagSet(O_CLOEXEC), _));
+
+    mgm::helpers::DRMHelper drm_helper;
+    drm_helper.setup(std::make_shared<mir::udev::Context>());
+}   
 
=== modified file 'tests/unit-tests/graphics/mesa/test_platform.cpp'
--- tests/unit-tests/graphics/mesa/test_platform.cpp	2014-02-19 14:01:37 +0000
+++ tests/unit-tests/graphics/mesa/test_platform.cpp	2014-02-26 12:30:46 +0000
@@ -29,6 +29,7 @@
 #include <gtest/gtest.h>
 
 #include "mir_test_framework/udev_environment.h"
+#include "mir_test/pipe.h"
 
 #include "mir_test_doubles/mock_drm.h"
 #include "mir_test_doubles/mock_gbm.h"
@@ -76,12 +77,13 @@
 TEST_F(MesaGraphicsPlatform, get_ipc_package)
 {
     using namespace testing;
-    const int auth_fd{66};
+    mir::test::Pipe auth_pipe;
+    int const auth_fd{auth_pipe.read_fd()};
 
     /* First time for master DRM fd, second for authenticated fd */
+    EXPECT_CALL(mock_drm, open(_,_,_))
+        .WillOnce(Return(mock_drm.fake_drm.fd()));
     EXPECT_CALL(mock_drm, drmOpen(_,_))
-        .Times(2)
-        .WillOnce(Return(mock_drm.fake_drm.fd()))
         .WillOnce(Return(auth_fd));
 
     /* Expect proper authorization */
@@ -107,7 +109,7 @@
 {
     using namespace ::testing;
 
-    EXPECT_CALL(mock_drm, drmOpen(_,_))
+    EXPECT_CALL(mock_drm, open(_,_,_))
             .WillRepeatedly(Return(-1));
 
     try