Mir

Merge lp:~afrantzis/mir/correct-cmsg-storage-1320821 into lp:mir

  1. correct-cmsg-storage-1320821
  2. Merge into development-branch
Proposed by Alexandros Frantzis
Status: Merged
Approved by: Alexandros Frantzis
Approved revision: no longer in the source branch.
Merged at revision: 1655
Proposed branch: lp:~afrantzis/mir/correct-cmsg-storage-1320821
Merge into: lp:mir
Diff against target: 128 lines (+36/-23)
2 files modified
src/client/rpc/mir_socket_rpc_channel.cpp (+24/-16)
src/server/frontend/socket_messenger.cpp (+12/-7)
To merge this branch: bzr merge lp:~afrantzis/mir/correct-cmsg-storage-1320821
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alberto Aguirre (community) Approve
Alan Griffiths Approve
Review via email: mp+220234@code.launchpad.net

Commit message

server,client: Allocate space properly for recvmsg/sendmsg control messages

Buffers for control messages need to follow alignment and padding restrictions that are enforced through the CMSG_* macros.

Description of the change

server,client: Allocate space properly for recvmsg/sendmsg control messages

Buffers for control messages need to follow alignment and padding restrictions that are enforced through the CMSG_* macros.

This MP also substitutes some std::vector<>s with mir::VariableLengthArrays to decrease heap allocations.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1643
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1682/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/264
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/265
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/264
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/201/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/200
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/200/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/683
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/683/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1512
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7393

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1644
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1684/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/267
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/268
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/267
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/203
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/203/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/202
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/202/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/692
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/692/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1515
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7403

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

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

38 - // Control message contains file descriptors
39 - struct cmsghdr *message = CMSG_FIRSTHDR(&header);
40 - message->cmsg_len = header.msg_controllen;
41 - message->cmsg_level = SOL_SOCKET;
42 - message->cmsg_type = SCM_RIGHTS;
43 -

I'm surprised we can lose this initialization. But it appears to work.

review: Approve
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Ok.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/692/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/303/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/304/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/303/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/76/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/76/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/798/console

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1645
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1711/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/306
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/306
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/230
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/230/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/229
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/229/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/807
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/807/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1550
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7529

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

review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> 38 - // Control message contains file descriptors
> 39 - struct cmsghdr *message = CMSG_FIRSTHDR(&header);
> 40 - message->cmsg_len = header.msg_controllen;
> 41 - message->cmsg_level = SOL_SOCKET;
> 42 - message->cmsg_type = SCM_RIGHTS;
> 43 -
>
> I'm surprised we can lose this initialization. But it appears to work.

