2

I have a very peculiar situation here ... I have inherited some old C++ code (pre-C++11) which is long, complex and half of it is written with C and the other half with C-with-classes mentality (ie: classes with more data members, not too many class methods, direct manipulation of data member... definitely this project needs some refactoring, that's what I'm working on right now). But the code exposes some issues I find puzzling.

For the beginning let's take the following very simple (error free) situation:

#include <iostream>

static int c = 0;

struct Bar
{
    Bar() : base_id(++c) { std::cout << "Bar "<< base_id << std::endl;}
    int base_id;
};

struct Foo : public Bar
{
  Foo() : x(c) { std::cout << "Foo "<< x << std::endl;}
  int x;
};

int main()
{
  Bar* b = new Foo[200];
  Foo *p;

  for(p = (Foo*)b; p - (Foo*)b < 200; p ++ )
  {
      std::cout << p->base_id << " " << ((Foo*)p)->x 
         << " p-b=" << (unsigned)(p-(Foo*)b) << std::endl;
  }


  delete[] b;
}

or text version: we create an array of base class objects via a new Derived. So far so good, this is what the large complex application does (there the classes are more hierarchical and the constructors does more), but the global static variable (c) is present also in the large complex application, it uses it as unique object identifiers. Then large complex application begins to work, etc....

Then at some point in time we iterate through the array of objects, doing some work. The iteration looks exactly the same as the one I wrote here, found in a for loop, with exhaustive pointer arithmetic. My example here just prints the object id (m) in the large complex application more stuff is done.

But somewhere in the large complex application some magic happens ... At some time after the half of the list the objects (obtained via the p pointer) are not valid anymore, their base_ids are showing data which to me pretty much looks like pointers and other data members's values. However if I check the array members at the specific index, the value there is valid (ie: correctly increasing object IDs).

I also checked the followings:

  • There seems to be no memory corruption issue. Valgrind and Clang's memory sanitizer show no obious things.
  • All the objects are properly created and destroyed, no memory leaks.
  • The only place where this craziness happens is in this loop above

So ... I came to one conclusion:

  • someone somewhere modifies some array values to actually point to different type of object (all of them are derived from Bar (e... some differently named base class) so possibly I'll need to dig more in the code to find this. But this is a huge codebase, there are literally thousands of references for that array and before that I'd like to ask:

(And here comes the question:)

I am not aware that a C++ object can change its size during runtime (if it is possible, please share it with us), so beside of the possible issue I have identified might anyone in the community have any ideas why that pointer arithmetics behaves so strangely?

(yes, the project will be converted to proper c++11 with standard containers, etc ... but for now I am just interested in this specific issue)

Ferenc Deak
  • 34,348
  • 17
  • 99
  • 167
  • 4
    Size of the object is calculated at compile time. It can not be changed in runtime. – Teivaz Apr 22 '16 at 11:23
  • 2
    "So far so good" - eh no, you're pretty screwed already there. See for ex here: http://stackoverflow.com/questions/6171814/why-is-it-undefined-behavior-to-delete-an-array-of-derived-objects-via-a-base – Mat Apr 22 '16 at 11:26
  • unless the classes are strictly standard layout, (Foo*)b will not necessarily yield the same value (address) as b. Could that be relevant? – Richard Hodges Apr 22 '16 at 11:27
  • @Mat yes, yes, this code was written ... not exactly with avoiding undefined behaviour in mind :) – Ferenc Deak Apr 22 '16 at 11:31
  • "we create an array of base class pointers via a `new Derived`" No, you don't. An array of base class pointers would be created via `new Base*[200]`. – MSalters Apr 22 '16 at 12:15
  • 1
    The sizes of objects do not change - they are fixed at compile time. Odds are, some code - somewhere - is doing something directly with `b[i]` without "correctly" typecasting to a `Foo *`. `b[i]` will not be equivalent to `p[i]` (e.g. `(void *)(&b[i]) != (void *)(&p[i])`) unless `Foo` and `Bar` have exactly the same size (which is not a fair expectation, since `Bar` inherits from `Foo` and has an additional data member). All it takes is for one function, somewhere, to not do the required typecasting before accessing array elements, and your program has undefined behaviour. – Peter Apr 22 '16 at 12:20
  • This is at the end, but ``delete[] b;`` is undefined behaviour. – Emerald Weapon Apr 22 '16 at 12:38
  • @MSalters removed the typo – Ferenc Deak Apr 22 '16 at 12:45

1 Answers1

2

Your problem is here:

Bar* b = new Foo[200];

b points to a Bar subobject in the first element of the Foo array, but is not usable as a way to access the array; the size is wrong. Constantly casting it back to a Foo pointer seems to work, but is prone to failure. (It gets even worse in the face of multiple inheritance, when the Bar subobject isn't even at the same address as the Foo object it's part of.)

The code you posted is careful to always cast b to a Foo* before doing pointer arithmetic. (Even too careful: ((Foo*)p)->x could be just p->x.) But if at any point, anywhere, someone forgets to do this (e.g. tries b[i], or b+i, or (Foo*)(b+i)…), it will lead to exactly the behavior you describe. Or perhaps, with all this downcasting going on, someone downcasts the Bar* to a Baz*, where Baz also inherits from Bar, but has a different size from Foo. That would also overwrite fields in wonky ways.

Also, delete[] b is undefined behavior. So it's not outside the realm of possibility for the compiler to conclude that b really points to Bar instances, and elide or screw up casts to Foo*—compilers do that sort of thing these days.

All in all, Bar* b = new Foo[200]; is unworkable. Use

Foo* fp = new Foo[200];

instead. If for some reason, you need a Bar*, you can follow it with

Bar* b = fp;

But it's not clear why you need this; you can use fp whenever a Bar* is required.

Community
  • 1
  • 1
Nick Matteo
  • 4,453
  • 1
  • 24
  • 35
  • Note however that `Bar* b = fp;` will tempt people to do things like `b[i]` and you're back where you started. Better would be `Bar& b = fp[0];`. – Raymond Chen Jun 02 '16 at 14:42