Global: fixes several issues found by AI

src/detection/bluetooth/bluetooth_apple.m

@@ -36,7 +36,7 @@
 // ---------------------------------------------------------------------------------------------
 //
 // Core Bluetooth is the only public route to a peripheral that speaks Low Energy rather than BR/EDR.
-// What it will and will not answer was measured on macOS 27 with `.workbuddy-ai/probes/ble/le_probe`
+// What it will and will not answer was measured on macOS 27 with a standalone probe program
 // (12 paired devices, one of them connected), because none of it follows from the headers:
 //
 // * `retrieveConnectedPeripheralsWithServices:@[]` answers with **nothing** -- 0 devices in 0.1 ms.
@@ -88,7 +88,6 @@
 //   fills its peer map (0 entries after two seconds, `retrievePairedPeersWithOptions:` answers nil),
 //   and `CBCentralManager.sharedPairingAgent.retrievePairedPeers` answers with 7 of the 11 devices
 //   as bare `CBPeripheral` objects carrying no address, no battery and no class of device.
-//   `.workbuddy-ai/probes/ble/cb_only_probe.m` is the probe that measured this.
 //
 // * `CBPeripheral.state` is not the connection state the caller means. It answers `Disconnected` for
 //   every peripheral the retrieval above returns, and so does `isConnected`; only the private

src/detection/gamepad/gamepad_windows.c

