Bug 1289718 - Fix Readlink, add tests. Wonky error handling. draft
authorGian-Carlo Pascutto <gcp@mozilla.com>
Mon, 05 Sep 2016 19:40:53 +0200
changeset 409939 97da480d8413efe100bca989fa9325b1944f1d78
parent 409938 ce977f89f04ba481c64ab6b3ffeaaf2591f5a8fa
child 409940 cc5e649bc384df81f2ed724162cced4878a1a6e1
push id28613
push usergpascutto@mozilla.com
push dateMon, 05 Sep 2016 18:00:21 +0000
bugs1289718
milestone51.0a1
Bug 1289718 - Fix Readlink, add tests. Wonky error handling. MozReview-Commit-ID: F8erB4Tvn2V
security/sandbox/linux/SandboxBrokerClient.cpp
security/sandbox/linux/broker/SandboxBroker.cpp
security/sandbox/linux/gtest/TestBroker.cpp
--- a/security/sandbox/linux/SandboxBrokerClient.cpp
+++ b/security/sandbox/linux/SandboxBrokerClient.cpp
@@ -123,29 +123,30 @@ SandboxBrokerClient::DoCall(const Reques
   if (recvd < 0) {
     return -recvErrno;
   }
   if (recvd == 0) {
     SANDBOX_LOG_ERROR("Unexpected EOF, op %d flags 0%o path %s",
                       aReq->mOp, aReq->mFlags, path);
     return -EIO;
   }
-  if (resp.mError != 0) {
+  // mError is positive for failure here, we invert the logic
+  if (resp.mError > 0) {
     // If the operation fails, the return payload will be empty;
     // adjust the iov_len for the following assertion.
     ios[1].iov_len = 0;
   }
   MOZ_ASSERT(static_cast<size_t>(recvd) <= ios[0].iov_len + ios[1].iov_len);
-  if (resp.mError == 0) {
+  if (resp.mError <= 0) {
     // Success!
     if (expectFd) {
       MOZ_ASSERT(openedFd >= 0);
       return openedFd;
     }
-    return 0;
+    return -resp.mError;
   }
   if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
     // Keep in mind that "rejected" files can include ones that don't
     // actually exist, if it's something that's optional or part of a
     // search path (e.g., shared libraries).  In those cases, this
     // error message is expected.
     SANDBOX_LOG_ERROR("Rejected errno %d op %d flags 0%o path %s",
                       resp.mError, aReq->mOp, aReq->mFlags, path);
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -675,25 +675,25 @@ SandboxBroker::ThreadMain(void)
           }
         }
         break;
 
       case SANDBOX_FILE_READLINK:
         if (permissive || AllowOperation(R_OK, perms)) {
           ssize_t respSize = DoReadlink(pathBuf, &respBuf, sizeof(respBuf));
           if (respSize >= 0) {
-            resp.mError = 0;
+            // Error codes are negated in the client
+            resp.mError = -respSize;
             ios[1].iov_base = &respBuf;
             ios[1].iov_len = respSize;
           } else {
             resp.mError = errno;
           }
         }
         break;
-
       }
     } else {
       MOZ_ASSERT(perms == 0);
       AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
     }
 
     const size_t numIO = ios[1].iov_len > 0 ? 2 : 1;
     DebugOnly<const ssize_t> sent = SendWithFd(respfd, ios, numIO, openedFd);
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@@ -76,16 +76,19 @@ protected:
     return mClient->Rename(aPath, bPath);
   }
   int Rmdir(const char* aPath) {
     return mClient->Rmdir(aPath);
   }
   int Unlink(const char* aPath) {
     return mClient->Unlink(aPath);
   }
+  ssize_t Readlink(const char* aPath, char* aBuff, size_t aSize) {
+    return mClient->Readlink(aPath, aBuff, aSize);
+  }
 
   virtual void SetUp() {
     ipc::FileDescriptor fd;
 
     mServer = SandboxBroker::Create(GetPolicy(), getpid(), fd);
     ASSERT_NE(mServer, nullptr);
     ASSERT_TRUE(fd.IsValid());
     auto rawFD = fd.ClonePlatformHandle();
@@ -372,16 +375,37 @@ TEST_F(SandboxBrokerTest, Unlink)
   ASSERT_GE(fd, 0) << "Opening /tmp/nope for writing failed.";
   close(fd);
   EXPECT_EQ(-EACCES, Unlink("/tmp/nope"));
 
   unlink("/tmp/blublu");
   rmdir("/tmp/blublu");
 }
 
+TEST_F(SandboxBrokerTest, Readlink)
+{
+  unlink("/tmp/blublu");
+  rmdir("/tmp/blublu");
+  unlink("/tmp/blublublu");
+
+  int fd = Open("/tmp/blublu", O_WRONLY | O_CREAT);
+  ASSERT_GE(fd, 0) << "Opening /tmp/blublu for writing failed.";
+  close(fd);
+  ASSERT_EQ(0, Symlink("/tmp/blublu", "/tmp/blublublu"));
+  EXPECT_EQ(0, Access("/tmp/blublublu", F_OK));
+  char linkBuff[256];
+  EXPECT_EQ(11, Readlink("/tmp/blublublu", linkBuff, sizeof(linkBuff)));
+  linkBuff[12] = '\0';
+  EXPECT_EQ(0, strcmp(linkBuff, "/tmp/blublu"));
+
+  unlink("/tmp/blublu");
+  rmdir("/tmp/blublu");
+  unlink("/tmp/blublublu");
+}
+
 TEST_F(SandboxBrokerTest, MultiThreadOpen) {
   RunOnManyThreads<SandboxBrokerTest,
                    &SandboxBrokerTest::MultiThreadOpenWorker>();
 }
 void SandboxBrokerTest::MultiThreadOpenWorker() {
   static const int kNumLoops = 10000;
 
   for (int i = 1; i <= kNumLoops; ++i) {