Bug 1289718 - Fix Readlink, add tests. Wonky error handling.
MozReview-Commit-ID: F8erB4Tvn2V
--- 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) {