use-after-free error in Wire library

[nigelb]

Subject of the issue

use-after-free error in Wire library.

The relevant methods are Wire.begin:

Arduino_Apollo3/libraries/Wire/src/Wire.cpp

Lines 14 to 19 in 4e24a02

	void arduino::MbedI2C::begin() {
	if(!master){
	master = new mbed::I2C((PinName)_sda, (PinName)_scl);
	setClock(100000); //Default to 100kHz
	}
	}

and Wire.end:

Arduino_Apollo3/libraries/Wire/src/Wire.cpp

Lines 31 to 40 in 4e24a02

	void arduino::MbedI2C::end() {
	if (master != NULL) {
	delete master;
	}
	#ifdef DEVICE_I2CSLAVE
	if (slave != NULL) {
	delete slave;
	}
	#endif
	}

With the sequence:

    Wire.begin();
    Wire.end();
    Wire.begin();
   //I2C Activity

After the call to Wire.end, master is pointing to the location of the deleted mbed::I2C object.

Your workbench

• RedBoard-Artemis-ATP
• Sparkfun Arduino_Apollo3 2.2.0, Arduino 1.8.13
• TMP117 connected via QWIIC connector.
• Powered via the USB-C plug.

Steps to reproduce

Example Code

#include "Wire.h"

arduino::MbedI2C *myWire;

void setup() {
  Serial.begin(115200);
  Serial.println("Wire Use-After-Free");
  Wire.end();
  myWire = new arduino::MbedI2C(40, 39);
}

void loop() {
  Serial.println("Loop");
  myWire->begin();
  Serial.println("Begin succeeded!");
  
  Serial.println("Starting I2C transmission");
  myWire->beginTransmission(0x48); //TMP117
  myWire->endTransmission();
  Serial.println("I2C transmission succeeded!");
  
  myWire->end();
}

Example Code Output

Wire Use-After-Free
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission

++ MbedOS Fault Handler ++

FaultType: HardFault

Context:
R0: 1000A750
R1: 90
R2: 1000A584
R3: A3009150
R4: 1000A750
R5: 0
R6: 0
R7: 0
R8: 0
R9: 0
R10: 0
R11: 0
R12: 23949
SP   : 10006E48
LR   : 10F2D
PC   : A3009150
xPSR : 0
PSP  : 10006DE0
MSP  : 1005FF70
CPUID: 410FC241
HFSR : 40000000
MMFSR: 1
BFSR : 0
UFSR : 0
DFSR : 0
AFSR : 0
Mode : Thread
Priv : Privileged
Stack: PSP

-- MbedOS Fault Handler --



++ MbedOS Error Info ++
Error Status: 0x80FF013D Code: 317 Module: 255
Error Message: Fault exception
Location: 0xA3009150
Error Value: 0x10005E30
Current Thread: main Id: 0x10004248 Entry: 0x22C75 StackSize: 0x1000 StackMem: 0x10005E90 SP: 0x10006E48 
For more info, visit: https://mbed.com/s/error?error=0x80FF013D&tgt=SFE_ARTEMIS_ATP
-- MbedOS Error Info --

Trace of I2C Output

First I2C transmission definitely worked:

Possible solution

This code would stop the use-after-free. Maybe more is needed.

 void arduino::MbedI2C::end() { 
 	if (master != NULL) { 
 		delete master; 
 		master = NULL;
 	} 
 #ifdef DEVICE_I2CSLAVE 
 	if (slave != NULL) { 
 		delete slave; 
 		slave = NULL; 
 	} 
 #endif 
 } 

[Wenn0101]

Great find! This wont just affect Wire, I know SPI has similar logic, and I will have to comb thru the codebase for other similar mistakes.

The suggested fix is a great start that fixes the use-after-free, but leads to another runtime error. As pointed out in #439 mbed::I2C does not have a destructor that calls the de-init for wire, none of the mbed objects do. So when the new wire is created, the HAL complains that the module has already been inited (and not deinited) and fails.

I am tempted to just add destructors for all the mbed classes that Artemis uses in the Sparkfun branch of mbed, but I fear that this will be to far a departure from the main branch to ever update again later. I may end up putting in a solution as @paulvha has suggested for the OpenLog in sparkfun/OpenLog_Artemis#117. This "breaks" some of the encapsulation of the mbed core, but isn't nearly as large of a change.

I will think on this a bit, but I may still try to sneak a fix in to the upcoming v2.2.1.

[nigelb]

Hi @Wenn0101 thanks for looking at this. If you created the destructors could you push them upstream to mbed? It might just be me, but it seems odd that they don't have them.

Also while we are on the topic of hardware de-initialization neither UART::~UART, nor UART::end properly de-initialize the UART hardware. #412, and maybe #411, seem to be in this wheelhouse as well.

[Wenn0101]

Looking at the Mbed issue, they seem to be reluctant to bring in these changes. Because the feature has been missing for so long, many of the targets never implemented the de-init functions for the various drivers. So if they added it in, it would lead to a situation where some of the target support it and other do not. It could be worth worth a try to see if they would though.

I think that this will require more thought then a last minute change, so I am not going to try and include this in v2.2.1.

[nigelb]

Hi @Wenn0101, I think I have come up with a shim that allows us to have our cake and eat it too.
With this you won't have to push code to mbed or maintain a forked version of their code.

Firstly make the changes I suggested above:

 void arduino::MbedI2C::end() { 
 	if (master != NULL) { 
 		delete master; 
 		master = NULL;
 	} 
 #ifdef DEVICE_I2CSLAVE 
 	if (slave != NULL) { 
 		delete slave; 
 		slave = NULL; 
 	} 
 #endif 
 } 

At this point, if we compile and run my example from above we get this:

Wire Use-After-Free
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop


++ MbedOS Error Info ++
Error Status: 0x80FF0144 Code: 324 Module: 255
Error Message: Assertion failed: AM_HAL_STATUS_SUCCESS == am_hal_iom_initialize(obj->iom.inst, &obj->iom.handle)
Location: 0x21C9D
File: mbed-os/targets/TARGET_Ambiq_Micro/TARGET_Apollo3/device/iom_api.c+33
Error Value: 0x0
Current Thread: main Id: 0x10004248 Entry: 0x22C85 StackSize: 0x1000 StackMem: 0x10005E90 SP: 0x10006DE4 
For more info, visit: https://mbed.com/s/error?error=0x80FF0144&tgt=SFE_ARTEMIS_ATP
-- MbedOS Error Info --

However if we create a new class derived from mbed::I2C by changing:

Arduino_Apollo3/libraries/Wire/src/Wire.h

Lines 20 to 22 in 4e24a02

	namespace arduino {
	class MbedI2C : public HardwareI2C

to:

namespace arduino {

class myI2C: public mbed::I2C
{
    public:
    using mbed::I2C::I2C;
    virtual ~myI2C(){
        i2c_free(&_i2c);
    };
};

class MbedI2C : public HardwareI2C

then replace this:

Arduino_Apollo3/libraries/Wire/src/Wire.h

Line 55 in 4e24a02

mbed::I2C* master;

with:

    myI2C*      master;

and this line:

Arduino_Apollo3/libraries/Wire/src/Wire.cpp

Line 16 in 4e24a02

master = new mbed::I2C((PinName)_sda, (PinName)_scl);

with this:

	master = new myI2C((PinName)_sda, (PinName)_scl);

Now if we compile and run this we get the following output:

Wire Use-After-Free
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
.
.
.

The myI2C class probably needs a more appropriate name, and I am not 100% certain that the hardware fully de-initialized, however this does demonstrate that we can get hold of the i2c_t object so we can call i2c_free with it.