InputTransport: store fd in a unique_fd.

Use unique_fd to hold the file descriptor, so that it gets protected
from being closed by someone else by fdsan.

Test: atest libinput_tests inputflinger_tests
Change-Id: I08df199294f9ddd7646c7bcd637b9c035e3c1e12

4 files changed, 70 insertions, 65 deletions

diff --git a/include/input/InputTransport.h b/include/input/InputTransport.h
index 690e0a11c8..28b8d80074 100644
--- a/include/input/InputTransport.h
+++ b/include/input/InputTransport.h
@@ -42,6 +42,8 @@
 #include <utils/Timers.h>
 #include <utils/Vector.h>
 
+#include <android-base/unique_fd.h>
+
 namespace android {
 class Parcel;
 
@@ -166,8 +168,7 @@ protected:
     virtual ~InputChannel();
 
 public:
-    InputChannel() = default;
-    InputChannel(const std::string& name, int fd);
+    static sp<InputChannel> create(const std::string& name, android::base::unique_fd fd);
 
     /* Creates a pair of input channels.
      *
@@ -177,7 +178,7 @@ public:
             sp<InputChannel>& outServerChannel, sp<InputChannel>& outClientChannel);
 
     inline std::string getName() const { return mName; }
-    inline int getFd() const { return mFd; }
+    inline int getFd() const { return mFd.get(); }
 
     /* Sends a message to the other endpoint.
      *
@@ -208,16 +209,15 @@ public:
     sp<InputChannel> dup() const;
 
     status_t write(Parcel& out) const;
-    status_t read(const Parcel& from);
+    static sp<InputChannel> read(const Parcel& from);
 
     sp<IBinder> getToken() const;
     void setToken(const sp<IBinder>& token);
 
 private:
-    void setFd(int fd);
-
+    InputChannel(const std::string& name, android::base::unique_fd fd);
     std::string mName;
-    int mFd = -1;
+    android::base::unique_fd mFd;
 
     sp<IBinder> mToken = nullptr;
 };
diff --git a/libs/input/IInputFlinger.cpp b/libs/input/IInputFlinger.cpp
index d6a73bfd27..90f6e09a69 100644
--- a/libs/input/IInputFlinger.cpp
+++ b/libs/input/IInputFlinger.cpp
@@ -92,15 +92,13 @@ status_t BnInputFlinger::onTransact(
     }
     case REGISTER_INPUT_CHANNEL_TRANSACTION: {
         CHECK_INTERFACE(IInputFlinger, data, reply);
-        sp<InputChannel> channel = new InputChannel();
-        channel->read(data);
+        sp<InputChannel> channel = InputChannel::read(data);
         registerInputChannel(channel);
         break;
     }
     case UNREGISTER_INPUT_CHANNEL_TRANSACTION: {
         CHECK_INTERFACE(IInputFlinger, data, reply);
-        sp<InputChannel> channel = new InputChannel();
-        channel->read(data);
+        sp<InputChannel> channel = InputChannel::read(data);
         unregisterInputChannel(channel);
         break;
     }
diff --git a/libs/input/InputTransport.cpp b/libs/input/InputTransport.cpp
index 2ff301e270..7835651f11 100644
--- a/libs/input/InputTransport.cpp
+++ b/libs/input/InputTransport.cpp
@@ -227,35 +227,28 @@ void InputMessage::getSanitizedCopy(InputMessage* msg) const {
 
 // --- InputChannel ---
 
-InputChannel::InputChannel(const std::string& name, int fd) :
-        mName(name) {
+sp<InputChannel> InputChannel::create(const std::string& name, android::base::unique_fd fd) {
+    const int result = fcntl(fd, F_SETFL, O_NONBLOCK);
+    if (result != 0) {
+        LOG_ALWAYS_FATAL("channel '%s' ~ Could not make socket non-blocking: %s", name.c_str(),
+                         strerror(errno));
+        return nullptr;
+    }
+    return new InputChannel(name, std::move(fd));
+}
+
+InputChannel::InputChannel(const std::string& name, android::base::unique_fd fd)
+      : mName(name), mFd(std::move(fd)) {
 #if DEBUG_CHANNEL_LIFECYCLE
     ALOGD("Input channel constructed: name='%s', fd=%d",
             mName.c_str(), fd);
 #endif
-
-    setFd(fd);
 }
 
 InputChannel::~InputChannel() {
 #if DEBUG_CHANNEL_LIFECYCLE
-    ALOGD("Input channel destroyed: name='%s', fd=%d",
-            mName.c_str(), mFd);
+    ALOGD("Input channel destroyed: name='%s', fd=%d", mName.c_str(), mFd.get());
 #endif
-
-    ::close(mFd);
-}
-
-void InputChannel::setFd(int fd) {
-    if (mFd > 0) {
-        ::close(mFd);
-    }
-    mFd = fd;
-    if (mFd > 0) {
-        int result = fcntl(mFd, F_SETFL, O_NONBLOCK);
-        LOG_ALWAYS_FATAL_IF(result != 0, "channel '%s' ~ Could not make socket "
-            "non-blocking.  errno=%d", mName.c_str(), errno);
-    }
 }
 
 status_t InputChannel::openInputChannelPair(const std::string& name,
@@ -276,13 +269,13 @@ status_t InputChannel::openInputChannelPair(const std::string& name,
     setsockopt(sockets[1], SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize));
     setsockopt(sockets[1], SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize));
 
-    std::string serverChannelName = name;
-    serverChannelName += " (server)";
-    outServerChannel = new InputChannel(serverChannelName, sockets[0]);
+    std::string serverChannelName = name + " (server)";
+    android::base::unique_fd serverFd(sockets[0]);
+    outServerChannel = InputChannel::create(serverChannelName, std::move(serverFd));
 
-    std::string clientChannelName = name;
-    clientChannelName += " (client)";
-    outClientChannel = new InputChannel(clientChannelName, sockets[1]);
+    std::string clientChannelName = name + " (client)";
+    android::base::unique_fd clientFd(sockets[1]);
+    outClientChannel = InputChannel::create(clientChannelName, std::move(clientFd));
     return OK;
 }
 
@@ -292,7 +285,7 @@ status_t InputChannel::sendMessage(const InputMessage* msg) {
     msg->getSanitizedCopy(&cleanMsg);
     ssize_t nWrite;
     do {
-        nWrite = ::send(mFd, &cleanMsg, msgLength, MSG_DONTWAIT | MSG_NOSIGNAL);
+        nWrite = ::send(mFd.get(), &cleanMsg, msgLength, MSG_DONTWAIT | MSG_NOSIGNAL);
     } while (nWrite == -1 && errno == EINTR);
 
     if (nWrite < 0) {
@@ -327,7 +320,7 @@ status_t InputChannel::sendMessage(const InputMessage* msg) {
 status_t InputChannel::receiveMessage(InputMessage* msg) {
     ssize_t nRead;
     do {
-        nRead = ::recv(mFd, msg, sizeof(InputMessage), MSG_DONTWAIT);
+        nRead = ::recv(mFd.get(), msg, sizeof(InputMessage), MSG_DONTWAIT);
     } while (nRead == -1 && errno == EINTR);
 
     if (nRead < 0) {
@@ -365,39 +358,44 @@ status_t InputChannel::receiveMessage(InputMessage* msg) {
 }
 
 sp<InputChannel> InputChannel::dup() const {
-    int fd = ::dup(getFd());
-    return fd >= 0 ? new InputChannel(getName(), fd) : nullptr;
+    android::base::unique_fd newFd(::dup(getFd()));
+    if (!newFd.ok()) {
+        ALOGE("Could not duplicate fd %i for channel %s: %s", getFd(), mName.c_str(),
+              strerror(errno));
+        return nullptr;
+    }
+    return InputChannel::create(mName, std::move(newFd));
 }
 
-
 status_t InputChannel::write(Parcel& out) const {
-    status_t s = out.writeString8(String8(getName().c_str()));
-
+    status_t s = out.writeCString(getName().c_str());
     if (s != OK) {
         return s;
     }
+
     s = out.writeStrongBinder(mToken);
     if (s != OK) {
         return s;
     }
 
-    s = out.writeDupFileDescriptor(getFd());
-
+    s = out.writeUniqueFileDescriptor(mFd);
     return s;
 }
 
-status_t InputChannel::read(const Parcel& from) {
-    mName = from.readString8();
-    mToken = from.readStrongBinder();
-
-    int rawFd = from.readFileDescriptor();
-    setFd(::dup(rawFd));
-
-    if (mFd < 0) {
-        return BAD_VALUE;
+sp<InputChannel> InputChannel::read(const Parcel& from) {
+    std::string name = from.readCString();
+    sp<IBinder> token = from.readStrongBinder();
+    android::base::unique_fd rawFd;
+    status_t fdResult = from.readUniqueFileDescriptor(&rawFd);
+    if (fdResult != OK) {
+        return nullptr;
     }
 
-    return OK;
+    sp<InputChannel> channel = InputChannel::create(name, std::move(rawFd));
+    if (channel != nullptr) {
+        channel->setToken(token);
+    }
+    return channel;
 }
 
 sp<IBinder> InputChannel::getToken() const {
diff --git a/libs/input/tests/InputChannel_test.cpp b/libs/input/tests/InputChannel_test.cpp
index f1675c0d36..af74edd65d 100644
--- a/libs/input/tests/InputChannel_test.cpp
+++ b/libs/input/tests/InputChannel_test.cpp
@@ -22,11 +22,12 @@
 #include <time.h>
 #include <errno.h>
 
+#include <binder/Binder.h>
 #include <gtest/gtest.h>
 #include <input/InputTransport.h>
-#include <utils/Timers.h>
 #include <utils/StopWatch.h>
 #include <utils/StrongPointer.h>
+#include <utils/Timers.h>
 
 namespace android {
 
@@ -43,20 +44,28 @@ TEST_F(InputChannelTest, ConstructorAndDestructor_TakesOwnershipOfFileDescriptor
     // of a pipe and to check for EPIPE on the other end after the channel is destroyed.
     Pipe pipe;
 
-    sp<InputChannel> inputChannel = new InputChannel("channel name", pipe.sendFd);
+    android::base::unique_fd sendFd(pipe.sendFd);
+
+    sp<InputChannel> inputChannel = InputChannel::create("channel name", std::move(sendFd));
 
+    EXPECT_NE(inputChannel, nullptr) << "channel should be successfully created";
     EXPECT_STREQ("channel name", inputChannel->getName().c_str())
             << "channel should have provided name";
-    EXPECT_EQ(pipe.sendFd, inputChannel->getFd())
-            << "channel should have provided fd";
+    EXPECT_NE(-1, inputChannel->getFd()) << "channel should have valid fd";
 
-    inputChannel.clear(); // destroys input channel
+    // InputChannel should be the owner of the file descriptor now
+    ASSERT_FALSE(sendFd.ok());
+}
 
-    EXPECT_EQ(-EPIPE, pipe.readSignal())
-            << "channel should have closed fd when destroyed";
+TEST_F(InputChannelTest, SetAndGetToken) {
+    Pipe pipe;
+    sp<InputChannel> channel =
+            InputChannel::create("test channel", android::base::unique_fd(pipe.sendFd));
+    EXPECT_EQ(channel->getToken(), nullptr);
 
-    // clean up fds of Pipe endpoints that were closed so we don't try to close them again
-    pipe.sendFd = -1;
+    sp<IBinder> token = new BBinder();
+    channel->setToken(token);
+    EXPECT_EQ(token, channel->getToken());
 }
 
 TEST_F(InputChannelTest, OpenInputChannelPair_ReturnsAPairOfConnectedChannels) {