@@ -97,23 +97,28 @@ static const char* detectKnownDeviceName(uint32_t vendorId, uint32_t productId)
 static bool readHidReport(HANDLE hHidFile, uint8_t* buffer, uint32_t length, DWORD timeoutMs, DWORD* nBytes) {
     OVERLAPPED overlapped = {};
     DWORD read;
-    if (ReadFile(hHidFile, buffer, length, &read, &overlapped)) {
-        *nBytes = read;
-        return true;
-    }
-    if (GetOverlappedResultEx(hHidFile, &overlapped, &read, timeoutMs, TRUE)) {
-        *nBytes = read;
-        return true;
+    if (!ReadFile(hHidFile, buffer, length, &read, &overlapped)) {
+        // A synchronous failure -- the device was unplugged, the handle went stale, access was
+        // denied -- does not set up `overlapped` at all, so it still looks like `STATUS_SUCCESS`
+        // and asking for the result below would report success with zero bytes read.
+        if (GetLastError() != ERROR_IO_PENDING) {
+            return false;
+        }
+
+        if (!GetOverlappedResultEx(hHidFile, &overlapped, &read, timeoutMs, TRUE)) {
+            // A timeout leaves the read pending, and the driver writes into `buffer` and `overlapped` when
+            // it eventually completes -- both of which are gone by then: the buffer belongs to the caller
+            // and the OVERLAPPED sits on this frame. So the read is cancelled by its own OVERLAPPED rather
+            // than by handle (CancelIo would take every other read on the same handle with it), and the
+            // cancellation is waited for before returning.
+            CancelIoEx(hHidFile, &overlapped);
+            GetOverlappedResult(hHidFile, &overlapped, &read, TRUE);
+            return false;
+        }
     }
 
-    // A timeout leaves the read pending, and the driver writes into `buffer` and `overlapped` when
-    // it eventually completes -- both of which are gone by then: the buffer belongs to the caller
-    // and the OVERLAPPED sits on this frame. So the read is cancelled by its own OVERLAPPED rather
-    // than by handle (CancelIo would take every other read on the same handle with it), and the
-    // cancellation is waited for before returning.
-    CancelIoEx(hHidFile, &overlapped);
-    GetOverlappedResult(hHidFile, &overlapped, &read, TRUE);
-    return false;
+    *nBytes = read;
+    return true;
 }
 
 // The DualShock 4 sends a battery level in its state packet, but only in the extended report it
@@ -266,7 +271,8 @@ static uint8_t detectSwitchBattery(HANDLE hHidFile, uint8_t* reportBuffer, uint3
         }
 
         DWORD nBytes;
-        if (!readHidReport(hHidFile, reportBuffer, reportLength, (DWORD) remaining, &nBytes)) {
+        // A report never arrives empty, so zero bytes means the read did not deliver one
+        if (!readHidReport(hHidFile, reportBuffer, reportLength, (DWORD) remaining, &nBytes) || !nBytes) {
             break;
         }
 
@@ -420,12 +426,14 @@ const char* ffDetectGamepad(FFlist* devices /* List of FFGamepadDevice */) {
                 DWORD nBytes;
                 switch (batteryKind) {
                     case FF_GAMEPAD_BATTERY_DS4:
-                        if (readHidReport(hHidFile, reportBuffer, caps.InputReportByteLength, FF_IO_TERM_RESP_WAIT_MS, &nBytes)) {
+                        // `reportBuffer` is uninitialised malloc() memory, so a read that delivered
+                        // nothing must not be parsed
+                        if (readHidReport(hHidFile, reportBuffer, caps.InputReportByteLength, FF_IO_TERM_RESP_WAIT_MS, &nBytes) && nBytes) {
                             device->battery = parseDs4Battery(reportBuffer, caps.InputReportByteLength);
                         }
                         break;
                     case FF_GAMEPAD_BATTERY_DUALSENSE:
-                        if (readHidReport(hHidFile, reportBuffer, caps.InputReportByteLength, FF_IO_TERM_RESP_WAIT_MS, &nBytes)) {
+                        if (readHidReport(hHidFile, reportBuffer, caps.InputReportByteLength, FF_IO_TERM_RESP_WAIT_MS, &nBytes) && nBytes) {
                             device->battery = parseDualSenseBattery(reportBuffer, caps.InputReportByteLength);
                         }
                         break;

src/detection/initsystem/initsystem_windows.c

@@ -25,7 +25,7 @@ const char* ffDetectInitSystem(FFInitSystemResult* result) {
     if (ffIsSystemBasicProcessInfoAvailable()) {
         // SYSTEM_BASICPROCESS_INFORMATION entries are much smaller than SYSTEM_PROCESS_INFORMATION ones,
         // so a modest buffer should be enough to contain all processes
-        SYSTEM_BASICPROCESS_INFORMATION buffer[1024];
+        SYSTEM_BASICPROCESS_INFORMATION buffer[1024] = {};
         NTSTATUS status = NtQuerySystemInformation(SystemBasicProcessInformation, buffer, sizeof(buffer), NULL);
         if (status != STATUS_INFO_LENGTH_MISMATCH && !NT_SUCCESS(status)) {
             goto fallback;

src/detection/media/media_android.c

@@ -828,6 +828,16 @@ static const char* androidDetectMediaSession(FFMediaResult* media, bool saveCove
     // artwork are fetched exactly once, for the winner.
     uint32_t tokens[FF_BINDER_MAX_HANDLES];
     uint32_t tokenCount = 0;
+    // Raised as soon as ffBinderTransact() has replied, because from that point on it is holding a
+    // strong reference to every handle it collected and the kernel has installed every descriptor it
+    // delivered into our own fd table. Any early return past that line has to give them back, so one
+    // cleanup attribute does it rather than a release call repeated at each exit.
+    //
+    // `replied` is what decides: ffBinderTransact() acquires the handles before it can fail -- it
+    // returns an error when the reply did not fit the caller buffer, and the tail that carries
+    // BC_ACQUIRE runs regardless -- so releasing without the flag would drop one reference too many.
+    bool replied = false;
+    [[gnu::cleanup(ffBinderAcquiredRelease)]] FFBinderAcquired acquired = { .binder = &binder, .replied = &replied };
     {
         uint8_t request[FF_MEDIA_ANDROID_REQUEST_SIZE];
         FFBinderParcel parcel = ffBinderParcelCreate(request, sizeof(request));
@@ -837,6 +847,7 @@ static const char* androidDetectMediaSession(FFMediaResult* media, bool saveCove
 
         FFBinderReply reply = ffBinderReplyCreate(s_reply, sizeof(s_reply));
         error = ffBinderTransact(&binder, service.handle, FF_MEDIA_ANDROID_GET_SESSIONS, 0, &parcel, &reply);
+        replied = true;
         if (error != nullptr) {
             return error;
         }
@@ -864,6 +875,13 @@ static const char* androidDetectMediaSession(FFMediaResult* media, bool saveCove
         for (uint32_t i = 0; i < tokenCount; i++) {
             tokens[i] = reply.handles[i];
         }
+        acquired.handles = tokens;
+        acquired.handleCount = tokenCount;
+        // Every descriptor the reply delivered is ours to close. The art of a session is read through
+        // one of them, and reading it happens over the fd number rather than over the file, so closing
+        // it on the way out does not cut the read short.
+        acquired.fds = reply.fds;
+        acquired.fdCount = reply.fdCount;
     }
 
     if (tokenCount == 0) {
@@ -889,13 +907,7 @@ static const char* androidDetectMediaSession(FFMediaResult* media, bool saveCove
 
     androidReadSession(&binder, tokens[best], media, saveCover);
 
-    // The tokens were acquired by ffBinderTransact() and are given back one by one; releasing them is
-    // what keeps a short-lived fastfetch from holding a session open for the life of the process.
-    for (uint32_t i = 0; i < tokenCount; i++) {
-        FFBinderServiceHandle token = { .binder = &binder, .handle = tokens[i] };
-        ffBinderServiceHandleRelease(&token);
-    }
-
+    // The tokens were acquired by ffBinderTransact() and are given back by the cleanup attribute above.
     return nullptr;
 }
 

src/detection/top/top.c

@@ -7,24 +7,12 @@ static double startTick;
 static FFTopTypes preparedTypes;
 
 void ffPrepareTopProcesses(FFTopTypes showTypes) {
-    if ((showTypes & (FF_TOP_TYPE_CPU | FF_TOP_TYPE_DISK)) == 0) {
-        return; // Memory usage is instantaneous; no baseline snapshot is needed
+    if (startTick != 0) {
+        return; // Already prepared
     }
 
-    if (startTick != 0) {
-        // Already prepared. The baseline above is one snapshot shared by every `top` module in the
-        // run, so a second module asking for a different set of counters would be measured against a
-        // snapshot that never collected them: its difference comes out as the process lifetime
-        // average rather than a rate, and the `newItem->... < oldItem->...` checks in
-        // `ffDetectTopProcesses` cannot tell, because the missing counters read as 0 there. This is
-        // a configuration fastfetch cannot serve, so it is refused rather than printed wrong.
-        if (showTypes != preparedTypes) {
-            fputs("Error: `top` modules with different `showTypes` cannot share a run\n", stderr);
-            fputs("       The baseline snapshot is collected once and reused, so it only holds the counters the first module asked for.\n", stderr);
-            fputs("       Give the modules the same `showTypes`, or run them in separate invocations.\n", stderr);
-            exit(1);
-        }
-        return;
+    if ((showTypes & (FF_TOP_TYPE_CPU | FF_TOP_TYPE_DISK)) == 0) {
+        return; // Memory usage is instantaneous; no baseline snapshot is needed
     }
 
     // Within one module `showTypes` cannot change between this call and `ffDetectTopProcesses`:
@@ -81,6 +69,13 @@ const char* ffDetectTopProcesses(FFTopOptions* options, FFlist* result) {
         return nullptr;
     }
 
+    // A baseline collected for a different set of counters is answered here rather than in
+    // ffPrepareTopProcesses, which has no way to report an error to the caller. The module that
+    // triggered the mismatch is the one that sees it; see ffPrepareTopProcesses.
+    if (options->showTypes != preparedTypes && (options->showTypes & (FF_TOP_TYPE_CPU | FF_TOP_TYPE_DISK)) != 0) {
+        return "`top` modules with different `showTypes` cannot share a run";
+    }
+
     // Memory usage and thread count are instantaneous; when neither CPU time nor disk IO
     // counters are requested, a single snapshot suffices and no sampling wait is needed.
     const bool sampleOnce = (options->showTypes & (FF_TOP_TYPE_CPU | FF_TOP_TYPE_DISK)) == 0;

src/detection/top/top_linux.c

@@ -68,7 +68,7 @@ static bool parseStat(const char* buffer, size_t length, FFTopProcessSnapshot* r
 }
 
 const char* ffTopGetProcessSnapshot(FFlist* snapshots, FFTopTypes showTypes) {
-    const long ticks = sysconf(_SC_CLK_TCK);
+    const long ticks = sysconf(_SC_CLK_TCK); // sysconf returns -1 ONLY if `_SC_CLK_TCK` is an invalid name
     const uint32_t pageSizeShift = instance.state.platform.sysinfo.pageSizeShift;
     FF_DEBUG("Scanning /proc: clk_tck=%ld, pageSizeShift=%u", ticks, (unsigned) pageSizeShift);
 

src/detection/wallpaper/wallpaper_android.c

@@ -49,9 +49,17 @@ typedef enum FFWallpaperAndroidWhich : int32_t {
 // methods as it likes -- this one dropped `getName`, whose code answers UNKNOWN_TRANSACTION here.
 #define FF_WALLPAPER_ANDROID_TRANSACTION_GET_WALLPAPER 4u
 
-// The request is the interface token, the package name, `which` and `userId`. The package name is the
-// longest thing in it, and the helper will not write more than 128 bytes of it.
-#define FF_WALLPAPER_ANDROID_PARCEL_SIZE 256
+// `ffAndroidGetOwnPackage` writes at most this many bytes, terminator included.
+#define FF_WALLPAPER_ANDROID_PACKAGE_SIZE 128
+
+// The request is the interface token, the package name, `which` and `userId`. A string16 costs
+// 4 + 2 * (length + 1) bytes padded to four, and the token puts three int32 in front of the
+// descriptor. Deriving the size from the two names rather than rounding it up is what keeps the
+// longest package name `ffAndroidGetOwnPackage` can produce from marking the parcel truncated --
+// that fails the whole detection, rather than degrading to a call without a name.
+#define FF_WALLPAPER_ANDROID_PARCEL_SIZE \
+    (12 + ((4 + 2 * sizeof(FF_WALLPAPER_ANDROID_DESCRIPTOR) + 3) & ~3u) \
+        + ((4 + 2 * FF_WALLPAPER_ANDROID_PACKAGE_SIZE + 3) & ~3u) + 8)
 
 // The reply carries the descriptor object plus, for this method, a Bundle of two ints: 108 bytes on
 // the device it was measured on, 80 when the wallpaper is a live one and the Bundle comes alone. Only
@@ -134,7 +142,7 @@ const char* ffDetectWallpaper(FFstrbuf* result) {
         return ffDetectWallpaperLinux(result);
     }
 
-    char package[128];
+    char package[FF_WALLPAPER_ANDROID_PACKAGE_SIZE];
     if (!ffAndroidGetOwnPackage(package, sizeof(package))) {
         return "Cannot determine the package name of this process";
     }

src/detection/wallpaper/wallpaper_apple.m

@@ -23,9 +23,16 @@ const char* detectFromPlist(FFstrbuf* result) {
         NSDictionary* choice = choices[0];
 
         NSArray* files = choice[@"Files"];
-        if (files.count > 0) {
-            NSString* file = files[0][@"relative"];
-            ffStrbufSetS(result, [NSURL URLWithString:file].path.UTF8String);
+        if ([files isKindOfClass:NSArray.class] && files.count > 0) {
+            NSDictionary* fileEntry = files[0];
+            // `relative` is read through a dictionary, which raises an unrecognized selector rather
+            // than returning nil when the entry is not one.
+            if ([fileEntry isKindOfClass:NSDictionary.class]) {
+                NSString* relative = fileEntry[@"relative"];
+                if ([relative isKindOfClass:NSString.class]) {
+                    ffStrbufSetS(result, [NSURL URLWithString:relative].path.UTF8String);
+                }
+            }
         }
 
         if (result->length == 0) {
@@ -127,6 +134,11 @@ const char* ffDetectWallpaper(FFstrbuf* result) {
     } else {
 #ifdef FF_HAVE_SQLITE3
         error = detectFromSQLite(result);
+        if (error) {
+            // The database only holds the user-picked picture, so it goes missing on a machine that
+            // never changed its wallpaper. NSWorkspace answers for the system default there.
+            error = detectFromNSWorkspace(result);
+        }
 #else
         error = detectFromNSWorkspace(result);
 #endif

src/detection/wifi/wifi_android.c

@@ -58,15 +58,29 @@
 // the fields the caller may not see removed -- `WifiInfo.makeCopy` drops the SSID, writes
 // `02:00:00:00:00:00` in place of the BSSID and the MAC address, and rewrites the network id to -1,
 // while the signal, the rates, the frequency, the IPv4 address and the supplicant state are carried
-// over untouched. It makes that copy for every caller that `enforceCanAccessScanResults` refuses,
-// which was measured here as a caller whose location permission had lapsed, next to the shell, which
-// holds one and reads the SSID. Nothing else about the reply moves, so the head is understood the
-// same way and everything the reply still says is reported: a connection that cannot be named is
-// still a connection, and the name is the only thing missing. The two names are reported as
-// `<redacted>` -- the same word the macOS backend uses for the same situation -- rather than as
-// nothing, because an empty SSID is what the module prints the interface state in place of, and the
-// reply did carry a signal, a channel and a rate worth showing. The placeholders the service writes
-// in their place are never reported as values. See parseConnectionInfo().
+// over untouched. It makes that copy for every caller that `enforceCanAccessScanResults` refuses.
+//
+// One route to a withheld name is a permission that can lapse. That copy is gated on location
+// permission -- `enforceCanAccessScanResults` asks for `ACCESS_FINE_LOCATION` -- which is a runtime
+// permission the user can revoke at any time, so a caller that once read the SSID keeps its Wi-Fi
+// permission and stops being told the name. Measured here as a caller whose location permission had
+// lapsed, next to the shell, which holds one and reads the SSID. Nothing else about the reply moves,
+// so the head is understood the same way and everything the reply still says is reported: a
+// connection that cannot be named is still a connection, and the name is the only thing missing.
+// The two names are reported as `<redacted>` -- the same word the macOS backend uses for the same
+// situation -- rather than as nothing, because an empty SSID is what the module prints the interface
+// state in place of, and the reply did carry a signal, a channel and a rate worth showing. The
+// placeholders the service writes in their place are never reported as values. See
+// parseConnectionInfo().
+//
+// A UID with no Wi-Fi permission at all takes a different route, and it is worth naming because it
+// is the one that looks like this one from the outside. There the first call raises EX_SECURITY (-1)
+// and the module fails outright: nothing is printed, `<redacted>` never appears, and the caller sees
+// "The Wifi service raised an exception". The reason is that the caller's UID is what the service
+// checks, so no later field of a reply that was never delivered can be recovered -- the two routes
+// are not a fallback chain. There is no degraded answer to give here and no route left in this file:
+// without the permission the service answers nothing at all, so the module cannot run rather than
+// run worse. The file header above says which UID holds the permission and why.
 //
 // The network id goes with them, and that one matters to this file: -1 is what it reads as "there is
 // no connection", so a redacted reply would otherwise be reported as a disconnected one. What
@@ -129,8 +143,9 @@
 //     from anywhere else -- sysfs and /proc/net are EACCES -- so the field stays empty rather than
 //     naming an interface that was guessed at. `inf.status` is still reported, from the radio.
 //   * `conn.ssid` and `conn.bssid` on a connection whose names the service withheld are reported as
-//     `<redacted>` instead, which is not empty: see the note above. They are left empty when there
-//     is no connection at all, where an absent name is the truth rather than a redaction.
+//     `<redacted>` instead, which is not empty: see the note above. They are the two fields a
+//     lapsed location permission takes away and nothing else. They are left empty when there is no
+//     connection at all, where an absent name is the truth rather than a redaction.
 
 #define FF_WIFI_ANDROID_SERVICE "wifi"
 #define FF_WIFI_ANDROID_DESCRIPTOR "android.net.wifi.IWifiManager"
@@ -634,7 +649,14 @@ static const char* parseConnectionInfo(const uint8_t* data, size_t size, FFWifiA
     }
     const int32_t exception = ffBinderReadI32(data, size, 0);
     if (exception != 0) {
-        FF_DEBUG("The Wifi service raised exception %d", exception);
+        // The two exceptions this call raises mean different things to a reader, and the code below
+        // can not tell them apart -- it only sees that the connection could not be read. -1 is
+        // EX_SECURITY, which is the service refusing a UID that does not hold ACCESS_WIFI_STATE, and
+        // that is the whole module failing rather than one field going missing. The reply with the
+        // names withheld is the *other* route: it is a successful reply, and it is handled below as
+        // a connection that can not be named, not as an error. Saying so here is what keeps the two
+        // from reading as one.
+        FF_DEBUG("The Wifi service raised exception %d%s", exception, exception == -1 ? " (EX_SECURITY): this UID has no ACCESS_WIFI_STATE, so no reply carries a connection" : "");
         return "The Wifi service raised an exception";
     }
     if (ffBinderReadU32(data, size, 4) == 0) {
@@ -792,9 +814,12 @@ static double wifiAndroidRate(int32_t specific, int32_t generic) {
 // The SSID and the BSSID as the module reports them: the value when the reply carried one, and
 // `<redacted>` when the service withheld it. A withheld name is not reported as nothing, because an
 // empty SSID is what the module prints the interface state in place of -- and the reply did carry a
-// signal, a channel and a rate that are worth showing. `withheld` is only ever true for a connection:
-// the placeholder the service writes in place of an address is also what an empty `WifiInfo` carries,
-// but a reply with no connection never reaches the point where the names are set.
+// signal, a channel and a rate that are worth showing. This is the whole of the degradation: what a
+// lapsed location permission costs the module is these two fields and nothing else, since the
+// service withholds them from the copy it makes and leaves the rest of the reply intact.
+// `withheld` is only ever true for a connection: the placeholder the service writes in place of an
+// address is also what an empty `WifiInfo` carries, but a reply with no connection never reaches the
+// point where the names are set.
 static const char* wifiName(const char* value, bool withheld) {
     if (value[0] != '\0') {
         return value;