BpfMap.h - hide dangerous stuff behind #ifdef BPF_MAP_MAKE_VISIBLE_FOR_TESTING
while we're at it:
- replace 'unique_fd != -1' with unique_fd.ok() which is
a test for fd.get() >= 0 and is thus effectively equivalent
- make use of the fact that unique_fd.reset()
takes care to save errno.
(see impl. in //system/libbase/include/android-base/unique_fd.h )
Bug: 236285127
Test: TreeHugger
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: I6fb7bf28a2265ad84baa3c552b39c620cb3875fe
diff --git a/staticlibs/native/bpf_headers/BpfMapTest.cpp b/staticlibs/native/bpf_headers/BpfMapTest.cpp
index d0737b0..10afe86 100644
--- a/staticlibs/native/bpf_headers/BpfMapTest.cpp
+++ b/staticlibs/native/bpf_headers/BpfMapTest.cpp
@@ -33,6 +33,7 @@
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
+#define BPF_MAP_MAKE_VISIBLE_FOR_TESTING
#include "bpf/BpfMap.h"
#include "bpf/BpfUtils.h"
diff --git a/staticlibs/native/bpf_headers/include/bpf/BpfMap.h b/staticlibs/native/bpf_headers/include/bpf/BpfMap.h
index afb6fdf..297b200 100644
--- a/staticlibs/native/bpf_headers/include/bpf/BpfMap.h
+++ b/staticlibs/native/bpf_headers/include/bpf/BpfMap.h
@@ -46,11 +46,15 @@
public:
BpfMap<Key, Value>() {};
+ // explicitly force no copy constructor, since it would need to dup the fd
+ // (later on, for testing, we still make available a copy assignment operator)
+ BpfMap<Key, Value>(const BpfMap<Key, Value>&) = delete;
+
protected:
// flag must be within BPF_OBJ_FLAG_MASK, ie. 0, BPF_F_RDONLY, BPF_F_WRONLY
BpfMap<Key, Value>(const char* pathname, uint32_t flags) {
mMapFd.reset(mapRetrieve(pathname, flags));
- if (mMapFd < 0) abort();
+ if (!mMapFd.ok()) abort();
if (isAtLeastKernelVersion(4, 14, 0)) {
if (bpfGetFdKeySize(mMapFd) != sizeof(Key)) abort();
if (bpfGetFdValueSize(mMapFd) != sizeof(Value)) abort();
@@ -60,10 +64,14 @@
public:
explicit BpfMap<Key, Value>(const char* pathname) : BpfMap<Key, Value>(pathname, 0) {}
+#ifdef BPF_MAP_MAKE_VISIBLE_FOR_TESTING
+ // All bpf maps should be created by the bpfloader, so this constructor
+ // is reserved for tests
BpfMap<Key, Value>(bpf_map_type map_type, uint32_t max_entries, uint32_t map_flags = 0) {
mMapFd.reset(createMap(map_type, sizeof(Key), sizeof(Value), max_entries, map_flags));
- if (mMapFd < 0) abort();
+ if (!mMapFd.ok()) abort();
}
+#endif
base::Result<Key> getFirstKey() const {
Key firstKey;
@@ -106,7 +114,7 @@
protected:
[[clang::reinitializes]] base::Result<void> init(const char* path, int fd) {
mMapFd.reset(fd);
- if (mMapFd == -1) {
+ if (!mMapFd.ok()) {
return ErrnoErrorf("Pinned map not accessible or does not exist: ({})", path);
}
if (isAtLeastKernelVersion(4, 14, 0)) {
@@ -135,13 +143,8 @@
[[clang::reinitializes]] base::Result<void> resetMap(bpf_map_type map_type,
uint32_t max_entries,
uint32_t map_flags = 0) {
- int map_fd = createMap(map_type, sizeof(Key), sizeof(Value), max_entries, map_flags);
- if (map_fd < 0) {
- auto err = ErrnoErrorf("Unable to create map.");
- mMapFd.reset();
- return err;
- };
- mMapFd.reset(map_fd);
+ mMapFd.reset(createMap(map_type, sizeof(Key), sizeof(Value), max_entries, map_flags));
+ if (!mMapFd.ok()) return ErrnoErrorf("Unable to create map.");
return {};
}
#endif
@@ -171,11 +174,15 @@
const base::unique_fd& getMap() const { return mMapFd; };
- // Copy assignment operator
+#ifdef BPF_MAP_MAKE_VISIBLE_FOR_TESTING
+ // Copy assignment operator - due to need for fd duping, should not be used in non-test code.
BpfMap<Key, Value>& operator=(const BpfMap<Key, Value>& other) {
if (this != &other) mMapFd.reset(fcntl(other.mMapFd.get(), F_DUPFD_CLOEXEC, 0));
return *this;
}
+#else
+ BpfMap<Key, Value>& operator=(const BpfMap<Key, Value>&) = delete;
+#endif
// Move assignment operator
BpfMap<Key, Value>& operator=(BpfMap<Key, Value>&& other) noexcept {
@@ -188,7 +195,12 @@
void reset(base::unique_fd fd) = delete;
- [[clang::reinitializes]] void reset(int fd = -1) {
+#ifdef BPF_MAP_MAKE_VISIBLE_FOR_TESTING
+ // Note that unique_fd.reset() carefully saves and restores the errno,
+ // and BpfMap.reset() won't touch the errno if passed in fd is negative either,
+ // hence you can do something like BpfMap.reset(systemcall()) and then
+ // check BpfMap.isValid() and look at errno and see why systemcall() failed.
+ [[clang::reinitializes]] void reset(int fd) {
mMapFd.reset(fd);
if ((fd >= 0) && isAtLeastKernelVersion(4, 14, 0)) {
if (bpfGetFdKeySize(mMapFd) != sizeof(Key)) abort();
@@ -196,8 +208,13 @@
if (bpfGetFdMapFlags(mMapFd) != 0) abort(); // TODO: fix for BpfMapRO
}
}
+#endif
- bool isValid() const { return mMapFd != -1; }
+ [[clang::reinitializes]] void reset() {
+ mMapFd.reset();
+ }
+
+ bool isValid() const { return mMapFd.ok(); }
base::Result<void> clear() {
while (true) {