Bad memory leak in al_get_pixel on Linux?

no-reply@allegro.cc (Chris Katko) — Mon, 27 Apr 2015 07:46:33 +0000

This function is throwing insane amounts of memory leaks in Valgrind and seems to be corrupting my game.

#SelectExpand
  1ALLEGRO_COLOR al_get_pixel(ALLEGRO_BITMAP *bitmap, int x, int y)
  2{
 ALLEGRO_LOCKED_REGION *lr;
 char *data;
 ALLEGRO_COLOR color;
  6
 if (bitmap->parent) {
    x += bitmap->xofs;
    y += bitmap->yofs;
    bitmap = bitmap->parent;
 }
 12
 if (bitmap->locked) {
    x -= bitmap->lock_x;
    y -= bitmap->lock_y;
    if (x < 0 || y < 0 || x >= bitmap->lock_w || y >= bitmap->lock_h) {
       ALLEGRO_ERROR("Out of bounds.");
       memset(&color, 0, sizeof(ALLEGRO_COLOR));
       return color;
    }
 21
    data = bitmap->locked_region.data;
    data += y * bitmap->locked_region.pitch;
    data += x * al_get_pixel_size(bitmap->locked_region.format);
 25
    _AL_INLINE_GET_PIXEL(bitmap->locked_region.format, data, color, false);
 }
 else {
    /* FIXME: must use clip not full bitmap */
    if (x < 0 || y < 0 || x >= bitmap->w || y >= bitmap->h) {
       memset(&color, 0, sizeof(ALLEGRO_COLOR));
       return color;
    }
 34
    if (!(lr = al_lock_bitmap_region(bitmap, x, y, 1, 1, bitmap->format,
          ALLEGRO_LOCK_READONLY)))
    {
       memset(&color, 0, sizeof(ALLEGRO_COLOR));
       return color;
    }
 41
    /* FIXME: check for valid pixel format */
 43
    data = lr->data;
    _AL_INLINE_GET_PIXEL(bitmap->format, data, color, false);
 46
    al_unlock_bitmap(bitmap);
 }
 49
 return color;
 51}

I kept noticing that randomly, one of the two players in my game would not be drawn. Then I found their x/y coordinates were corrupted.

This simple function calls it, and uses it for per-pixel collision detection:

#SelectExpand
  1bool check_map_blocked(float x, float y)
{
if(x < 0)return false;
if(y < 0)return false;
if(x > al_get_bitmap_width(map_bmp)-1)return false;
if(y > al_get_bitmap_height(map_bmp)-1)return false;
if(map_bmp == NULL)throw "shit storm";
ALLEGRO_COLOR temp = al_get_pixel(map_bmp, x, y);
  
if(temp.r == 0 && temp.g == 0 && temp.b == 0)
  {
  return false;
  }else{
  return true;
  }

}

The error goes over and over:

#SelectExpand
  1==37649==    at 0x4E7539A: al_get_pixel (bitmap_pixel.c:56)
  2==37649==    by 0x402F63: object_type::check_map_blocked(float, float) (joust_a5.cpp:244)
  3==37649==    by 0x403256: object_type::logic() (joust_a5.cpp:291)
  4==37649==    by 0x4036B9: man_type::logic() (joust_a5.cpp:388)
  5==37649==    by 0x40201A: logic(map_type*, std::vector<man_type*, std::allocator<man_type*> >, std::vector<bird_type*, std::allocator<bird_type*> >) (joust_a5.cpp:486)
  6==37649==    by 0x4026CF: main (joust_a5.cpp:623)
  7
  8
  9
 10==37649== Use of uninitialised value of size 8
 11==37649==    at 0x4E75DE1: al_get_pixel (bitmap_pixel.c:71)
 12==37649==    by 0x402F63: object_type::check_map_blocked(float, float) (joust_a5.cpp:244)
 13==37649==    by 0x403256: object_type::logic() (joust_a5.cpp:291)
 14==37649==    by 0x4036B9: man_type::logic() (joust_a5.cpp:388)
 15==37649==    by 0x40201A: logic(map_type*, std::vector<man_type*, std::allocator<man_type*> >, std::vector<bird_type*, std::allocator<bird_type*> >) (joust_a5.cpp:486)
 16==37649==    by 0x4026CF: main (joust_a5.cpp:623)

no-reply@allegro.cc (SiegeLord) — Mon, 27 Apr 2015 07:56:51 +0000

Those look like uninitialized memory reads, not leaks. What version of Allegro is this?

no-reply@allegro.cc (Chris Katko) — Wed, 29 Apr 2015 06:13:05 +0000

Sorry, that's what I meant to say.

Allegro 5.1.7.

Linux, OpenGL.

Though, I'm having trouble replicating it in a small example app, so maybe I'm being a moron somewhere else. I'm still checking.

[edit] I'm going to bed for the night.

[edit 2]

Okay, it doesn't actually look like it's Allegro fault. (Yippie!)

