From a5083ab7a76520ac0ed23804cec0e9ad62e00e24 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= <maze@google.com>
Date: Sun, 18 Dec 2022 21:46:55 +0000
Subject: [PATCH] qtaguid.cpp - improvements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Resolves a pair of TODO's, and makes a pair of error return
code paths not return null function pointers.

Note that:
  system/netd/client/NetdClient.cpp
implements this as:

int checkSocket(int socketFd) {
    if (socketFd < 0) {
        return -EBADF;
    }
    int family;
    socklen_t familyLen = sizeof(family);
    if (getsockopt(socketFd, SOL_SOCKET, SO_DOMAIN, &family, &familyLen) == -1) {
        return -errno;
    }
    if (!FwmarkClient::shouldSetFwmark(family)) {
        return -EAFNOSUPPORT;
    }
    return 0;
}

$define CHECK_SOCKET_IS_MARKABLE(sock) \
    do {                               \
        int err = checkSocket(sock);   \
        if (err) return err;           \
    } while (false)

extern "C" int tagSocket(int socketFd, uint32_t tag, uid_t uid) {
    CHECK_SOCKET_IS_MARKABLE(socketFd);
    FwmarkCommand command = {FwmarkCommand::TAG_SOCKET, 0, uid, tag};
    return FwmarkClient().send(&command, socketFd, nullptr);
}

extern "C" int untagSocket(int socketFd) {
    CHECK_SOCKET_IS_MARKABLE(socketFd);
    FwmarkCommand command = {FwmarkCommand::UNTAG_SOCKET, 0, 0, 0};
    return FwmarkClient().send(&command, socketFd, nullptr);
}

which means it *already* verifies that the passed in sockfd
is >= 0 and a socket via getsockopt(SOL_SOCKET, SO_DOMAIN),
as such the 'fcntl(sockfd, F_GETFD)' check is spurious.

Test: TreeHugger
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: I91ef68be5b0cc6b1972d514c13a76eaf834a3d5d
---
 libcutils/qtaguid.cpp | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/libcutils/qtaguid.cpp b/libcutils/qtaguid.cpp
index a987b855d..f8477828a 100644
--- a/libcutils/qtaguid.cpp
+++ b/libcutils/qtaguid.cpp
@@ -22,76 +22,60 @@
 
 #include <dlfcn.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <inttypes.h>
-#include <stdio.h>
-#include <string.h>
-#include <unistd.h>
 
 #include <log/log.h>
 
-class netdHandler {
-  public:
+struct netdHandler {
     int (*netdTagSocket)(int, uint32_t, uid_t);
     int (*netdUntagSocket)(int);
 };
 
-int stubTagSocket(int, uint32_t, uid_t) {
+static int stubTagSocket(int, uint32_t, uid_t) {
     return -EREMOTEIO;
 }
 
-int stubUntagSocket(int) {
+static int stubUntagSocket(int) {
     return -EREMOTEIO;
 }
 
-netdHandler initHandler(void) {
-    netdHandler handler = {stubTagSocket, stubUntagSocket};
+static netdHandler initHandler(void) {
+    const netdHandler stubHandler = { stubTagSocket, stubUntagSocket };
 
     void* netdClientHandle = dlopen("libnetd_client.so", RTLD_NOW);
     if (!netdClientHandle) {
         ALOGE("Failed to open libnetd_client.so: %s", dlerror());
-        return handler;
+        return stubHandler;
     }
 
+    netdHandler handler;
     handler.netdTagSocket = (int (*)(int, uint32_t, uid_t))dlsym(netdClientHandle, "tagSocket");
     if (!handler.netdTagSocket) {
         ALOGE("load netdTagSocket handler failed: %s", dlerror());
+        return stubHandler;
     }
 
     handler.netdUntagSocket = (int (*)(int))dlsym(netdClientHandle, "untagSocket");
     if (!handler.netdUntagSocket) {
         ALOGE("load netdUntagSocket handler failed: %s", dlerror());
+        return stubHandler;
     }
 
     return handler;
 }
 
 // The language guarantees that this object will be initialized in a thread-safe way.
-static netdHandler& getHandler() {
-    static netdHandler instance = initHandler();
+static const netdHandler& getHandler() {
+    static const netdHandler instance = initHandler();
     return instance;
 }
 
 int qtaguid_tagSocket(int sockfd, int tag, uid_t uid) {
-    // Check the socket fd passed to us is still valid before we load the netd
-    // client. Pass a already closed socket fd to netd client may let netd open
-    // the unix socket with the same fd number and pass it to server for
-    // tagging.
-    // TODO: move the check into netdTagSocket.
-    int res = fcntl(sockfd, F_GETFD);
-    if (res < 0) return res;
-
     ALOGV("Tagging socket %d with tag %u for uid %d", sockfd, tag, uid);
     return getHandler().netdTagSocket(sockfd, tag, uid);
 }
 
 int qtaguid_untagSocket(int sockfd) {
-    // Similiar to tag socket. We need a check before untag to make sure untag a closed socket fail
-    // as expected.
-    // TODO: move the check into netdTagSocket.
-    int res = fcntl(sockfd, F_GETFD);
-    if (res < 0) return res;
-
     ALOGV("Untagging socket %d", sockfd);
     return getHandler().netdUntagSocket(sockfd);
 }