recvmsg() receives any pending data and control messages that fit into the buffers we are giving it. There is no support for message filtering, e.g., to return only control messages of the SCM_RIGHTS type, as was misleadingly implied by the removed snippet. So we were just writing into a buffer the contents of which were never read, just got overwritten by recvmsg.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
2--- src/client/rpc/mir_socket_rpc_channel.cpp 2014-05-22 11:31:21 +0000
3+++ src/client/rpc/mir_socket_rpc_channel.cpp 2014-05-22 12:43:57 +0000
4@@ -24,6 +24,7 @@
5 #include "../mir_surface.h"
6 #include "../display_configuration.h"
7 #include "../lifecycle_control.h"
8+#include "mir/variable_length_array.h"
9
10 #include "mir_protobuf.pb.h" // For Buffer frig
11 #include "mir_protobuf_wire.pb.h"
12@@ -208,7 +209,7 @@
13 complete->Run();
14 }
15
16-void mclr::MirSocketRpcChannel::receive_file_descriptors(std::vector<int> &fds)
17+void mclr::MirSocketRpcChannel::receive_file_descriptors(std::vector<int>& fds)
18 {
19 // We send dummy data
20 struct iovec iov;
21@@ -217,8 +218,10 @@
22 iov.iov_len = 1;
23
24 // Allocate space for control message
25- auto n_fds = fds.size();
26- std::vector<char> control(sizeof(struct cmsghdr) + sizeof(int) * n_fds);
27+ static auto const builtin_n_fds = 5;
28+ static auto const builtin_cmsg_space = CMSG_SPACE(builtin_n_fds * sizeof(int));
29+ auto const fds_bytes = fds.size() * sizeof(int);
30+ mir::VariableLengthArray<builtin_cmsg_space> control{CMSG_SPACE(fds_bytes)};
31
32 // Message to send
33 struct msghdr header;
34@@ -230,14 +233,8 @@
35 header.msg_control = control.data();
36 header.msg_flags = 0;
37
38- // Control message contains file descriptors
39- struct cmsghdr *message = CMSG_FIRSTHDR(&header);
40- message->cmsg_len = header.msg_controllen;
41- message->cmsg_level = SOL_SOCKET;
42- message->cmsg_type = SCM_RIGHTS;
43-
44 using std::chrono::steady_clock;
45- auto time_limit = steady_clock::now() + timeout;
46+ auto const time_limit = steady_clock::now() + timeout;
47
48 while (recvmsg(socket.native_handle(), &header, 0) < 0)
49 {
50@@ -245,12 +242,23 @@
51 return;
52 }
53
54- // Copy file descriptors back to caller
55- n_fds = (message->cmsg_len - sizeof(struct cmsghdr)) / sizeof(int);
56- fds.resize(n_fds);
57- int *data = (int *)CMSG_DATA(message);
58- for (std::vector<int>::size_type i = 0; i < n_fds; i++)
59- fds[i] = data[i];
60+ // If we get a proper control message, copy the received
61+ // file descriptors back to the caller
62+ struct cmsghdr const* const cmsg = CMSG_FIRSTHDR(&header);
63+
64+ if (cmsg && cmsg->cmsg_len == CMSG_LEN(fds_bytes) &&
65+ cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
66+ {
67+ int const* const data = reinterpret_cast<int const*>CMSG_DATA(cmsg);
68+ int i = 0;
69+ for (auto& fd : fds)
70+ fd = data[i++];
71+ }
72+ else
73+ {
74+ BOOST_THROW_EXCEPTION(
75+ std::runtime_error("Invalid control message for receiving file descriptors"));
76+ }
77 }
78
79 void mclr::MirSocketRpcChannel::CallMethod(
80
81=== modified file 'src/server/frontend/socket_messenger.cpp'
82--- src/server/frontend/socket_messenger.cpp 2014-05-22 11:31:21 +0000
83+++ src/server/frontend/socket_messenger.cpp 2014-05-22 12:43:57 +0000
84@@ -85,8 +85,7 @@
85
86 void mfd::SocketMessenger::send_fds_locked(std::unique_lock<std::mutex> const&, std::vector<int32_t> const& fds)
87 {
88- auto n_fds = fds.size();
89- if (n_fds > 0)
90+ if (fds.size() > 0)
91 {
92 // We send dummy data
93 struct iovec iov;
94@@ -95,7 +94,12 @@
95 iov.iov_len = 1;
96
97 // Allocate space for control message
98- std::vector<char> control(sizeof(struct cmsghdr) + sizeof(int) * n_fds);
99+ static auto const builtin_n_fds = 5;
100+ static auto const builtin_cmsg_space = CMSG_SPACE(builtin_n_fds * sizeof(int));
101+ auto const fds_bytes = fds.size() * sizeof(int);
102+ mir::VariableLengthArray<builtin_cmsg_space> control{CMSG_SPACE(fds_bytes)};
103+ // Silence valgrind uninitialized memory complaint
104+ memset(control.data(), 0, control.size());
105
106 // Message to send
107 struct msghdr header;
108@@ -109,15 +113,16 @@
109
110 // Control message contains file descriptors
111 struct cmsghdr *message = CMSG_FIRSTHDR(&header);
112- message->cmsg_len = header.msg_controllen;
113+ message->cmsg_len = CMSG_LEN(fds_bytes);
114 message->cmsg_level = SOL_SOCKET;
115 message->cmsg_type = SCM_RIGHTS;
116- int *data = reinterpret_cast<int*>(CMSG_DATA(message));
117+
118+ int* const data = reinterpret_cast<int*>(CMSG_DATA(message));
119 int i = 0;
120- for (auto &fd: fds)
121+ for (auto fd : fds)
122 data[i++] = fd;
123
124- auto sent = sendmsg(socket->native_handle(), &header, 0);
125+ auto const sent = sendmsg(socket->native_handle(), &header, 0);
126 if (sent < 0)
127 BOOST_THROW_EXCEPTION(std::runtime_error("Failed to send fds: " + std::string(strerror(errno))));
128 }

Subscribers

People subscribed via source and target branches