PR feedback

Buildscripts/DevicetreeCompiler/source/generator.py

@@ -134,7 +134,7 @@ def write_device_structs(file, device: Device, parent_device: Device, bindings:
     file.write(f"\t.name = \"{device.node_name}\",\n") # Use original name
     file.write(f"\t.config = &{config_variable_name},\n")
     file.write(f"\t.parent = {parent_value},\n")
-    file.write(f"\t.internal = NULL\n")
+    file.write("\t.internal = NULL\n")
     file.write("};\n\n")
     # Child devices
     for child_device in device.devices:

TactilityKernel/Include/tactility/concurrent/mutex.h

@@ -18,12 +18,10 @@ struct Mutex {
 };
 
 inline static void mutex_construct(struct Mutex* mutex) {
-    assert(mutex->handle == NULL);
     mutex->handle = xSemaphoreCreateMutex();
 }
 
 inline static void mutex_destruct(struct Mutex* mutex) {
-    assert(mutex->handle != NULL);
     vPortAssertIfInISR();
     vSemaphoreDelete(mutex->handle);
     mutex->handle = NULL;

TactilityKernel/Include/tactility/device.h

@@ -20,19 +20,25 @@ struct DeviceInternal;
 
 /** Enables discovering devices of the same type */
 struct DeviceType {
-    /* Placeholder because empty structs have a different size with C vs C++ compilers */
+    const char* name;
-    uint8_t _;
 };
 
 /** Represents a piece of hardware */
 struct Device {
     /** The name of the device. Valid characters: a-z a-Z 0-9 - _ . */
     const char* name;
+
     /** The configuration data for the device's driver */
     const void* config;
+
     /** The parent device that this device belongs to. Can be NULL, but only the root device should have a NULL parent. */
     struct Device* parent;
-    /** Internal data */
+
+    /**
+     * Internal state managed by the kernel.
+     * Device implementers should initialize this to NULL.
+     * Do not access or modify directly; use device_* functions.
+     */
     struct DeviceInternal* internal;
 };
 

TactilityKernel/Include/tactility/driver.h

@@ -29,7 +29,11 @@ struct Driver {
     const struct DeviceType* device_type;
     /** The module that owns this driver. When it is NULL, the system owns the driver and it cannot be removed from registration. */
     const struct Module* owner;
-    /** Internal data */
+    /**
+     * Internal state managed by the kernel.
+     * Driver implementers should initialize this to NULL.
+     * Do not access or modify directly; use driver_* functions.
+     */
     struct DriverInternal* internal;
 };
 

TactilityKernel/Include/tactility/module.h

@@ -38,7 +38,6 @@ struct Module {
      * Desirable format "platform-esp32", "lilygo-tdeck", etc.
      */
     const char* name;
-
     /**
      * A function to initialize the module.
      * Should never be NULL.
@@ -46,21 +45,23 @@ struct Module {
      * @return ERROR_NONE if successful
      */
     error_t (*start)(void);
-
     /**
      * Deinitializes the module.
      * Should never be NULL.
      * @return ERROR_NONE if successful
      */
     error_t (*stop)(void);
-
     /**
      * A list of symbols exported by the module.
      * Should be terminated by MODULE_SYMBOL_TERMINATOR.
      * Can be a NULL value.
      */
     const struct ModuleSymbol* symbols;
-
+    /**
+     * Internal state managed by the kernel.
+     * Module implementers should initialize this to NULL.
+     * Do not access or modify directly; use module_* functions.
+     */
     struct ModuleInternal* internal;
 };
 

TactilityKernel/Source/device.cpp

@@ -15,19 +15,19 @@
 
 struct DeviceInternal {
     /** Address of the API exposed by the device instance. */
-    struct Driver* driver;
+    struct Driver* driver = nullptr;
     /** The driver data for this device (e.g. a mutex) */
-    void* driver_data;
+    void* driver_data = nullptr;
     /** The mutex for device operations */
-    struct Mutex mutex;
+    struct Mutex mutex {};
     /** The device state */
     struct {
-        int start_result;
+        int start_result = 0;
-        bool started : 1;
+        bool started : 1 = false;
-        bool added : 1;
+        bool added : 1 = false;
     } state;
     /** Attached child devices */
-    std::vector<Device*> children;
+    std::vector<Device*> children {};
 };
 
 struct DeviceLedger {
@@ -55,6 +55,9 @@ extern "C" {
 #define ledger_lock() mutex_lock(&ledger.mutex)
 #define ledger_unlock() mutex_unlock(&ledger.mutex)
 
+#define lock_internal(internal) mutex_lock(&internal->mutex)
+#define unlock_internal(internal) mutex_unlock(&internal->mutex)
+
 error_t device_construct(Device* device) {
     device->internal = new(std::nothrow) DeviceInternal;
     if (device->internal == nullptr) {
@@ -66,16 +69,24 @@ error_t device_construct(Device* device) {
 }
 
 error_t device_destruct(Device* device) {
-    if (device->internal->state.started || device->internal->state.added) {
+    lock_internal(device->internal);
+
+    auto* internal = device->internal;
+
+    if (internal->state.started || internal->state.added) {
+        unlock_internal(device->internal);
         return ERROR_INVALID_STATE;
     }
-    if (!device->internal->children.empty()) {
+    if (!internal->children.empty()) {
+        unlock_internal(device->internal);
         return ERROR_INVALID_STATE;
     }
     LOG_D(TAG, "destruct %s", device->name);
-    mutex_destruct(&device->internal->mutex);
+
-    delete device->internal;
     device->internal = nullptr;
+    mutex_unlock(&internal->mutex);
+    delete internal;
+
     return ERROR_NONE;
 }
 

TactilityKernel/Source/driver.cpp

@@ -13,22 +13,13 @@
 #define TAG "driver"
 
 struct DriverInternal {
-    Mutex mutex { 0 };
     int use_count = 0;
     bool destroying = false;
-
-    DriverInternal() {
-        mutex_construct(&mutex);
-    }
-
-    ~DriverInternal() {
-        mutex_destruct(&mutex);
-    }
 };
 
 struct DriverLedger {
     std::vector<Driver*> drivers;
-    Mutex mutex { 0 };
+    Mutex mutex {};
 
     DriverLedger() { mutex_construct(&mutex); }
     ~DriverLedger() { mutex_destruct(&mutex); }
@@ -37,16 +28,7 @@ struct DriverLedger {
     void unlock() { mutex_unlock(&mutex); }
 };
 
-static DriverLedger& get_ledger() {
+static DriverLedger ledger;
-    static DriverLedger ledger;
-    return ledger;
-}
-
-#define ledger get_ledger()
-
-#define get_driver_internal(driver) static_cast<DriverInternal*>(driver->internal)
-#define driver_lock(driver) mutex_lock(&get_driver_internal(driver)->mutex);
-#define driver_unlock(driver) mutex_unlock(&get_driver_internal(driver)->mutex);
 
 extern "C" {
 
@@ -59,21 +41,19 @@ error_t driver_construct(Driver* driver) {
 }
 
 error_t driver_destruct(Driver* driver) {
-    driver_lock(driver);
+    auto* internal = driver->internal;
-    // No module means the system owns it and it cannot be destroyed
+
     if (driver->owner == nullptr) {
-        driver_unlock(driver);
         return ERROR_NOT_ALLOWED;
     }
-    if (get_driver_internal(driver)->use_count != 0 || get_driver_internal(driver)->destroying) {
+    if (internal->use_count != 0 || internal->destroying) {
-        driver_unlock(driver);
         return ERROR_INVALID_STATE;
     }
-    get_driver_internal(driver)->destroying = true;
+    internal->destroying = true;
 
-    driver_unlock(driver);
+    // Nullify internal reference before deletion to prevent use-after-free
-    delete get_driver_internal(driver);
     driver->internal = nullptr;
+    delete internal;
 
     return ERROR_NONE;
 }
@@ -143,10 +123,8 @@ Driver* driver_find_compatible(const char* compatible) {
 }
 
 error_t driver_bind(Driver* driver, Device* device) {
-    driver_lock(driver);
-
     error_t error = ERROR_NONE;
-    if (get_driver_internal(driver)->destroying || !device_is_added(device)) {
+    if (driver->internal->destroying || !device_is_added(device)) {
         error = ERROR_INVALID_STATE;
         goto error;
     }
@@ -158,23 +136,18 @@ error_t driver_bind(Driver* driver, Device* device) {
         }
     }
 
-    get_driver_internal(driver)->use_count++;
+    driver->internal->use_count++;
-    driver_unlock(driver);
 
     LOG_I(TAG, "bound %s to %s", driver->name, device->name);
     return ERROR_NONE;
 
 error:
-
-    driver_unlock(driver);
     return error;
 }
 
 error_t driver_unbind(Driver* driver, Device* device) {
-    driver_lock(driver);
-
     error_t error = ERROR_NONE;
-    if (get_driver_internal(driver)->destroying || !device_is_added(device)) {
+    if (driver->internal->destroying || !device_is_added(device)) {
         error = ERROR_INVALID_STATE;
         goto error;
     }
@@ -186,16 +159,13 @@ error_t driver_unbind(Driver* driver, Device* device) {
         }
     }
 
-    get_driver_internal(driver)->use_count--;
+    driver->internal->use_count--;
-    driver_unlock(driver);
 
     LOG_I(TAG, "unbound %s from %s", driver->name, device->name);
 
     return ERROR_NONE;
 
 error:
-
-    driver_unlock(driver);
     return error;
 }
 

TactilityKernel/Source/drivers/gpio_controller.cpp

@@ -32,6 +32,8 @@ error_t gpio_controller_get_pin_count(struct Device* device, uint32_t* count) {
     return GPIO_DRIVER_API(driver)->get_pin_count(device, count);
 }
 
-extern const struct DeviceType GPIO_CONTROLLER_TYPE { 0 };
+extern const struct DeviceType GPIO_CONTROLLER_TYPE {
+    .name = "gpio-controller"
+};
 
 }

TactilityKernel/Source/drivers/i2c_controller.cpp

@@ -49,6 +49,8 @@ error_t i2c_controller_has_device_at_address(Device* device, uint8_t address, Ti
     return I2C_DRIVER_API(driver)->write(device, address, message, 2, timeout);
 }
 
-extern const struct DeviceType I2C_CONTROLLER_TYPE { 0 };
+extern const struct DeviceType I2C_CONTROLLER_TYPE {
+    .name = "i2c-controller"
+};
 
 }

TactilityKernel/Source/drivers/i2s_controller.cpp

@@ -32,6 +32,8 @@ error_t i2s_controller_reset(struct Device* device) {
     return I2S_DRIVER_API(driver)->reset(device);
 }
 
-extern const struct DeviceType I2S_CONTROLLER_TYPE { 0 };
+extern const struct DeviceType I2S_CONTROLLER_TYPE {
+    .name = "i2s-controller"
+};
 
 }

TactilityKernel/Source/module.cpp

@@ -1,19 +1,19 @@
-#include <vector>
+#include <cstring>
-#include <string.h>
 #include <algorithm>
+#include <new>
 #include <tactility/concurrent/mutex.h>
 #include <tactility/module.h>
-#include <new>
+#include <vector>
 
 #define TAG "module"
 
 struct ModuleInternal {
-    bool started;
+    bool started = false;
 };
 
 struct ModuleLedger {
     std::vector<struct Module*> modules;
-    struct Mutex mutex = { 0 };
+    struct Mutex mutex {};
 
     ModuleLedger() { mutex_construct(&mutex); }
     ~ModuleLedger() { mutex_destruct(&mutex); }
@@ -24,7 +24,7 @@ static ModuleLedger ledger;
 extern "C" {
 
 error_t module_construct(struct Module* module) {
-    module->internal = new (std::nothrow) ModuleInternal { .started = false };
+    module->internal = new (std::nothrow) ModuleInternal();
     if (module->internal == nullptr) return ERROR_OUT_OF_MEMORY;
     return ERROR_NONE;
 }
@@ -116,4 +116,3 @@ bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_add
 }
 
 }
-

Tests/TactilityKernel/Source/DeviceTest.cpp

@@ -26,7 +26,12 @@ TEST_CASE("device_construct and device_destruct should set and unset the interna
 }
 
 TEST_CASE("device_add should add the device to the list of all devices") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
     CHECK_EQ(device_construct(&device), ERROR_NONE);
     CHECK_EQ(device_add(&device), ERROR_NONE);
 
@@ -46,12 +51,18 @@ TEST_CASE("device_add should add the device to the list of all devices") {
 }
 
 TEST_CASE("device_add should add the device to its parent") {
-    Device parent = { 0 };
+    Device parent = {
+        .name = "parent",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
 
     Device child = {
-        .name = nullptr,
+        .name = "child",
         .config = nullptr,
-        .parent = &parent
+        .parent = &parent,
+        .internal = nullptr
     };
 
     CHECK_EQ(device_construct(&parent), ERROR_NONE);
@@ -79,7 +90,13 @@ TEST_CASE("device_add should add the device to its parent") {
 }
 
 TEST_CASE("device_add should set the state to 'added'") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
+
     CHECK_EQ(device_construct(&device), ERROR_NONE);
 
     CHECK_EQ(device_is_added(&device), false);
@@ -91,7 +108,13 @@ TEST_CASE("device_add should set the state to 'added'") {
 }
 
 TEST_CASE("device_remove should remove it from the list of all devices") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
+
     CHECK_EQ(device_construct(&device), ERROR_NONE);
     CHECK_EQ(device_add(&device), ERROR_NONE);
     CHECK_EQ(device_remove(&device), ERROR_NONE);
@@ -110,12 +133,18 @@ TEST_CASE("device_remove should remove it from the list of all devices") {
 }
 
 TEST_CASE("device_remove should remove the device from its parent") {
-    Device parent = { 0 };
+    Device parent = {
+        .name = "parent",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
 
     Device child = {
-        .name = nullptr,
+        .name = "child",
         .config = nullptr,
-        .parent = &parent
+        .parent = &parent,
+        .internal = nullptr
     };
 
     CHECK_EQ(device_construct(&parent), ERROR_NONE);
@@ -142,7 +171,13 @@ TEST_CASE("device_remove should remove the device from its parent") {
 }
 
 TEST_CASE("device_remove should clear the state 'added'") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
+
     CHECK_EQ(device_construct(&device), ERROR_NONE);
 
     CHECK_EQ(device_add(&device), ERROR_NONE);

Tests/TactilityKernel/Source/ModuleTest.cpp

@@ -64,6 +64,8 @@ TEST_CASE("Module lifecycle") {
         .internal = nullptr
     };
 
+    CHECK_EQ(module_construct(&module), ERROR_NONE);
+
     // 1. Successful start (no parent required anymore)
     CHECK_EQ(module_start(&module), ERROR_NONE);
     CHECK_EQ(module_is_started(&module), true);
@@ -104,6 +106,8 @@ TEST_CASE("Module lifecycle") {
     // Clean up: fix stop result so we can stop it
     test_stop_result = ERROR_NONE;
     CHECK_EQ(module_stop(&module), ERROR_NONE);
+
+    CHECK_EQ(module_destruct(&module), ERROR_NONE);
 }
 
 TEST_CASE("Global symbol resolution") {
@@ -120,6 +124,8 @@ TEST_CASE("Global symbol resolution") {
         .internal = nullptr
     };
 
+    REQUIRE_EQ(module_construct(&module), ERROR_NONE);
+
     uintptr_t addr;
     // Should fail as it is not added or started
     CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), false);
@@ -128,8 +134,8 @@ TEST_CASE("Global symbol resolution") {
     REQUIRE_EQ(module_start(&module), ERROR_NONE);
     // Still fails as symbols are null
     CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true);
-
     // Cleanup
     CHECK_EQ(module_remove(&module), ERROR_NONE);
+
     CHECK_EQ(module_destruct(&module), ERROR_NONE);
 }