search

espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.77k stars 7.31k forks source link

ESP-IDF V4.4 Crashes on I2C Link with very short commands. (IDFGH-6928) #8548

Closed not-my-real-name closed 2 years ago

not-my-real-name commented 2 years ago

Environment

Problem Description

Short or incomplete transactions on I2C crash the CPU due to trying to modify a null pointer. Extension of #8348

I think the code also needs a null check earlier on in this function before the while loop.

static void IRAM_ATTR i2c_master_cmd_begin_static(i2c_port_t i2c_num)
{
    i2c_obj_t *p_i2c = p_i2c_obj[i2c_num];
    portBASE_TYPE HPTaskAwoken = pdFALSE;
    i2c_cmd_evt_t evt = { 0 };
    if (p_i2c->cmd_link.head != NULL && p_i2c->status == I2C_STATUS_READ) {
        i2c_cmd_t *cmd = &p_i2c->cmd_link.head->cmd;
        i2c_hal_read_rxfifo(&(i2c_context[i2c_num].hal), cmd->data + cmd->bytes_used, p_i2c->rx_cnt);
        /* rx_cnt bytes have just been read, increment the number of bytes used from the buffer */
        cmd->bytes_used += p_i2c->rx_cnt;

        /* Test if there are still some remaining bytes to send. */
        if (cmd->bytes_used != cmd->total_bytes) {
            p_i2c->cmd_idx = 0;
        } else {
            p_i2c->cmd_link.head = p_i2c->cmd_link.head->next;
            if(p_i2c->cmd_link.head){              // <--- ADD NULL CHECK HERE 
                p_i2c->cmd_link.head->cmd.bytes_used = 0;
            }
        }

Expected Behavior

No crash when sending very short commands.

Actual Behavior

Crashes.

Steps to reproduce

I'm using two commands to split up one i2c transaction. I need to respond to data in the frame in order to determine how long my read needs to be after it has already begun. I seem to be encountering the same null head pointer issue at the very beginning of the function as well because my commands are unusually short.

Code to reproduce this issue

Example snippet:

uint8_t replyLength = 0;
i2c_master_start(cmd);                                                          
i2c_master_write_byte(cmd, i2cSlave << 1 | I2C_MASTER_READ, 1);                 //Write slave address
i2c_master_read_byte(cmd, &replyLength, I2C_MASTER_ACK);                        //Read length of bytes pending from slave

esp_err_t ret = i2c_master_cmd_begin(I2C_NUM_0, cmd, 100/portTICK_PERIOD_MS);   // Send the first half of the read.
i2c_cmd_link_delete(cmd); 

cmd = NULL;
esp_err_t ret2 = ESP_FAIL;

if(ret == ESP_OK){
         cmd = i2c_cmd_link_create();
         if(cmd){
        if(replyLength > maxLength){
            replyLength = maxLength;
        }
        i2c_master_read(cmd, buff, replyLength, I2C_MASTER_LAST_NACK);          //Read the number of pending bytes as indicated by the slave.
        i2c_master_stop(cmd);                                                   //Add a stop (finishes the split command).
        ret2 = i2c_master_cmd_begin(I2C_NUM_0, cmd, 100/portTICK_PERIOD_MS);    
        i2c_cmd_link_delete(cmd);
    }
}

Debug Logs

Guru Meditation Error: Core  0 panic'ed (StoreProhibited). Exception was unhandled.

Core  0 register dump:
PC      : 0x40084b5b  PS      : 0x00060031  A0      : 0x80084d2a  A1      : 0x3ffb0dd0
0x40084b5b: i2c_master_cmd_begin_static at <<my idf path>>/components/driver/i2c.c:1289

A2      : 0x00000000  A3      : 0x3ffda31c  A4      : 0x00000000  A5      : 0x00000000
A6      : 0x00000001  A7      : 0x00000001  A8      : 0x3ff53000  A9      : 0x3ffd9e70
A10     : 0x00000000  A11     : 0x00000023  A12     : 0x3ffb1dfc  A13     : 0x00000050
A14     : 0x3ffb1e00  A15     : 0x000000a0  SAR     : 0x0000001d  EXCCAUSE: 0x0000001d
EXCVADDR: 0x00000008  LBEG    : 0x40085e59  LEND    : 0x40085e6c  LCOUNT  : 0x000f4222
o-marshmallow commented 2 years ago

Hi @not-my-real-name ,

Thanks for reporting this, I am going to handle this.

AxelLin commented 2 years ago

@o-marshmallow

People keep hitting similar issue, https://github.com/espressif/esp-idf/issues/8543#issuecomment-1066713610 Any chance to speed up this bug fix and backport to stable branches ASAP?

AxelLin commented 2 years ago

@o-marshmallow

There are multiple issues in i2c now. Can you fix this one and backport to stale branches so we can excluding issues one-by-one. ref: https://github.com/espressif/esp-idf/issues/8543#issuecomment-1066713610

o-marshmallow commented 2 years ago

@AxelLin Yes, a fix is in progress of being merged

AxelLin commented 2 years ago

@AxelLin Yes, a fix is in progress of being merged

I still don't find the fix in recent update of master and v4.4 branch.

ginkgm commented 2 years ago

Backports created:

Target Branch Merged Supported Last N.A.
master (v5.0) 36e7ada null null
release/v4.4 9f2d407 v4.4.2 v4.4.1