Possible Memory Leak in `EthDriver.set_rx_callback`

[owenthewizard]

Please take this with a massive grain of salt... It appears that buf is never getting freed here: https://docs.esp-rs.org/esp-idf-svc/src/esp_idf_svc/eth.rs.html#754-764 Minimal example:

use esp_idf_svc::eth::{EthDriver, RmiiEthChipset};
use esp_idf_svc::eventloop::EspSystemEventLoop;
use esp_idf_svc::hal::gpio;
use esp_idf_svc::hal::prelude::Peripherals;
use esp_idf_svc::sys::EspError;

fn main() -> Result<(), EspError> {
    // It is necessary to call this function once. Otherwise some patches to the runtime
    // implemented by esp-idf-sys might not link properly. See https://github.com/esp-rs/esp-idf-template/issues/71
    esp_idf_svc::sys::link_patches();

    // Bind the log crate to the ESP Logging facilities
    esp_idf_svc::log::EspLogger::initialize_default();

    let peripherals = Peripherals::take()?;
    let sysloop = EspSystemEventLoop::take()?;
    let pins = peripherals.pins;

    let mut eth = EthDriver::new_rmii(
        peripherals.mac,
        pins.gpio25, // RMII RDX0
        pins.gpio26, // RMII RDX1
        pins.gpio27, // RMII CRS DV
        pins.gpio23, // ETH01 SMI MDC
        pins.gpio22, // EMII TXD1
        pins.gpio21, // RMII TX EN
        pins.gpio19, // RMII TXD0
        pins.gpio18, // WT32-ETH01 SMI MDIO
        esp_idf_svc::eth::RmiiClockConfig::<gpio::Gpio0, gpio::Gpio16, gpio::Gpio17>::Input(
            pins.gpio0, // WT32-ETH01 external clock
        ),
        Some(pins.gpio16), // WT32-ETH01 PHY reset
        RmiiEthChipset::LAN87XX,
        Some(1), // WT32-ETH01 PHY address
        sysloop,
    )?;

    log::warn!("Setting eth promiscuous...");
    unsafe {
        use esp_idf_svc::handle::RawHandle;
        use esp_idf_svc::sys::{esp_eth_io_cmd_t_ETH_CMD_S_PROMISCUOUS, esp_eth_ioctl};
        let handle = eth.handle();
        let res = esp_eth_ioctl(
            handle,
            esp_eth_io_cmd_t_ETH_CMD_S_PROMISCUOUS,
            &true as *const bool as *mut _,
        );
        match EspError::convert(res) {
            Ok(()) => {
                log::warn!("eth promiscuous success!");
            }
            Err(e) => {
                log::error!("Failed to set eth promiscuous: {}", e);
                return Err(e);
            }
        }
    }

    eth.set_rx_callback(move |frame| {
        unsafe {
            use esp_idf_svc::sys::esp_get_free_heap_size;
            let free = esp_get_free_heap_size();
            log::warn!("Free heap: {}", free);
            log::trace!("{}", frame.len());
        }
        // uncomment this to fix heap exhaustion
        /*
        use esp_idf_svc::sys::free;
        unsafe {
            free(frame.as_ptr() as *mut _);
        }
        */
    })?;

    eth.start()?;

    // let eth run forever
    std::mem::forget(eth);

    Ok(())
}

Upon running the above and generating significant Ethernet traffic (I use packETH), note that the heap memory is being exhausted. Uncomment the unsafe section, and note that the free heap size remains constant (and a double free error isn't thrown). Other explanations: The memory is being re-allocated before being free'd

[owenthewizard]

In my testing the Wi-Fi rx callback does not suffer the same issue. I haven't tested Eth or Wifi tx callbacks.

[ivmarkov]

The problem is, the eth RX callback is badly documented, in that who owns the buffer is not even mentioned. So I assumed the usual rule where the caller of the callback owns the buffer applies, or else it is weird, as we have to free the buffer every time we are called back, which would mean the buffer is allocated for every eth frame, which is also weird.

But maybe that's the case after all. I think it is best to look at the "netif" lwip C source code to understand what it does w.r.t. eth and wifi buffer mgmt and then we should do the same.

[owenthewizard]

@ivmarkov Looking at the sta2eth example, it seems to me like the callbacks should be called with an owned buffer rather than a slice. Then, when the owned buffer is dropped, free(*eb) should be called. *buffer is valid until *eb is free'ed.

[owenthewizard]

Resolved in #406, merged 215363f.