Core panic if rc522 not connected

[Bolderaysky]

If rfid-rc522 not connected, even if checking return values of the API, core panic occures. Looking in the code, this is because of these functions:

static uint8_t* rc522_read_n(rc522_handle_t rc522, uint8_t addr, uint8_t n)
{
    uint8_t* buffer = (uint8_t*) malloc(n); // FIXME: memcheck
    esp_err_t ret;
    switch(rc522->config->transport) {
        case RC522_TRANSPORT_SPI:
            ret = rc522_spi_receive(rc522, buffer, n, addr);
            break;
        case RC522_TRANSPORT_I2C:
            ret = rc522_i2c_receive(rc522, buffer, n, addr);
            break;
        default:
            ESP_LOGE(TAG, "read: Unknown transport");
            ret = ESP_ERR_INVALID_STATE; // unknown transport
    }
    if(ESP_OK != ret) {
        free(buffer);
        buffer = NULL;
        ESP_LOGE(TAG, "Failed to read data (err: %s)", esp_err_to_name(ret));
    }
    return buffer;
}

static inline uint8_t rc522_read(rc522_handle_t rc522, uint8_t addr)
{
    uint8_t* buffer = rc522_read_n(rc522, addr, 1);
    uint8_t res = buffer[0];
    free(buffer);

    return res;
}

The most interesting here is that rc522_read_n will return NULL if rc522 not connected, but rc522_read dereferences returned pointer without checking if it's NULL in line

uint8_t res = buffer[0]; // buffer[0] is dereference of NULL

Also, if NULL returned, as we can see, rc522_read_n calls free to free the buffer, but rc522_read do the same on it's own, making another potential problem which is double free.

Fixing this will allow to repair in case rc522 not connected and will not cause esp32 to indefinetely restart in case of problems with sensor.

How this may look:

static inline uint8_t rc522_read(rc522_handle_t rc522, uint8_t addr)
{
    uint8_t* buffer = rc522_read_n(rc522, addr, 1);
    
    // If buffer is NULL, it's already freed and also we don't want to dereference it, so just returning some value indicating that there are some errors. All functions using this should check if return value equal to that 
    if (buffer == NULL) return 0;
    
    uint8_t res = buffer[0];
    free(buffer);

    return res;
}

[Bolderaysky]

This also would solve #18 which is the same problem basically, occuring on rc522_read_n

[abobija]

Hi @Bolderaysky

Thank you so much for taking the time to investigate this issue in detail. Your observations are correct, and the problems you pointed out are indeed concerning.

The potential for null pointer dereferences and double free certainly needs to be addressed for better code stability. Your proposed fix seems straightforward and efficient, following best practices.

Can you open a pull request to implement this fix? It would be a valuable contribution to the project.