From 4f6573959b614cdf95cde48ace907debe421c32a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= <maze@google.com>
Date: Mon, 20 Jan 2020 15:06:22 -0800
Subject: [PATCH] make BpfMap.reset() harder to use
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Long term we don't even want reset() to exist.

It's simply not useful since only the bpfloader can create maps,
and thus all we can do is construct from pre-existing pinned bpf
path - and we should never destroy/unpin/delete these.

Similarly the move constructor is probably spurious.

Basically the only real use for reset() is in tests,
and even there the overall utility is not clear.

Hence, in the mean time, make it harder to use incorrectly,
and harder to use in general.

To do that we get rid of reset() -> you must call reset(-1)
and we eliminate the ability to call reset(unique_fd)
which has unclear fd-leaking semantics.

Also remove a spot where it is called spuriously.

Test: build, atest
Bug: 129773125
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: Ia018224aca0005ab68809b350595485ccd3f1bd3
---
 libbpf_android/BpfMapTest.cpp       | 2 +-
 libbpf_android/include/bpf/BpfMap.h | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/libbpf_android/BpfMapTest.cpp b/libbpf_android/BpfMapTest.cpp
index 6c28c05..a3c9004 100644
--- a/libbpf_android/BpfMapTest.cpp
+++ b/libbpf_android/BpfMapTest.cpp
@@ -140,7 +140,7 @@ TEST_F(BpfMapTest, reset) {
     uint32_t value_write = TEST_VALUE1;
     writeToMapAndCheck(testMap, key, value_write);
 
-    testMap.reset();
+    testMap.reset(-1);
     checkMapInvalid(testMap);
     ASSERT_GT(0, findMapEntry(testMap.getMap(), &key, &value_write));
     ASSERT_EQ(EBADF, errno);
diff --git a/libbpf_android/include/bpf/BpfMap.h b/libbpf_android/include/bpf/BpfMap.h
index 4b941a8..243d9dc 100644
--- a/libbpf_android/include/bpf/BpfMap.h
+++ b/libbpf_android/include/bpf/BpfMap.h
@@ -131,12 +131,12 @@ class BpfMap {
     // Move constructor
     void operator=(BpfMap<Key, Value>&& other) noexcept {
         mMapFd = std::move(other.mMapFd);
-        other.reset();
+        other.reset(-1);
     }
 
-    void reset(int fd = -1) {
-        mMapFd.reset(fd);
-    }
+    void reset(base::unique_fd fd) = delete;
+
+    void reset(int fd) { mMapFd.reset(fd); }
 
     bool isValid() const { return mMapFd != -1; }
 
@@ -172,7 +172,6 @@ template <class Key, class Value>
 base::Result<void> BpfMap<Key, Value>::init(const char* path) {
     mMapFd = base::unique_fd(mapRetrieve(path, 0));
     if (mMapFd == -1) {
-        reset();
         return base::ErrnoErrorf("Pinned map not accessible or does not exist: ({})", path);
     }
     return {};