commit fc3b35f7b391d7917d54b0c9fa331c5daf408702
author Jonathan Basseri <misterikkit@google.com> Thu Sep 14 15:58:16 2017 -0700
committer Jonathan Basseri <misterikkit@google.com> Thu Sep 14 18:16:12 2017 -0700
tree de8532e251f21b27f603617d73822537e4bdfdfa
parent c61286619ff79895129b26e0cc14a6c43aff960d

Fix use-after-free in XfrmControllerTest.

The test was capturing 'optval' and 'optlen' from setsockopt calls, but
by the time the code under test has returned, the memory pointed to by
optval is no longer valid.  Update the test to copy the data before it
is too late.

If you are confused about how the gTest API is used here, look at
SaveArg and SaveArgPointee here:
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.md#side-effects

Bug: 65495297
Test: runtest -x system/netd/server/netd_unit_test.cpp
Change-Id: I490485dc8e1ef3569234653fa63af1c6409c075e

server/XfrmControllerTest.cpp

diff --git a/server/XfrmControllerTest.cpp b/server/XfrmControllerTest.cpp
index 7064ca9..07db844 100644
--- a/server/XfrmControllerTest.cpp
+++ b/server/XfrmControllerTest.cpp
@@ -63,9 +63,11 @@
 using android::netdutils::Syscalls;
 
 using ::testing::DoAll;
+using ::testing::Invoke;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::SetArgPointee;
+using ::testing::WithArg;
 using ::testing::_;
 
 /**
@@ -304,7 +306,13 @@
 TEST_F(XfrmControllerTest, TestIpSecApplyTransportModeTransform) {
 
     int optlen = 0;
-    const void* optval;
+    Policy policy{};
+    // Need to cast from void* in order to "SaveArg" policy. Easier to invoke a
+    // lambda than to write a gMock action.
+    auto SavePolicy = [&policy](const void* value) {
+        policy = *reinterpret_cast<const Policy*>(value);
+    };
+
     XfrmController ctrl;
 
     struct sockaddr socketaddr;
@@ -316,7 +324,8 @@
         .WillOnce(DoAll(SetArgPointee<1>(socketaddr), Return(netdutils::status::ok)));
 
     EXPECT_CALL(mockSyscalls, setsockopt(_, _, _, _, _))
-        .WillOnce(DoAll(SaveArg<3>(&optval), SaveArg<4>(&optlen), Return(netdutils::status::ok)));
+        .WillOnce(DoAll(WithArg<3>(Invoke(SavePolicy)), SaveArg<4>(&optlen),
+                        Return(netdutils::status::ok)));
 
     Status res = ctrl.ipSecApplyTransportModeTransform(
         sock, 1 /* resourceId */, static_cast<int>(XfrmDirection::OUT),
@@ -325,17 +334,16 @@
     EXPECT_EQ(0, res.code());
     EXPECT_EQ(static_cast<int>(sizeof(Policy)), optlen);
 
-    const Policy* policy = reinterpret_cast<const Policy*>(optval);
-    EXPECT_EQ(1 /* resourceId */, static_cast<int>(policy->tmpl.reqid));
-    EXPECT_EQ(htonl(DROID_SPI), policy->tmpl.id.spi);
+    EXPECT_EQ(1 /* resourceId */, static_cast<int>(policy.tmpl.reqid));
+    EXPECT_EQ(htonl(DROID_SPI), policy.tmpl.id.spi);
 
     xfrm_address_t saddr{};
     inet_pton(AF_INET, "127.0.0.1", reinterpret_cast<void*>(&saddr));
-    EXPECT_EQ(0, memcmp(&saddr, &policy->tmpl.saddr, sizeof(xfrm_address_t)));
+    EXPECT_EQ(0, memcmp(&saddr, &policy.tmpl.saddr, sizeof(xfrm_address_t)));
 
     xfrm_address_t daddr{};
     inet_pton(AF_INET, "8.8.8.8", reinterpret_cast<void*>(&daddr));
-    EXPECT_EQ(0, memcmp(&daddr, &policy->tmpl.id.daddr, sizeof(xfrm_address_t)));
+    EXPECT_EQ(0, memcmp(&daddr, &policy.tmpl.id.daddr, sizeof(xfrm_address_t)));
 }
 
 TEST_F(XfrmControllerTest, TestIpSecDeleteSecurityAssociation) {