Potential Design Pattern bug in Chibi Abstract peripheral interfaces Topic is solved

[yusuf.husainy]

Hi All,

We are working on writing external device drivers for certain Accelerometers/Gyros and Magnetometers using the abstract peripheral interface. While going through the design pattern(object oriented C having virtual function implementations), I found a potential flaw in the design pattern:

- ChibiOS version or subversion revision number : 17.6.2
- Compiler : gcc
- Platform and board : STM32, STM32L476RG-Nucleo
- Nature of the problem : Potential pointer dereference in polymorphic type inheritance models written in C for abstract peripheral devices
- Failure mode: Accessing polymorphic base class member data variables from a derived class object, using a base class pointer.

Eg: in the driver lis302dl.h, driver for accelerometer, the derived class of LIS302DLDriver from BaseAccelerometer, which in turn is derived from BaseSensor, has 3 virtual tables and 3 virtual pointers for each of them. So, if user creates an instance of LIS302DLDriver, typecasts it's address to a pointer of type BaseAccelerometer, then tries to access data members of BaseAccelerometer using that pointer, it would get garbage data since, the offset of the _base_accelerometer_data is actually the second virtual table of LIS302DLDriver declaration. (Luckily, for accelerometer abstract peripheral interface, there is no data for the base class, so this bug is missed.)

• The following example code snippet wouldn't work(assume BaseSensor has a data variable called 'msg_t stateSensor') (just for illustration):
  In hal_sensors.h:
  

  Code: Select all

  /**
   * @brief   @p BaseSensor specific data.
   * @note    It is empty because @p BaseSensor is only an interface
   *          without implementation.
   */
  #define _base_sensor_data \
    msg_t stateSensor;
  

  And in main.c:
  

  Code: Select all

    
    LIS302DLDriver derivedAccelerometerObject = {0};
    LIS302DLDriver * derivedAccelerometerPointer = &derivedAccelerometerObject;
    BaseSensor * baseAccelerometerPointer = (BaseSensor *)&derivedAccelerometerObject;
    derivedAccelerometerPointer->stateSensor = 1;
    msg_t stateViewedFromBaseAccelerometerPointer = baseAccelerometerPointer->stateSensor;
  

  In above snippet, stateViewedFromBaseAccelerometerPointer doesn't get updated with value of '1'.
  
  
• A potential solution would be is to define class heirarchy of hal_sensors and it's derived forms as:
  

  Code: Select all

  /**
   * @brief   Base sensor class.
   * @details This class represents a generic blocking unbuffered sequential
   *          data stream.
   */
  typedef struct {
    /** @brief Virtual Methods Table.*/
    const struct BaseSensorVMT *vptr;
    _base_sensor_data
  } BaseSensor;
  
  /**
   * @brief   Base accelerometer class.
   * @details This class represents a generic a generic accelerometer.
   */
  typedef struct {
    /** @brief Virtual Methods Table.*/
    const struct BaseAccelerometerVMT *vptr;
    _base_sensor_data
  } BaseAccelerometer;
  
  
  /**
   * @extends BaseAccelerometer
   *
   * @brief   LIS302DL 3-axis accelerometer class.
   * @details This class extends @p BaseAccelerometer by adding physical
   *          driver implementation.
   */
  struct LIS302DLDriver {
    /** @brief LIS302DL Virtual Methods Table. */
    const struct LIS302DLVMT *vptr;
    _lis302dl_data
  };
  

  And then the macro definitions for function call helpers as:
  

  Code: Select all

  /**
   * @name    Macro Functions (BaseSensor)
   * @{
   */
  /**
   * @brief   Sensors get channels number.
   *
   * @param[in] ip        pointer to a @p BaseSensor or derived class.
   * @return              The number of channels of the BaseSensor
   *
   * @api
   */
  #define sensorGetChannelNumber(ip) (ip)->vptr->get_channels_number(ip)
  
  /**
   * @brief   Sensors read raw data.
   *
   * @param[in] ip        pointer to a @p BaseSensor or derived class.
   * @param[in] dp        pointer to a data array.
   * 
   * @return              The operation status.
   * @retval MSG_OK       if the function succeeded.
   * @retval MSG_RESET    if one or more errors occurred.
   *
   * @api
   */
  #define sensorReadRaw(ip, dp) (ip)->vptr->read_raw(ip, dp)
  

  
  ...similarly for all derived classes of BaseSensor

Regards,
Yusuf Husainy

[faisal]

Another example of this design bug is if you use a JESD216 flash driver with MFS.

A BaseFlash instance looks like this:

Code: Select all

typedef struct {
  _base_flash_methods_alone
  _base_flash_data
} BaseFlash;

A JESD216 instance looks like this:

Code: Select all

typedef struct {
  _base_flash_methods_alone
  _jesd216_flash_methods_alone 
  _base_flash_data
} JESD215Flash;

As an example, when the MFS driver uses a m25q driver (which extends the JESD216 class), which is casted as a BaseFlash instance, the _base_flash_data (which constitutes a flash_state_t) will not be in the right place - rather _jesd216_flash_methods_alone is there! Is it a fluke that the demo is working, or do I have a fundamental misunderstanding?

Shouldn't the layering be something like this (incrementally adding to the structure, preserving the parent classes):