However, check this code out:

#SelectExpand
  1class object_type
{
//...
void virtual set_owner(object_type *obj)
  {
  assert(owner != NULL);
  owner = obj; 
  is_connected_to_owner = true; 
  }
};
 11
 12class man_type : public object_type
{
//...
};
 16
 17class bird_type : public object_type
{
//...
};
 21
 22int main()
{
//...
map_type map("data/map.png");
 26
man_type *temp_man   = new man_type(200, 200, &map);
bird_type *temp_bird = new bird_type(200, 200, &map);

  assert(temp_man  != NULL);
  assert(temp_bird != NULL);

temp_man ->set_owner(temp_bird); //FAILS on assert inside set_owner
temp_bird->set_owner(temp_man);

man_type *temp_man2   = new man_type(300, 300, &map);
bird_type *temp_bird2 = new bird_type(300, 300, &map);

  assert(temp_man2  != NULL);
  assert(temp_bird2 != NULL);  

temp_man2 ->set_owner(temp_bird2);
temp_bird2->set_owner(temp_man2);  
}

Can I not pass a child class with a bass class pointer? Shouldn't RTTI allow this?

no-reply@allegro.cc (Edgar Reynaldo) — Wed, 29 Apr 2015 06:40:05 +0000

Are you initializing owner anywhere?

Those assert(temp_man) statements you have aren't doing anything, because by default new throws std::bad_alloc upon failure. You have to use no throw new, or catch the exception.

no-reply@allegro.cc (Chris Katko) — Wed, 29 Apr 2015 07:05:06 +0000

Edgar Reynaldo said:

Are you initializing owner anywhere?

Those assert(temp_man) statements you have aren't doing anything, because by default new throws std::bad_alloc upon failure. You have to use no throw new, or catch the exception.

I just tossed those asserts in there to be absolutely sure. But thanks, I don't think I remembered that bad_alloc gets thrown.

Owner is just a member of object_type. set_owner() is a required function, it's not apart of the constructor to allow for circular linking (bird <-> man). So owner gets set by set_owner() and nowhere else.

The strange thing is that sometimes... it doesn't fail. Re-running it only sometimes crashes it. But I'm not really doing much else in my code that involves pointers...

no-reply@allegro.cc (Edgar Reynaldo) — Wed, 29 Apr 2015 07:30:31 +0000

Chris Katko said:

If you don't initialize owner it could be any value. Which means this code could evaluate to true or false. assert(owner != NULL);. If you want it to only be set once upon construction, initialize it to 0 and then assert it is NULL before assigning it.

no-reply@allegro.cc (Chris Katko) — Wed, 29 Apr 2015 07:37:24 +0000

Durr, that's a good point.

Secondly, oh my god, I can't believe I missed a typo... and apparently failed to copy it properly when I posted... or maybe I changed it, I don't know but this is definitely wrong:

void virtual set_owner(object_type *obj, std::string helper)
    {
    printf("%s\n", helper.c_str());
    assert(owner != NULL); //<--- should be obj != NULL
    owner = obj; 
    is_connected_to_owner = true; 
    }

The game is running now... though, I can't think of what I've done that fixed it except fixing the improper debugging code which was added after things started corrupting... I'll do more testing.

no-reply@allegro.cc (Edgar Reynaldo) — Wed, 29 Apr 2015 07:38:41 +0000

That does change things a bit.

no-reply@allegro.cc (Chris Katko) — Thu, 30 Apr 2015 04:49:42 +0000

It's still corrupted somewhere. Any ideas for tracking down a lone pointer problem? The whole program can't be more than 800 lines.

no-reply@allegro.cc (bamccaig) — Thu, 30 Apr 2015 05:28:48 +0000

Protip: Learn to use Git and use it. If things are broken commit. If things start working commit. You can easily compare. The history doesn't have to be permanent. If it's stupid and you don't think it adds value you can rewrite the history to remove it after. It helps you to make sense of things like this. And so much more.

Append:

You can try valgrind in Linux. I'm not sure if it will catch this particular problem, but I suspect that it might...

no-reply@allegro.cc (Thomas Fjellstrom) — Thu, 30 Apr 2015 07:12:45 +0000

valgrind and similar tools are your friend. a good debugger helps too.

no-reply@allegro.cc (raynebc) — Thu, 30 Apr 2015 09:49:54 +0000

Memwatch works pretty well and isn't *nix specific so you could probably use it in any platform.

no-reply@allegro.cc (GullRaDriel) — Tue, 05 May 2015 15:25:09 +0000

On SOLARIS there is also dbx debugger with the check -all or check -memuse.

no-reply@allegro.cc (Chris Katko) — Tue, 05 May 2015 17:59:47 +0000

I think I might have done something even sillier. I had velocity code in an object that when I moved some code around, no longer initialized velocities to zero at start. Ever since I fixed that, I haven't noticed the problem anymore--though, it could still be there.