Bugs of the day, Monday 2013-03-25

Initialized, but not in the right order

One runtime bug today, being fallout from an attempted fix for Friday’s Unexpected use of class bug.

To recap/elaborate: I changed something like this

struct Foo : public Baz {
  Bar* m_Bar;
  virtual void Destroy() override { }
};

to be something more like this

struct Foo : public Baz {
  Bar* m_Bar;
  virtual void Destroy() override { m_Bar->Destroy(); }
};

and I had assumed this would be OK because I [thought that I] knew the only place that Foos would be created, which would always safely initialize m_Bar.

Unfortunately, there is another way to construct these objects with some unexpected (bad) input data that would not initialize m_Bar. In such a case, Foo::Destroy() would crash because m_Bar pointed at who-knows-what.

So I implemented something like

struct Foo : public Baz {
  Bar* m_Bar;
  virtual void Init() override { m_Bar = NULL; }
  virtual void Destroy() override {
    if( m_Bar != NULL)
      m_Bar->Destroy();
    else
      HARD_TO_MISS_NOTIFICATION( USEFUL_WARNING_37 );
  }
};

“Hooray,” I thought. “That will catch those pesky bad datas.”

In practice what I got was a crash from the first thing that tried to use m_Bar.

It turns out that the construction path for Foos sets up m_Bar before calling Foo::Init(), so now our correctly constructed Foo object has it’s valid data overwritten by an overzealous initialization function.

Foo::Init() is called from both the preferred and not-preferred construction paths, so my hope had been to fix it there and catch use of the not-preferred path, but this didn’t work. My next thought was to reorder the setup of m_Bar and the call to Init(), but that seemed far too risky — there are a lot of subclasses of Foo, and many of them have non-trivial Init() overrides of their own, so there is a high probability that something else will break if m_Bar is not set up before the call to Init().

The winner this time seems to be to initialize the member to zero using: a constructor. Yes, it’s probably the obvious option, but for much of this codebase explicit Init() functions are used instead of constructors. Fortunately, for Foo and its friends (sub classes, parent class) we call the constructor (via placement new) to set up the vtable for us, so we can use it to also initialize the m_Bar pointer and consequently help catch occurrences of bad data in a less fatal fashion.

 

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>