Bug in BusOut class, possibly affecting others

24 May 2013

I was recently working on a project and ran into a problem that seemed very odd. I was using the BusOut class as an argument in my class constructor, which then used the default copy constructor to initialize another BusOut object within the class. I found that using this method produced very odd, seemingly unpredictable errors when using the heap elsewhere in my code.

After much debugging, I isolated this problem to the BusOut class. I then looked in the source code for the BusOut class, and found this:

BusOut::BusOut(PinName p0, PinName p1, PinName p2, PinName p3, PinName p4, PinName p5, PinName p6, PinName p7, PinName p8, PinName p9, PinName p10, PinName p11, PinName p12, PinName p13, PinName p14, PinName p15) {
    PinName pins[16] = {p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, p10, p11, p12, p13, p14, p15};
 
    for (int i=0; i<16; i++) {
        _pin[i] = (pins[i] != NC) ? new DigitalOut(pins[i]) : 0;
    }
}

and:

BusOut::~BusOut() {
    for (int i=0; i<16; i++) {
        if (_pin[i] != 0) {
            delete _pin[i];
        }
    }
}

So, the BusOut class uses dynamic memory allocation to create DigitalOut objects. That doesn't seem like an issue until you look at the class declaration:

class BusOut {
 
public:
 
    /** Create an BusOut, connected to the specified pins
     *
     *  @param p<n> DigitalOut pin to connect to bus bit <n> (p5-p30, NC)
     *
     *  @note
     *  It is only required to specify as many pin variables as is required
     *  for the bus; the rest will default to NC (not connected)
     */
    BusOut(PinName p0, PinName p1 = NC, PinName p2 = NC, PinName p3 = NC,
           PinName p4 = NC, PinName p5 = NC, PinName p6 = NC, PinName p7 = NC,
           PinName p8 = NC, PinName p9 = NC, PinName p10 = NC, PinName p11 = NC,
           PinName p12 = NC, PinName p13 = NC, PinName p14 = NC, PinName p15 = NC);
 
    BusOut(PinName pins[16]);
 
    virtual ~BusOut();
 
    /** Write the value to the output bus
     *
     *  @param value An integer specifying a bit to write for every corresponding DigitalOut pin
     */
    void write(int value);
 
    /** Read the value currently output on the bus
     *
     *  @returns
     *    An integer with each bit corresponding to associated DigitalOut pin setting
     */
    int read();
 
#ifdef MBED_OPERATORS
    /** A shorthand for write()
     */
    BusOut& operator= (int v);
    BusOut& operator= (BusOut& rhs);
 
    /** A shorthand for read()
     */
    operator int();
#endif
 
protected:
    DigitalOut* _pin[16]; // HERE
};

The DigitalOut objects are stored as pointers! So, when my class used the BusOut class as an argument(passed by copy), it allocated memory for the DigitalOut pins. These were stored as pointers by this member variable. When I called the default copy constructor, these pointers were directly copied into my member BusOut object. Once the constructor exited, the destructor of local copy (the function argument) of BusOut was called, deleting the DigitalOut objects also pointed to by the valid instance of BusOut within my class. This means that as soon as I allocated anything else on the heap the BusOut instance in my class became invalid.

I see two possible solutions to this problem.

  • Stop using dynamic memory allocation. This means storing 16 DigitalOut objects themselves instead of pointers to them.
  • Create a custom copy constructor. This constructor would have to make sure that the DigitalOut pointers are properly allocated instead of copied directly.

Also, I would guess that this same technique is used in other classes within the mbed library. This issue probably affects them as well.

16 Jun 2013

Has this issue been looked into?

As I mentioned in my first post, this should be an easy problem to solve. A simple copy constructor would prevent this problem.

I think a problem like this would be much more daunting to someone who is new to C/C++. I am a fairly experienced programmer and because of the extremely odd results this error produced, It took me much longer than it should have to track down the problem.