Code: Select all

typedef struct {
  _animal_methods_alone
  _animal_data
} AnimalClass;

typedef struct {
  _animal_methods_alone
  _animal_data
  _mammal_methods_alone
  _mammal_data
} MammalClass;

typedef struct {
  _animal_methods_alone
  _animal_data
  _mammal_methods_alone
  _mammal_data
  _human_methods_alone
  _human_data
} HumanClass;

[Giovanni]

Hi,

About flash, thet code I am looking at is different from what you posted.

Code: Select all

#define _jesd216_flash_methods                                              \
  _base_flash_methods                                                       \
  _jesd216_flash_methods_alone

struct JESD215FlashVMT {
  _jesd216_flash_methods
};

#define _jesd216_flash_data                                                 \
  _base_flash_data

typedef struct {
  /** @brief Virtual Methods Table.*/
  const struct JESD215FlashVMT *vmt;
  _jesd216_flash_data
} JESD215Flash;

The offset of _jesd216_flash_data does not change, there is always just one pointer before it.

RoccoMarco is looking into the other problem.

Giovanni

[Giovanni]

Hi,

We identified various problems in EX related to multiple inheritance, we are working on it, thanks for finding.

Giovanni

[faisal]

Hi,

About flash, thet code I am looking at is different from what you posted.

Code: Select all

#define _jesd216_flash_methods                                              \
  _base_flash_methods                                                       \
  _jesd216_flash_methods_alone

struct JESD215FlashVMT {
  _jesd216_flash_methods
};

#define _jesd216_flash_data                                                 \
  _base_flash_data

typedef struct {
  /** @brief Virtual Methods Table.*/
  const struct JESD215FlashVMT *vmt;
  _jesd216_flash_data
} JESD215Flash;

The offset of _jesd216_flash_data does not change, there is always just one pointer before it.

RoccoMarco is looking into the other problem.

Giovanni

Yes, the text of the code is different. I posted the effective code after manually expanding the macros to make the bug I've identified clearer.

The issue is with where the base flash data is located in this particular example. In general, the way multiple inheritance is implemented is flawed, and I showed an example of how it could be done correctly using the Animal, Mammal, Human example.

[faisal]

Hi,

We identified various problems in EX related to multiple inheritance, we are working on it, thanks for finding.

Giovanni

Cool, thanks for looking into it. Was worried if you would be able to address it before 18.x release.

[RoccoMarco]

Fixed on trunk and backported to 17.6.4 as Bug #915.

[faisal]

Fixed on trunk and backported to 17.6.4 as Bug #915.

I believe there are still issues. Take the lis302dl driver as an example, which has been recently 'fixed'. A simple test you can do to see if the polymorphism will work, is to take the driver structure and cast it with the various base classes.

If you cast the LIS302DLDriver struct as a BaseAccelerometer, it will fail. Following the pattern of hal_sensors, and hal_accelerometer, the driver structure should look like this (remove the BaseSensorVMT and _base_sensor_data).

Code: Select all

struct LIS302DLDriver {
  const struct LIS302DLAccelerometerVMT *vmt_accelerometer;
  _base_accelerometer_data
  _lis302dl_data
};

I checked most of the ST devices, and this bug is in all of them.

Aside from the above bug fix, an improvement would be to just call the virtual method table 'vmt', instead of vmt_sensor/accelerometer, etc ... In fact, this is already the case in the flash peripheral classes.

[RoccoMarco]

Hello there,
thanks for reporting.

Discussing with Giovanni we noticed that there is a huge difference between flash and mems: here we have both polymorphism and multi-interface. Each MEMS indeed can suit more than an interface.

To address this difference we have added an additional API which give you the sub-interface pointer: a simple cast is not enough. I will provide some articles and example on play embedded soon.

[Giovanni]

Hi,

In the new scheme you cannot cast an object pointer to any of the ancestors, you need to cast the pointer to the VMT field and use that as reference. Each class now exports a "get" macro that does that.

Code: Select all

#define getBaseAccelerometer(ip) ((BaseAccelerometer *)&(ip)->vmt_accelerometer)

I think there is still a problem too, taking the pointer to VMTs makes the data in the class inaccessible because the offset. also, the macro is not used by the invoker macros.

Code: Select all

#define accelerometerGetAxesNumber(ip)                                      \
        (ip)->vmt_accelerometer->get_channels_number(ip)

It should be something like:

Code: Select all

#define accelerometerGetAxesNumber(ip)                                      \
        getBaseAccelerometer(ip)->vmt_accelerometer->get_channels_number(ip)

This would mean that the accelerometer methods would only see the accelerometer data but not the lis302dl-specific data because the offset is shifted...

Code: Select all

struct LIS302DLDriver {
  /** @brief BaseSensor Virtual Methods Table. */
  const struct BaseSensorVMT *vmt_sensor;
  _base_sensor_data
  /** @brief BaseAccelerometer Virtual Methods Table. */
  const struct LIS302DLAccelerometerVMT *vmt_accelerometer;
  _base_accelerometer_data
  _lis302dl_data
};

I don't see a solution right now... The requirement is that a device implementing multiple sensors must be accessible as instances of separate sensors.

Giovanni