From bba45429ae15aa0ce55f351c98abea8f53715982 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E9=80=9A=E6=B4=B2?= Date: Thu, 24 Sep 2026 21:21:02 +0800 Subject: [PATCH] Global: fixes issues found by AI --- CMakeLists.txt | 29 +++++- src/common/android/binder.c | 42 ++++++++- src/common/android/binder.h | 13 +++ src/common/android/dex.c | 90 ++++++++++++++----- src/common/impl/io_windows.c | 18 +++- src/common/impl/jsonconfig.c | 28 +++++- src/common/impl/networking_common.c | 23 +++-- src/common/impl/networking_linux.c | 12 ++- src/common/impl/networking_windows.c | 12 ++- src/common/io.h | 14 ++- src/common/networking.h | 5 +- src/common/windows/winrt.cpp | 13 ++- src/common/windows/winrt.hpp | 2 + src/detection/battery/battery_android.c | 11 +-- src/detection/bluetooth/bluetooth_apple.m | 7 ++ src/detection/gamepad/gamepad_windows.c | 16 +++- .../terminalfont/terminalfont_windows.c | 44 ++++++--- src/detection/top/top.c | 24 +++-- src/detection/wallpaper/wallpaper_apple.m | 5 +- src/detection/wifi/wifi_android.c | 29 ++++-- src/logo/image/aid.c | 12 +++ src/logo/image/image.c | 11 ++- src/options/logo.c | 4 + tests/networking.c | 3 +- 24 files changed, 387 insertions(+), 80 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a2c1681d6..6a7d4035a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -135,15 +135,30 @@ endif() # other leftovers, and makes `CONFIGURE_DEPENDS` react to added or removed sources. file(GLOB FF_MODULE_SRCS CONFIGURE_DEPENDS RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}/src/modules" "${CMAKE_CURRENT_SOURCE_DIR}/src/modules/*/*.c") set(FF_MODULE_DIRS "") +set(FF_MODULE_SRC_DIRS "") foreach(FF_MODULE_SRC ${FF_MODULE_SRCS}) get_filename_component(FF_MODULE_DIR "${FF_MODULE_SRC}" DIRECTORY) get_filename_component(FF_MODULE_NAME "${FF_MODULE_SRC}" NAME_WE) if("${FF_MODULE_DIR}" STREQUAL "${FF_MODULE_NAME}") list(APPEND FF_MODULE_DIRS "${FF_MODULE_DIR}") endif() + list(APPEND FF_MODULE_SRC_DIRS "${FF_MODULE_DIR}") endforeach() unset(FF_MODULE_SRCS) +# A directory of sources whose name no source carries registers no module at all, and nothing else +# in the build says so -- the module simply never appears. Reported rather than left to be noticed. +# Directories are compared rather than sources, so that a module which keeps helper sources next to +# its own `.c` does not warn about every one of them. +list(REMOVE_DUPLICATES FF_MODULE_SRC_DIRS) +foreach(FF_MODULE_DIR ${FF_MODULE_SRC_DIRS}) + if(NOT FF_MODULE_DIR IN_LIST FF_MODULE_DIRS) + message(WARNING "src/modules/${FF_MODULE_DIR}/ contains C sources but no ${FF_MODULE_DIR}.c, " + "so no module is registered for it. A module directory must hold a source file named after it.") + endif() +endforeach() +unset(FF_MODULE_SRC_DIRS) + foreach(FF_MODULE_DIR ${FF_MODULE_DIRS}) string(TOUPPER "${FF_MODULE_DIR}" FF_MODULE_UPPER) option(MODULE_DISABLE_${FF_MODULE_UPPER} "Disable module ${FF_MODULE_DIR}" OFF) @@ -520,6 +535,11 @@ if(ENABLE_IMAGE_LOGO) src/logo/image/im7.c src/logo/image/sixel.c ) + if(ANDROID) + list(APPEND LIBFASTFETCH_SRC + src/logo/image/aid.c + ) + endif() endif() foreach(FF_MODULE_DIR ${FF_MODULE_DIRS}) @@ -698,7 +718,6 @@ elseif(ANDROID) src/detection/de/de_linux.c src/detection/wmtheme/wmtheme_linux.c src/detection/camera/camera_android.c - src/logo/image/aid.c ) elseif(FreeBSD) list(APPEND LIBFASTFETCH_SRC @@ -2044,6 +2063,14 @@ elseif(ANDROID) # they have to be weak references: otherwise the executable would fail to load on an older device. # Guarding every use with __builtin_available() is what makes that safe, and turning the guard # diagnostic into an error is what keeps a use from slipping past it. + # + # camera_android.c is deliberately not in this list. The camera2 NDK is a *library* that starts at + # API 24, and a weak reference weakens the symbol, never the library: DT_NEEDED would still name + # libcamera2ndk.so, and a device below API 24 would fail to load fastfetch at all. The NDK's guide + # is explicit that there is no such thing as a weak library reference, and that dlopen is the only + # way to reach a library newer than minSdkVersion. Nothing here pins ANDROID_PLATFORM, so a build + # aimed below 24 would need that; on the supported path -- Termux, which targets 24 -- the library + # is always present and the plain call is correct. set_source_files_properties( ${CMAKE_CURRENT_SOURCE_DIR}/src/logo/image/aid.c ${CMAKE_CURRENT_SOURCE_DIR}/src/detection/codec/codec_android.c diff --git a/src/common/android/binder.c b/src/common/android/binder.c index 6d07a9147..519ba6e46 100644 --- a/src/common/android/binder.c +++ b/src/common/android/binder.c @@ -197,7 +197,13 @@ const char* ffBinderTransact(FFBinder* binder, uint32_t handle, uint32_t code, c .read_consumed = 0, .read_buffer = 0, }; - ioctl(binder->fd, BINDER_WRITE_READ, &release); + // This one ioctl carries the whole tail, so its failure is not cosmetic: without + // BC_ACQUIRE the handles handed to the caller are only weakly referenced and every + // later transaction on them fails with BR_FAILED_REPLY, which says nothing about the + // real cause. Reported, but only when nothing more specific is already at hand. + if (ioctl(binder->fd, BINDER_WRITE_READ, &release) != 0 && error == nullptr) { + error = "Releasing the binder reply failed"; + } return error; } @@ -235,3 +241,37 @@ const char* ffBinderLookupService(FFBinder* binder, const char* name, uint32_t t *handle = reply.handles[0]; return nullptr; } + +void ffBinderServiceHandleRelease(FFBinderServiceHandle* service) { + if (service->binder == nullptr || service->handle == 0) { + return; + } + + // The mirror of the acquire at the end of ffBinderTransact(): one strong and one weak reference + // each, dropped in the same order they were taken. The kernel keeps the service node alive until + // the last one goes, so this is what stops a short-lived fastfetch from holding one for the whole + // of its life. + const uint32_t commands[] = { BC_RELEASE, BC_DECREFS }; + uint8_t tail[2 * 2 * sizeof(uint32_t)]; + size_t tailSize = 0; + for (size_t i = 0; i < 2; i++) { + memcpy(tail + tailSize, &commands[i], sizeof(uint32_t)); + tailSize += sizeof(uint32_t); + memcpy(tail + tailSize, &service->handle, sizeof(uint32_t)); + tailSize += sizeof(uint32_t); + } + + struct binder_write_read release = { + .write_size = tailSize, + .write_consumed = 0, + .write_buffer = (binder_uintptr_t) (uintptr_t) tail, + .read_size = 0, + .read_consumed = 0, + .read_buffer = 0, + }; + // Not reported: this runs from a cleanup attribute, where there is no caller left to report to, + // and the only way for it to fail is a binder that is already dead -- which every earlier call on + // the same binder would have said already. + ioctl(service->binder->fd, BINDER_WRITE_READ, &release); + service->handle = 0; +} diff --git a/src/common/android/binder.h b/src/common/android/binder.h index 5ad0e1642..91dd25032 100644 --- a/src/common/android/binder.h +++ b/src/common/android/binder.h @@ -222,3 +222,16 @@ typedef struct FFBinderReply { // Resolves a service name to a handle through the service manager. Returns nullptr on success. [[gnu::nonnull(1, 2, 4), nodiscard]] const char* ffBinderLookupService(FFBinder* binder, const char* name, uint32_t transactionCode, uint32_t* handle); + +// A handle resolved through the service manager, held together with the binder it came from so that a +// single cleanup attribute can give it back on every path out of the function that took it. Zeroed -- +// or left at handle 0 by a lookup that has not run or did not succeed -- it is a no-op, which is what +// makes it safe to declare above the lookup itself. +typedef struct FFBinderServiceHandle { + FFBinder* binder; + uint32_t handle; +} FFBinderServiceHandle; + +// Returns the strong and weak references ffBinderLookupService() acquired on the handle. Usable as a +// cleanup attribute; safe on a zeroed or already released FFBinderServiceHandle. +[[gnu::nonnull(1)]] void ffBinderServiceHandleRelease(FFBinderServiceHandle* service); diff --git a/src/common/android/dex.c b/src/common/android/dex.c index 7fdb90458..3b8cdf36d 100644 --- a/src/common/android/dex.c +++ b/src/common/android/dex.c @@ -55,10 +55,37 @@ static uint32_t dexU32(const uint8_t* p) { return (uint32_t) p[0] | ((uint32_t) p[1] << 8) | ((uint32_t) p[2] << 16) | ((uint32_t) p[3] << 24); } -// LEB128, at most five bytes for the 32-bit values a dex stores this way. -static uint32_t dexUleb128(const uint8_t** p) { +// The dex header points at its tables with offsets that a corrupt or hostile file is free to set +// anywhere, including past the end of the mapping. Testing `end - p` alone does not catch that: once +// `p` is past `end` the subtraction is negative and casting it to size_t turns it into a huge value, +// so the comparison passes and the read goes off the end. Both directions have to be one test. +static bool dexInRange(const uint8_t* dex, const uint8_t* end, const uint8_t* p, size_t size) { + return p >= dex && p <= end && (size_t) (end - p) >= size; +} + +// Same, for a table of `count` elements of `elementSize` bytes. `count` is 64-bit because the +// callers add one to an index that came out of the file, and doing that in 32 bits can wrap to zero +// and make the check pass for an index that is not in the table. Dividing rather than multiplying is +// what keeps a hostile `count` from wrapping the size_t on a 32-bit build -- armeabi-v7a is one, and +// there `count * 4` overflows for any count above 0x3FFFFFFF and the check passes again. +static bool dexTableInRange(const uint8_t* dex, const uint8_t* end, const uint8_t* p, uint64_t count, size_t elementSize) { + if (p < dex || p > end) { + return false; + } + return (size_t) (end - p) / elementSize >= count; +} + +// LEB128, at most five bytes for the 32-bit values a dex stores this way. `end` bounds every read: +// the lengths and indexes decoded here come out of the file, so a truncated one must not walk off +// the end. A read that runs into `end` yields what was decoded so far, and the callers treat the +// resulting value as untrusted -- it is either compared against another count or used as an index +// that is range checked in turn. +static uint32_t dexUleb128(const uint8_t** p, const uint8_t* end) { uint32_t value = 0; for (int shift = 0; shift <= 28; shift += 7) { + if (*p >= end) { + break; + } const uint8_t byte = *(*p)++; value |= (uint32_t) (byte & 0x7f) << shift; if ((byte & 0x80) == 0) { @@ -71,12 +98,18 @@ static uint32_t dexUleb128(const uint8_t** p) { // encoded_value: one header byte holding ((size - 1) << 5) | type, then that many payload bytes. A // `static final int` constant is written as VALUE_INT, whose payload is a sign-extended // little-endian integer. Reading through 64 bits keeps every shift in range even if the size field -// is nonsense; the caller's length check is what rejects such a dex. -static int32_t dexEncodedInt(const uint8_t** p) { +// is nonsense; `end` is what keeps a truncated payload from running off the end of the mapping. +static int32_t dexEncodedInt(const uint8_t** p, const uint8_t* end) { + if (*p >= end) { + return 0; + } const uint8_t header = *(*p)++; const uint32_t size = (uint32_t) (header >> 5) + 1; uint64_t value = 0; for (uint32_t i = 0; i < size && i < 8; ++i) { + if (*p >= end) { + break; + } value |= (uint64_t) (*(*p)++) << (i * 8); } const uint32_t bits = size * 8; @@ -91,14 +124,17 @@ static int32_t dexEncodedInt(const uint8_t** p) { // bytes can be used as they are. static const char* dexString(const uint8_t* dex, const uint8_t* end, uint32_t index) { const uint8_t* ids = dex + dexU32(dex + FF_DEX_OFF_STRING_IDS); - if (ids < dex || (size_t) (end - ids) < (size_t) index * 4 + 4) { + // `string_ids_size` is not read: the check is that the entry `index` names lies inside the + // mapping, which is what the read below needs, and the table is the only thing that can say it + // without trusting a second field out of the same file. + if (!dexTableInRange(dex, end, ids, (uint64_t) index + 1, 4)) { return nullptr; } const uint8_t* p = dex + dexU32(ids + (size_t) index * 4); if (p < dex || p >= end) { return nullptr; } - (void) dexUleb128(&p); + (void) dexUleb128(&p, end); if (p >= end) { return nullptr; } @@ -123,7 +159,7 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla // The class's type index, so that the class_def_item and the field ids can be filtered by class. const uint8_t* types = dex + dexU32(dex + FF_DEX_OFF_TYPE_IDS); const uint32_t typeCount = dexU32(dex + FF_DEX_OFF_TYPE_IDS_SIZE); - if (types < dex || (size_t) (end - types) < (size_t) typeCount * 4) { + if (!dexTableInRange(dex, end, types, typeCount, 4)) { return "The dex type table is out of range"; } uint32_t typeIndex = UINT32_MAX; @@ -140,7 +176,7 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla const uint8_t* classes = dex + dexU32(dex + FF_DEX_OFF_CLASS_DEFS); const uint32_t classCount = dexU32(dex + FF_DEX_OFF_CLASS_DEFS_SIZE); - if (classes < dex || (size_t) (end - classes) < (size_t) classCount * FF_DEX_CLASS_DEF_SIZE) { + if (!dexTableInRange(dex, end, classes, classCount, FF_DEX_CLASS_DEF_SIZE)) { return "The dex class table is out of range"; } @@ -152,15 +188,16 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla const uint8_t* fields = dex + dexU32(classDef + FF_DEX_OFF_CLASS_DEF_DATA); const uint8_t* values = dex + dexU32(classDef + FF_DEX_OFF_CLASS_DEF_STATIC_VALUES); - if (fields < dex || fields >= end || values < dex || values >= end) { + // One byte each, so that the first uleb128 below has something to read. + if (!dexInRange(dex, end, fields, 1) || !dexInRange(dex, end, values, 1)) { return "The dex class data is out of range"; } - const uint32_t staticFields = dexUleb128(&fields); - (void) dexUleb128(&fields); // instance_fields_size - (void) dexUleb128(&fields); // direct_methods_size - (void) dexUleb128(&fields); // virtual_methods_size - const uint32_t staticValues = dexUleb128(&values); + const uint32_t staticFields = dexUleb128(&fields, end); + (void) dexUleb128(&fields, end); // instance_fields_size + (void) dexUleb128(&fields, end); // direct_methods_size + (void) dexUleb128(&fields, end); // virtual_methods_size + const uint32_t staticValues = dexUleb128(&values, end); if (staticFields != staticValues) { return "The dex static fields and values do not pair up"; } @@ -168,14 +205,20 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla const uint8_t* fieldIds = dex + dexU32(dex + FF_DEX_OFF_FIELD_IDS); uint32_t fieldIndex = 0; for (uint32_t j = 0; j < staticFields; ++j) { - if (fields >= end) { + // The two arrays are walked in step, so running out of either one is a truncation. + if (fields >= end || values >= end) { return "The dex static field list is truncated"; } - fieldIndex += dexUleb128(&fields); // field_idx_diff - (void) dexUleb128(&fields); // access_flags - const int32_t value = dexEncodedInt(&values); - if ((size_t) (end - fieldIds) < ((size_t) fieldIndex + 1) * FF_DEX_FIELD_ID_SIZE) { + const uint32_t fieldIndexDiff = dexUleb128(&fields, end); // field_idx_diff + if (fieldIndexDiff > UINT32_MAX - fieldIndex) { + return "The dex field index is out of range"; + } + fieldIndex += fieldIndexDiff; + (void) dexUleb128(&fields, end); // access_flags + const int32_t value = dexEncodedInt(&values, end); + + if (!dexTableInRange(dex, end, fieldIds, (uint64_t) fieldIndex + 1, FF_DEX_FIELD_ID_SIZE)) { return "The dex field table is out of range"; } const char* name = dexString(dex, end, dexU32(fieldIds + (size_t) fieldIndex * FF_DEX_FIELD_ID_SIZE + FF_DEX_OFF_FIELD_ID_NAME)); @@ -335,8 +378,6 @@ const char* ffDexStaticInt(const char* jarPath, const char* classDescriptor, con if (data == nullptr) { return "No dex in the jar"; } - } else if (method == FF_ZIP_METHOD_STORED) { - dataSize = uncompressedSize; } else if (method == FF_ZIP_METHOD_DEFLATED) { #ifdef FF_HAVE_ZLIB const char* error = inflateDex(data, dataSize, uncompressedSize, &mapping.inflated); @@ -348,9 +389,14 @@ const char* ffDexStaticInt(const char* jarPath, const char* classDescriptor, con #else return "The jar deflates classes.dex and fastfetch was built without zlib"; #endif - } else { + } else if (method != FF_ZIP_METHOD_STORED) { return "classes.dex uses an unsupported compression method"; } + // A STORED entry needs no further work: `dataSize` already holds the `compressedSize` that + // `findDexEntry` checked against the mapping, and for a STORED entry the payload is the file + // itself. The header's `uncompressedSize` is deliberately not used for it -- nothing bounds that + // value, so a corrupt one reaches past the end of the mapping, which is exactly what the dex + // header would then be validated against. mapping.data = data; mapping.size = dataSize; diff --git a/src/common/impl/io_windows.c b/src/common/impl/io_windows.c index e052b9f56..4f4b637ed 100644 --- a/src/common/impl/io_windows.c +++ b/src/common/impl/io_windows.c @@ -8,6 +8,20 @@ #include +// TODO: long paths on Windows. +// +// Every path that crosses into the wide API from this file is converted into a fixed +// `wchar_t[MAX_PATH]` buffer, and `RtlUTF8ToUnicodeN` refuses to fill it for a path of MAX_PATH +// characters or more (STATUS_BUFFER_TOO_SMALL) -- which makes each of those call sites a silent +// failure above that length rather than an error anyone sees. Raising a buffer on its own does not +// help: the wide APIs accept a longer path only either with a `\\?\` prefix on every absolute path +// (which also switches off the normalization the rest of the code relies on) or with a +// `longPathAware` manifest plus the LongPathsEnabled registry value. +// +// The plan is the second route: define MAX_PATH as 32768 program-wide, add the manifest flag, and +// size these buffers from the UTF-8 input rather than from the constant. It has to be done in one +// pass, which is why it is recorded here instead of being applied at the call sites the review +// happened to look at. static bool createSubfolders(wchar_t* fileName) { HANDLE hRoot = ffGetPeb()->ProcessParameters->CurrentDirectory.Handle; bool closeRoot = false; @@ -512,7 +526,7 @@ bool ffIsTerminal(int fd) { } bool ffRemoveFile(const char* fileName) { - wchar_t fileNameW[MAX_PATH]; + wchar_t fileNameW[MAX_PATH]; // see the long path TODO at the top of this file ULONG len; if (!NT_SUCCESS(RtlUTF8ToUnicodeN(fileNameW, (ULONG) sizeof(fileNameW), &len, fileName, (ULONG) strlen(fileName) + 1))) { return false; @@ -524,7 +538,7 @@ bool ffRemoveFile(const char* fileName) { } uint64_t ffPathGetMtime(const char* path) { - wchar_t fileNameW[MAX_PATH]; + wchar_t fileNameW[MAX_PATH]; // see the long path TODO at the top of this file ULONG len; if (!NT_SUCCESS(RtlUTF8ToUnicodeN(fileNameW, (ULONG) sizeof(fileNameW), &len, path, (ULONG) strlen(path) + 1))) { return 0; diff --git a/src/common/impl/jsonconfig.c b/src/common/impl/jsonconfig.c index 9b9316150..232d155e5 100644 --- a/src/common/impl/jsonconfig.c +++ b/src/common/impl/jsonconfig.c @@ -149,7 +149,10 @@ static bool parseModuleJsonObject(const char* type, yyjson_val* jsonVal, yyjson_ static void prepareModuleJsonObject(const char* type, yyjson_val* module) { switch (type[0]) { - #if !FF_MODULE_DISABLE_CPUUSAGE + // Both `cpuusage` and `command` start with a c, and they are disabled by separate options, + // so the block has to survive either one being off -- otherwise a build with only the other + // one enabled would never prepare this module (see the same guard in `commandoption.c`). + #if !FF_MODULE_DISABLE_CPUUSAGE || !FF_MODULE_DISABLE_COMMAND case 'c': case 'C': { if (ffStrEqualsIgnCase(type, ffCPUUsageModuleInfo.name)) { @@ -292,6 +295,7 @@ static const char* printJsonConfig(FFdata* data, bool prepare) { } yyjson_val* module = item; + bool gatedOnSucceeded = false; const char* type = yyjson_get_str(module); if (type) { module = nullptr; @@ -327,6 +331,8 @@ static const char* printJsonConfig(FFdata* data, bool prepare) { if (!unsafe_yyjson_is_bool(previousSucceeded)) { return "Property 'succeeded' in 'condition' must be a boolean"; } + gatedOnSucceeded = true; + if (prepare) { // Whether this module is printed at all depends on the result of the previous // one, which is only known in the print pass. Preparing it here would leave a @@ -337,6 +343,26 @@ static const char* printJsonConfig(FFdata* data, bool prepare) { // to be printed after all. continue; } + + #if !FF_MODULE_DISABLE_COMMAND + // A `command` module that runs in parallel is exactly the pairing above, so this + // combination cannot be left to the print pass: the module would be handed the + // result prepared for the next `command` module and print that instead of its + // own output. Refused here rather than in the prepare pass, because returning + // from there would leave every later module unprepared -- which the print pass + // would then report as a failure of theirs. + // `parallel` is on by default, so leaving the key out is the parallel case too. + if (gatedOnSucceeded && ffStrEqualsIgnCase(yyjson_get_str(yyjson_obj_get(module, "type")), ffCommandModuleInfo.name)) { + yyjson_val* parallel = yyjson_obj_get(module, "parallel"); + if (parallel == nullptr || yyjson_get_bool(parallel)) { + return "Module \"command\" cannot be combined with \"condition.succeeded\" while it runs in " + "parallel: whether it is printed is only known in the print pass, so it cannot be " + "prepared, and the prepared results are matched to modules by position. Set " + "\"parallel\": false on it, or drop the condition."; + } + } + #endif + if (succeeded != unsafe_yyjson_get_bool(previousSucceeded)) { continue; } diff --git a/src/common/impl/networking_common.c b/src/common/impl/networking_common.c index 949cc9ce0..732a011c9 100644 --- a/src/common/impl/networking_common.c +++ b/src/common/impl/networking_common.c @@ -325,7 +325,12 @@ int ffNetworkingChunkedComplete(const char* body, uint32_t bodyLen, uint32_t* co // Keeps the status line and every header except `dropHeader` and `Content-Length`, // then emits a `Content-Length` matching the (already decoded) body. // `body` may point into `buffer->chars`; it is copied before `buffer` is released. -static void rebuildResponse(FFstrbuf* buffer, uint32_t headerEnd, const char* body, uint32_t bodyLen, const char* dropHeader) { +// +// Returns the offset of the `\r` that opens the terminating CRLF CRLF of the new response. The +// rebuild drops a header line and rewrites another, so the header it produces is shorter than the +// one that went in: the offset the caller held before is no longer the one that describes this +// buffer, and handing it on would point into the middle of the body. +static uint32_t rebuildResponse(FFstrbuf* buffer, uint32_t headerEnd, const char* body, uint32_t bodyLen, const char* dropHeader) { FF_STRBUF_AUTO_DESTROY newBuffer = ffStrbufCreateA(headerEnd + bodyLen + 64); uint32_t pos = 0; @@ -349,18 +354,24 @@ static void rebuildResponse(FFstrbuf* buffer, uint32_t headerEnd, const char* bo ffStrbufDestroy(buffer); ffStrbufInitMove(buffer, &newBuffer); + + // The terminating CRLF CRLF is the four bytes in front of the body + return buffer->length - bodyLen - 4; } -bool ffNetworkingDecodeChunked(FFstrbuf* buffer, uint32_t headerEnd) { +bool ffNetworkingDecodeChunked(FFstrbuf* buffer, uint32_t* headerEnd) { assert(buffer->allocated > 0); + // Held in a local because the rebuild below hands a different value back + const uint32_t headerLength = *headerEnd; + // The header block is terminated by CR LF CR LF - if (headerEnd + 4 > buffer->length) { + if (headerLength + 4 > buffer->length) { return false; } - char* body = buffer->chars + headerEnd + 4; - uint32_t bodyLen = buffer->length - headerEnd - 4; + char* body = buffer->chars + headerLength + 4; + uint32_t bodyLen = buffer->length - headerLength - 4; uint32_t consumed = 0; if (ffNetworkingChunkedComplete(body, bodyLen, &consumed) != 1) { @@ -391,6 +402,6 @@ bool ffNetworkingDecodeChunked(FFstrbuf* buffer, uint32_t headerEnd) { } FF_DEBUG("Decoded chunked body: %u bytes encoded, %u bytes decoded", bodyLen, outLen); - rebuildResponse(buffer, headerEnd, out, outLen, "Transfer-Encoding:"); + *headerEnd = rebuildResponse(buffer, headerLength, out, outLen, "Transfer-Encoding:"); return true; } diff --git a/src/common/impl/networking_linux.c b/src/common/impl/networking_linux.c index f27aec670..f1ee80b33 100644 --- a/src/common/impl/networking_linux.c +++ b/src/common/impl/networking_linux.c @@ -466,7 +466,10 @@ const char* ffNetworkingRecvHttpResponse(FFNetworkingState* state, FFstrbuf* buf return "Response too large"; } FF_DEBUG("Receive buffer is full, extending it"); - ffStrbufEnsureFreeNoCheck(buffer, buffer->allocated); + // Asking for exactly the room that is left under the cap, rather than for as much as + // the buffer holds again: the allocation is rounded up to a power of two, so a doubling + // from a size that is not one of those lands past the cap the check above just cleared. + ffStrbufEnsureFreeNoCheck(buffer, FF_NETWORKING_MAX_RESPONSE_SIZE - buffer->length - 1); } // When the remaining length is known, ask for exactly that much. MSG_WAITALL then @@ -589,7 +592,7 @@ const char* ffNetworkingRecvHttpResponse(FFNetworkingState* state, FFstrbuf* buf return "No HTTP header end found"; } - if (chunked && !ffNetworkingDecodeChunked(buffer, headerEnd)) { + if (chunked && !ffNetworkingDecodeChunked(buffer, &headerEnd)) { return "Failed to decode chunked response"; } @@ -599,7 +602,10 @@ const char* ffNetworkingRecvHttpResponse(FFNetworkingState* state, FFstrbuf* buf } FF_DEBUG("Received valid HTTP 200 response, content %u bytes, total %u bytes", contentLength, buffer->length); - if (contentLength > 0 && buffer->length != contentLength + headerEnd + 4) { + // A chunked response was framed by its last-chunk and has been rewritten around it, so any + // `Content-Length` the server also sent describes a body that no longer exists. RFC 9112 6.3 + // says not to send both; when one does, the chunked framing is the one that was acted on. + if (!chunked && contentLength > 0 && buffer->length != contentLength + headerEnd + 4) { FF_DEBUG("Received content length mismatches: %u != %u", buffer->length, contentLength + headerEnd + 4); return "Content length mismatch"; } diff --git a/src/common/impl/networking_windows.c b/src/common/impl/networking_windows.c index f5ed6c400..1b6388c22 100644 --- a/src/common/impl/networking_windows.c +++ b/src/common/impl/networking_windows.c @@ -298,7 +298,10 @@ const char* ffNetworkingRecvHttpResponse(FFNetworkingState* state, FFstrbuf* buf return "Response too large"; } FF_DEBUG("Receive buffer is full, extending it"); - ffStrbufEnsureFreeNoCheck(buffer, buffer->allocated); + // Asking for exactly the room that is left under the cap, rather than for as much as + // the buffer holds again: the allocation is rounded up to a power of two, so a doubling + // from a size that is not one of those lands past the cap the check above just cleared. + ffStrbufEnsureFreeNoCheck(buffer, FF_NETWORKING_MAX_RESPONSE_SIZE - buffer->length - 1); } FF_DEBUG("Data reception loop #%d, current buffer size: %u, available space: %u", @@ -413,7 +416,7 @@ const char* ffNetworkingRecvHttpResponse(FFNetworkingState* state, FFstrbuf* buf return "No HTTP header end found"; } - if (chunked && !ffNetworkingDecodeChunked(buffer, headerEnd)) { + if (chunked && !ffNetworkingDecodeChunked(buffer, &headerEnd)) { return "Failed to decode chunked response"; } @@ -425,7 +428,10 @@ const char* ffNetworkingRecvHttpResponse(FFNetworkingState* state, FFstrbuf* buf contentLength, buffer->length); - if (contentLength > 0 && buffer->length != contentLength + headerEnd + 4) { + // A chunked response was framed by its last-chunk and has been rewritten around it, so any + // `Content-Length` the server also sent describes a body that no longer exists. RFC 9112 6.3 + // says not to send both; when one does, the chunked framing is the one that was acted on. + if (!chunked && contentLength > 0 && buffer->length != contentLength + headerEnd + 4) { FF_DEBUG("Received content length mismatches: %u != %u", buffer->length, contentLength + headerEnd + 4); return "Content length mismatch"; } diff --git a/src/common/io.h b/src/common/io.h index 6faaf4d66..78da571fc 100644 --- a/src/common/io.h +++ b/src/common/io.h @@ -132,12 +132,18 @@ static inline ssize_t ffReadFDData(FFNativeFD fd, size_t dataSize, void* data) { return ffReadFDData(fd, dataSize, data); } +// `CreateFileA` reads the path in the ANSI code page, so a UTF-8 path holding anything outside ASCII +// names a different file, or none at all. The wide API is the one that takes what UTF-8 decodes to, +// and the conversion is done the same way ffPathExists() below does it. [[gnu::nonnull(1, 2)]] static inline bool ffAppendFileBuffer(const char* fileName, FFstrbuf* buffer) { - FF_AUTO_CLOSE_FD FFNativeFD fd = -#ifndef _WIN32 - open(fileName, O_RDONLY | O_CLOEXEC); +#ifdef _WIN32 + wchar_t fileNameW[MAX_PATH]; + if (!NT_SUCCESS(RtlUTF8ToUnicodeN(fileNameW, (ULONG) sizeof(fileNameW), nullptr, fileName, (ULONG) strlen(fileName) + 1))) { + return false; + } + FF_AUTO_CLOSE_FD FFNativeFD fd = CreateFileW(fileNameW, GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); #else - CreateFileA(fileName, GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + FF_AUTO_CLOSE_FD FFNativeFD fd = open(fileName, O_RDONLY | O_CLOEXEC); #endif if (!ffIsValidNativeFD(fd)) { diff --git a/src/common/networking.h b/src/common/networking.h index c5c0e1626..645787bd0 100644 --- a/src/common/networking.h +++ b/src/common/networking.h @@ -60,7 +60,10 @@ typedef struct FFNetworkingState { // Decodes a `Transfer-Encoding: chunked` body in place and rewrites the response with a // `Content-Length` header in place of `Transfer-Encoding`. -[[gnu::nonnull(1), nodiscard]] bool ffNetworkingDecodeChunked(FFstrbuf* buffer, uint32_t headerEnd); +// `headerEnd` is updated in place: the rewrite changes the header length, so the value the caller +// passed in no longer describes the buffer afterwards. Callers must use the new one for anything +// that addresses the body, such as ffNetworkingDecompressGzip(). +[[gnu::nonnull(1, 2), nodiscard]] bool ffNetworkingDecodeChunked(FFstrbuf* buffer, uint32_t* headerEnd); // Result of parsing a `Transfer-Encoding` header value typedef enum FFNetworkingTransferEncoding { diff --git a/src/common/windows/winrt.cpp b/src/common/windows/winrt.cpp index 767fa626e..2acb41658 100644 --- a/src/common/windows/winrt.cpp +++ b/src/common/windows/winrt.cpp @@ -230,7 +230,15 @@ std::int32_t STDMETHODCALLTYPE FFWinrtHstringIterator::GetMany(std::uint32_t cap HSTRING* hstrings = reinterpret_cast(values); for (std::uint32_t i = 0; i < taken; ++i) { - WindowsDuplicateString(*(HSTRING*) ffListGet(&owner->strings, sizeof(HSTRING), index + i), &hstrings[i]); + HRESULT hr = WindowsDuplicateString(*(HSTRING*) ffListGet(&owner->strings, sizeof(HSTRING), index + i), &hstrings[i]); + if (FAILED(hr)) { + // Whatever was handed out already belongs to the caller, so a half filled array has to + // be taken back before the error goes out: the caller has no way to free a partial one. + for (std::uint32_t j = 0; j < i; ++j) { + WindowsDeleteString(hstrings[j]); + } + return hr; + } } index += taken; @@ -259,6 +267,9 @@ HRESULT ffWinrtCreateHstringIterable(const wchar_t* const* items, uint32_t count for (uint32_t i = 0; i < count; ++i) { HSTRING* slot = (HSTRING*) ffListAdd(&iterable->strings, sizeof(HSTRING)); + // ffListAdd does not zero the slot it hands out, and Release() walks every one of them, so + // the slot has to hold a usable HSTRING before WindowsCreateString gets a chance to fail. + *slot = nullptr; HRESULT hr = WindowsCreateString(items[i], (UINT32) ::wcslen(items[i]), slot); if (FAILED(hr)) { iterable->Release(); diff --git a/src/common/windows/winrt.hpp b/src/common/windows/winrt.hpp index 876a6d0d2..d1438870a 100644 --- a/src/common/windows/winrt.hpp +++ b/src/common/windows/winrt.hpp @@ -99,6 +99,8 @@ static HRESULT ffWaitForAsyncOperation(TOperationAbi* operation, TResultAbi** re template static HRESULT ffRunAndWait(TOperation&& operation, abi_t** result) { + *result = nullptr; + FF_AUTO_RELEASE_COM_OBJECT abi_t>* opResult = nullptr; HRESULT hr = operation(reinterpret_cast(&opResult)); if (FAILED(hr) || !opResult) { diff --git a/src/detection/battery/battery_android.c b/src/detection/battery/battery_android.c index 04f90530b..d750c07f1 100644 --- a/src/detection/battery/battery_android.c +++ b/src/detection/battery/battery_android.c @@ -172,23 +172,24 @@ static const char* parseBinder(FFlist* results) { return error; } - uint32_t handle = 0; - error = ffBinderLookupService(&binder, FF_BATTERY_ANDROID_SERVICE, FF_BINDER_SM_GET_SERVICE, &handle); + // Released on every path out of this function, including the ones below that return early. + [[gnu::cleanup(ffBinderServiceHandleRelease)]] FFBinderServiceHandle service = { .binder = &binder }; + error = ffBinderLookupService(&binder, FF_BATTERY_ANDROID_SERVICE, FF_BINDER_SM_GET_SERVICE, &service.handle); if (error != nullptr) { FF_DEBUG("Looking up the \"%s\" service failed: %s", FF_BATTERY_ANDROID_SERVICE, error); return error; } - FF_DEBUG("The \"%s\" service is handle %u", FF_BATTERY_ANDROID_SERVICE, handle); + FF_DEBUG("The \"%s\" service is handle %u", FF_BATTERY_ANDROID_SERVICE, service.handle); uint64_t capacity = 0; - error = getProperty(&binder, handle, FF_BATTERY_ANDROID_PROPERTY_CAPACITY, &capacity); + error = getProperty(&binder, service.handle, FF_BATTERY_ANDROID_PROPERTY_CAPACITY, &capacity); if (error != nullptr) { FF_DEBUG("The capacity is what failed, so no battery is reported at all"); return error; } uint64_t status = 0; - error = getProperty(&binder, handle, FF_BATTERY_ANDROID_PROPERTY_STATUS, &status); + error = getProperty(&binder, service.handle, FF_BATTERY_ANDROID_PROPERTY_STATUS, &status); if (error != nullptr) { FF_DEBUG("The status is what failed, so no battery is reported at all"); return error; diff --git a/src/detection/bluetooth/bluetooth_apple.m b/src/detection/bluetooth/bluetooth_apple.m index 4a782e4cd..1830efea7 100644 --- a/src/detection/bluetooth/bluetooth_apple.m +++ b/src/detection/bluetooth/bluetooth_apple.m @@ -271,6 +271,13 @@ static void setClassOfDevice(FFstrbuf* type, IOBluetoothDevice* ioDevice) { static FFBluetoothResult* findByName(FFlist* devices, const char* name) { FFBluetoothResult* found = nullptr; FF_LIST_FOR_EACH (FFBluetoothResult, device, *devices) { + // Only the classic half counts. The LE pass runs after the classic one and appends its + // entries to the same list, so the peripheral an earlier iteration of this very loop created + // would otherwise make its own name look ambiguous -- and the rule is there to keep one name + // from standing for two classic devices, not to count the LE copy of one of them. + if (!(device->deviceType & FF_BLUETOOTH_DEVICE_TYPE_CLASSIC_BIT)) { + continue; + } if (!ffStrEqualsIgnCase(device->name.chars, name)) { continue; } diff --git a/src/detection/gamepad/gamepad_windows.c b/src/detection/gamepad/gamepad_windows.c index 73719411c..3c95c2a5e 100644 --- a/src/detection/gamepad/gamepad_windows.c +++ b/src/detection/gamepad/gamepad_windows.c @@ -105,7 +105,14 @@ static bool readHidReport(HANDLE hHidFile, uint8_t* buffer, uint32_t length, DWO *nBytes = read; return true; } - CancelIo(hHidFile); + + // 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; } @@ -299,6 +306,13 @@ static FFGamepadBatteryKind detectBatteryKind(uint32_t vendorId, uint32_t produc return FF_GAMEPAD_BATTERY_NONE; } + // The Nintendo list is deliberately shorter than the name table above. The Charging Grip + // (0x200E) is a rail that charges Joy-Con and has no battery of its own, and the SNES + // online controller (0x2017) and the Switch 2 controllers (0x2066-0x2069) are ones SDL -- + // which is what this parsing follows -- reports no battery for: `SDL_hidapi_switch2.c` has + // no power reporting at all, and the SNES entry is commented out of `controller_list.h`. + // Reading the Switch state report from them would be a guess, so they keep the module's + // "unknown". case 0x057E: // Nintendo switch (productId) { case 0x2006: // Joy-Con L diff --git a/src/detection/terminalfont/terminalfont_windows.c b/src/detection/terminalfont/terminalfont_windows.c index 4fde0b6c2..1bd765d2b 100644 --- a/src/detection/terminalfont/terminalfont_windows.c +++ b/src/detection/terminalfont/terminalfont_windows.c @@ -80,6 +80,10 @@ static void applyWTProfile(yyjson_val* profile, uint8_t priority, FFTerminalFont // A fragment either defines a profile with "guid" or updates an existing one with "updates", // the latter being more important, so it is looked up first. // Note that "guid" and "updates" may be missing: yyjson_get_str() returns nullptr then. +// `fromUpdates` may be null: it is only useful to a caller that ranks the two apart, which a +// fragment does. settings.json does not -- an "updates" entry and a "guid" entry in profiles.list +// both sit at the same point of the inheritance chain, and the two passes above are what pick the +// right one between them. static yyjson_val* findWTProfileInArray(yyjson_val* profiles, const FFstrbuf* wtProfileId, bool* fromUpdates) { if (!yyjson_is_arr(profiles)) { return nullptr; @@ -91,7 +95,9 @@ static yyjson_val* findWTProfileInArray(yyjson_val* profiles, const FFstrbuf* wt yyjson_arr_foreach (profiles, idx, max, profile) { const char* id = yyjson_get_str(yyjson_obj_get(profile, pass == 0 ? "updates" : "guid")); if (id && ffStrbufEqualS(wtProfileId, id)) { - *fromUpdates = pass == 0; + if (fromUpdates) { + *fromUpdates = pass == 0; + } return profile; } } @@ -122,8 +128,7 @@ static const char* detectFromWTSettings(FFstrbuf* content, const FFstrbuf* wtPro } if (wtProfileId->length > 0) { - bool fromUpdates = false; - yyjson_val* profile = findWTProfileInArray(yyjson_obj_get(profiles, "list"), wtProfileId, &fromUpdates); + yyjson_val* profile = findWTProfileInArray(yyjson_obj_get(profiles, "list"), wtProfileId, nullptr); if (profile) { applyWTProfile(profile, WT_FONT_PRIORITY_PROFILE, result); } @@ -167,25 +172,40 @@ static void detectWTProfileFromFragmentsIn(const FFstrbuf* fragmentDir, const FF const uint32_t baseLength = path.length; ffStrbufAppendC(&path, '*'); - WIN32_FIND_DATAA entry; - FF_AUTO_CLOSE_DIR HANDLE hFind = FindFirstFileA(path.chars, &entry); + // The wide enumeration, because the directory above came from SHGetKnownFolderPath(): a profile + // whose name is not ASCII would otherwise hand FindFirstFileA a UTF-8 path that it reads as ANSI, + // and no fragment would be found at all. + wchar_t pathW[MAX_PATH]; + if (!NT_SUCCESS(RtlUTF8ToUnicodeN(pathW, (ULONG) sizeof(pathW), nullptr, path.chars, (ULONG) path.length + 1))) { + return; + } + + WIN32_FIND_DATAW entry; + FF_AUTO_CLOSE_DIR HANDLE hFind = FindFirstFileW(pathW, &entry); if (hFind == INVALID_HANDLE_VALUE) { return; } do { - if (!(entry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) || entry.cFileName[0] == '.') { + if (!(entry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) || entry.cFileName[0] == L'.') { continue; } ffStrbufSubstrBefore(&path, baseLength); - ffStrbufAppendS(&path, entry.cFileName); + ffStrbufAppendWS(&path, entry.cFileName); ffStrbufAppendC(&path, '\\'); const uint32_t appLength = path.length; ffStrbufAppendS(&path, "*.json"); - WIN32_FIND_DATAA fileEntry; - FF_AUTO_CLOSE_DIR HANDLE hFile = FindFirstFileA(path.chars, &fileEntry); + // The path changed, so the pattern has to be converted again. A name that cannot be converted + // is skipped rather than failing the whole scan: the next iteration rebuilds `path` from + // `baseLength` anyway. + if (!NT_SUCCESS(RtlUTF8ToUnicodeN(pathW, (ULONG) sizeof(pathW), nullptr, path.chars, (ULONG) path.length + 1))) { + continue; + } + + WIN32_FIND_DATAW fileEntry; + FF_AUTO_CLOSE_DIR HANDLE hFile = FindFirstFileW(pathW, &fileEntry); if (hFile != INVALID_HANDLE_VALUE) { do { if (fileEntry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { @@ -193,13 +213,13 @@ static void detectWTProfileFromFragmentsIn(const FFstrbuf* fragmentDir, const FF } ffStrbufSubstrBefore(&path, appLength); - ffStrbufAppendS(&path, fileEntry.cFileName); + ffStrbufAppendWS(&path, fileEntry.cFileName); applyWTFragmentFile(path.chars, wtProfileId, result); - } while (FindNextFileA(hFile, &fileEntry)); + } while (FindNextFileW(hFile, &fileEntry)); } ffStrbufSubstrBefore(&path, baseLength); - } while (FindNextFileA(hFind, &entry)); + } while (FindNextFileW(hFind, &entry)); } static void detectFromWTFragments(const FFstrbuf* wtProfileId, FFTerminalFontWT* result) { diff --git a/src/detection/top/top.c b/src/detection/top/top.c index 8b50991ec..a39aeb8a1 100644 --- a/src/detection/top/top.c +++ b/src/detection/top/top.c @@ -4,6 +4,7 @@ static FFlist first; static double startTick; +static FFTopTypes preparedTypes; void ffPrepareTopProcesses(FFTopTypes showTypes) { if ((showTypes & (FF_TOP_TYPE_CPU | FF_TOP_TYPE_DISK)) == 0) { @@ -11,13 +12,26 @@ void ffPrepareTopProcesses(FFTopTypes showTypes) { } if (startTick != 0) { - return; // Already prepared + // 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; } - // `showTypes` cannot change between this call and `ffDetectTopProcesses`: `ffPrepareCommandOption` - // and `parseStructureCommand` both build the options through `initStructureModuleOptions`, which - // merges the module object from the JSON config. So the baseline always matches what the second - // snapshot collects and needs no re-validation. + // Within one module `showTypes` cannot change between this call and `ffDetectTopProcesses`: + // `ffPrepareCommandOption` and `parseStructureCommand` both build the options through + // `initStructureModuleOptions`, which merges the module object from the JSON config, so the + // baseline always matches what the second snapshot collects. + preparedTypes = showTypes; ffListInit(&first); startTick = ffTimeGetTick(); ffTopGetProcessSnapshot(&first, showTypes); diff --git a/src/detection/wallpaper/wallpaper_apple.m b/src/detection/wallpaper/wallpaper_apple.m index 9e9d86356..b736ee864 100644 --- a/src/detection/wallpaper/wallpaper_apple.m +++ b/src/detection/wallpaper/wallpaper_apple.m @@ -1,6 +1,5 @@ #include "wallpaper.h" #include "common/settings.h" -#include "common/apple/osascript.h" #import #import @@ -8,7 +7,9 @@ const char* detectFromPlist(FFstrbuf* result) { // For Sonoma and later (macOS 14.0+) // https://github.com/JohnCoates/Aerial/issues/1332 - NSError* error; + // Initialised because the check below reads it: the method is documented to assign it, but an + // uninitialised read decides between two sources of the wallpaper, so it is not worth relying on. + NSError* error = nil; NSString* fileName = [NSString stringWithFormat:@"file://%s/Library/Application Support/com.apple.wallpaper/Store/Index.plist", instance.state.platform.homeDir.chars]; NSDictionary* dict = [NSDictionary dictionaryWithContentsOfURL:[NSURL URLWithString:fileName] error:&error]; diff --git a/src/detection/wifi/wifi_android.c b/src/detection/wifi/wifi_android.c index b746598f3..960bfdfc0 100644 --- a/src/detection/wifi/wifi_android.c +++ b/src/detection/wifi/wifi_android.c @@ -894,8 +894,21 @@ static const char* callIntMethod(FFBinder* binder, uint32_t handle, const char* return nullptr; } +// `getOwnPackage` writes at most this many bytes, terminator included. +#define FF_WIFI_ANDROID_PACKAGE_SIZE 128 + +// The parcel for `getConnectionInfo` is the interface token, the package name and the -1 that +// stands for a null `callingFeatureId`. 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 `getOwnPackage` can produce +// from marking the parcel truncated -- that fails the whole detection, rather than degrading to a +// call without a name. +#define FF_WIFI_ANDROID_PARCEL_SIZE \ + (12 + ((4 + 2 * sizeof(FF_WIFI_ANDROID_DESCRIPTOR) + 3) & ~3u) \ + + ((4 + 2 * FF_WIFI_ANDROID_PACKAGE_SIZE + 3) & ~3u) + 4) + static const char* detectWithBinder(FFlist* result) { - char package[128]; + char package[FF_WIFI_ANDROID_PACKAGE_SIZE]; if (!getOwnPackage(package, sizeof(package))) { return "Cannot determine the package name of this process"; } @@ -913,15 +926,17 @@ static const char* detectWithBinder(FFlist* result) { return error; } - uint32_t handle = 0; - error = ffBinderLookupService(&binder, FF_WIFI_ANDROID_SERVICE, FF_BINDER_SM_GET_SERVICE, &handle); + // Released on every path out of this function, including the ones below that return early: the + // references the lookup takes are the process's only claim on the service node. + [[gnu::cleanup(ffBinderServiceHandleRelease)]] FFBinderServiceHandle service = { .binder = &binder }; + error = ffBinderLookupService(&binder, FF_WIFI_ANDROID_SERVICE, FF_BINDER_SM_GET_SERVICE, &service.handle); if (error != nullptr) { return error; } FF_DEBUG("The \"%s\" service is handle %u, %s is transaction %d", - FF_WIFI_ANDROID_SERVICE, handle, FF_WIFI_ANDROID_GET_CONNECTION_INFO, transaction); + FF_WIFI_ANDROID_SERVICE, service.handle, FF_WIFI_ANDROID_GET_CONNECTION_INFO, transaction); - uint8_t parcelBuffer[256]; + uint8_t parcelBuffer[FF_WIFI_ANDROID_PARCEL_SIZE]; FFBinderParcel parcel = ffBinderParcelCreate(parcelBuffer, sizeof(parcelBuffer)); ffBinderParcelPutInterfaceToken(&parcel, FF_WIFI_ANDROID_DESCRIPTOR); ffBinderParcelPutString16(&parcel, package); @@ -930,7 +945,7 @@ static const char* detectWithBinder(FFlist* result) { uint8_t replyBuffer[FF_WIFI_ANDROID_REPLY_SIZE]; FFBinderReply reply = ffBinderReplyCreate(replyBuffer, sizeof(replyBuffer)); - error = ffBinderTransact(&binder, handle, (uint32_t) transaction, &parcel, &reply); + error = ffBinderTransact(&binder, service.handle, (uint32_t) transaction, &parcel, &reply); if (error != nullptr) { return error; } @@ -957,7 +972,7 @@ static const char* detectWithBinder(FFlist* result) { // that is up. WIFI_STATE_DISABLING is a radio that is still up, so only the one value means // down, and WIFI_STATE_UNKNOWN is the service declining to say. int32_t state = 0; - const char* stateError = callIntMethod(&binder, handle, FF_WIFI_ANDROID_GET_WIFI_ENABLED_STATE, &state); + const char* stateError = callIntMethod(&binder, service.handle, FF_WIFI_ANDROID_GET_WIFI_ENABLED_STATE, &state); if (stateError == nullptr && state != FF_WIFI_ANDROID_WIFI_STATE_UNKNOWN) { connection.up = state != FF_WIFI_ANDROID_WIFI_STATE_DISABLED; connection.upKnown = true; diff --git a/src/logo/image/aid.c b/src/logo/image/aid.c index 935269732..79eed1b42 100644 --- a/src/logo/image/aid.c +++ b/src/logo/image/aid.c @@ -10,6 +10,7 @@ #include #include #include +#include static inline bool androidImageDecoderError(const char** error, const char* message) { if (error) { @@ -206,6 +207,7 @@ bool ffImageCreateAID(FFLogoRequestData* requestData, FFImageBuffer* out, const typedef struct FFAndroidAnimation { AImageDecoder* decoder; + int fd; // the source the decoder keeps reading from while frames are decoded uint8_t* canvas; // the decoder's buffer; every frame is blended into what is already in it size_t stride; size_t size; @@ -272,6 +274,9 @@ FF_ANDROID_REQUIRES_API(31) static void androidAnimationDestroy(FFImageAnimation } AImageDecoder_delete(session->decoder); + // The decoder reads from the source, so the fd has to outlive every decode, which is why the + // session holds it rather than the function that opened it. + close(session->fd); free(session->canvas); free(session->delaysCs); free(session); @@ -389,6 +394,13 @@ bool ffImageAnimationOpenAID(FFLogoRequestData* requestData, FFImageAnimation** return androidImageDecoderError(error, "out of memory"); } + // Frames are decoded after this function returns, and the decoder reads from the source every + // time it does. The fd therefore has to outlive the function: it is handed to the session, + // which closes it in androidAnimationDestroy. Clearing the local is what keeps the cleanup + // attribute from closing it here -- every path above this line still relies on that attribute. + session->fd = fd; + fd = -1; + *out = animation; return true; } else { diff --git a/src/logo/image/image.c b/src/logo/image/image.c index c81a2aae6..121f9061e 100644 --- a/src/logo/image/image.c +++ b/src/logo/image/image.c @@ -1309,6 +1309,9 @@ static bool printCachedKittyAnimation(FFLogoRequestData* requestData) { FFKittyAnimation animation = {}; const char* error = "the cached animation is corrupt"; if (!parseKittyAnimation(content.chars, content.length, &animation, &error)) { + // The parser allocates the frame table before it can still fail, so what it got to allocate + // belongs to the caller either way. + destroyKittyAnimation(&animation); return false; } @@ -1317,7 +1320,7 @@ static bool printCachedKittyAnimation(FFLogoRequestData* requestData) { const FFOptionsLogo* options = &instance.config.logo; instance.state.logoWidth = requestData->logoCharacterWidth + options->paddingLeft + options->paddingRight; - instance.state.logoHeight = requestData->logoCharacterHeight + options->paddingTop; + instance.state.logoHeight = requestData->logoCharacterHeight + options->paddingTop - 1; printImageResult(requestData, &result); destroyKittyAnimation(&animation); @@ -1574,8 +1577,12 @@ static bool printCachedPixel(FFLogoRequestData* requestData) { } } + // The `- 1` matches printImagePixels and printImagePixelsNoCache, which print the image + // themselves: the cursor ends on the last row of the image rather than past it. A cached run and + // the run that filled the cache have to report the same height, or the logo shifts by one line + // between the first and every later run. instance.state.logoWidth = requestData->logoCharacterWidth + options->paddingLeft + options->paddingRight; - instance.state.logoHeight = requestData->logoCharacterHeight + options->paddingTop; + instance.state.logoHeight = requestData->logoCharacterHeight + options->paddingTop - 1; if (options->position != FF_LOGO_POSITION_TOP) { // Go to upper left corner diff --git a/src/options/logo.c b/src/options/logo.c index f4045569c..5a3de49e7 100644 --- a/src/options/logo.c +++ b/src/options/logo.c @@ -396,6 +396,10 @@ const char* ffOptionsParseLogoJsonConfig(FFOptionsLogo* options, yyjson_val* roo return "Property 'logo.cache' must be a boolean or the string \"regen\""; } continue; + } else if (unsafe_yyjson_equals_str(key, "recache")) { + // Kept as a named rejection so the message points at the replacement, which the generic + // "Unknown logo key" below can not do. + return "Property 'logo.recache' has been replaced by 'logo.cache' with the value \"regen\""; } else if (unsafe_yyjson_equals_str(key, "position")) { int value; const char* error = ffJsonConfigParseEnum(val, &value, (FFKeyValuePair[]) { diff --git a/tests/networking.c b/tests/networking.c index 18f0fc927..4d2a70862 100644 --- a/tests/networking.c +++ b/tests/networking.c @@ -73,7 +73,8 @@ int main(void) { FFstrbuf response = ffStrbufCreateS("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nhello\r\n0\r\n\r\n"); uint32_t headerEnd = (uint32_t) (strstr(response.chars, "\r\n\r\n") - response.chars); - VERIFY(ffNetworkingDecodeChunked(&response, headerEnd)); + VERIFY(ffNetworkingDecodeChunked(&response, &headerEnd)); + VERIFY(headerEnd == (uint32_t) (strstr(response.chars, "\r\n\r\n") - response.chars)); VERIFY(ffStrbufContainS(&response, "Content-Length: 5")); VERIFY(!ffStrbufContainS(&response, "Transfer-Encoding")); VERIFY(ffStrbufEndsWithS(&response, "hello"));