Allegro.cc - Online Community

Allegro.cc Forums » Allegro Development » Bad memory leak in al_get_pixel on Linux?

This thread is locked; no one can reply to it. rss feed Print
Bad memory leak in al_get_pixel on Linux?
Chris Katko
Member #1,881
January 2002
avatar

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{ 3 ALLEGRO_LOCKED_REGION *lr; 4 char *data; 5 ALLEGRO_COLOR color; 6 7 if (bitmap->parent) { 8 x += bitmap->xofs; 9 y += bitmap->yofs; 10 bitmap = bitmap->parent; 11 } 12 13 if (bitmap->locked) { 14 x -= bitmap->lock_x; 15 y -= bitmap->lock_y; 16 if (x < 0 || y < 0 || x >= bitmap->lock_w || y >= bitmap->lock_h) { 17 ALLEGRO_ERROR("Out of bounds."); 18 memset(&color, 0, sizeof(ALLEGRO_COLOR)); 19 return color; 20 } 21 22 data = bitmap->locked_region.data; 23 data += y * bitmap->locked_region.pitch; 24 data += x * al_get_pixel_size(bitmap->locked_region.format); 25 26 _AL_INLINE_GET_PIXEL(bitmap->locked_region.format, data, color, false); 27 } 28 else { 29 /* FIXME: must use clip not full bitmap */ 30 if (x < 0 || y < 0 || x >= bitmap->w || y >= bitmap->h) { 31 memset(&color, 0, sizeof(ALLEGRO_COLOR)); 32 return color; 33 } 34 35 if (!(lr = al_lock_bitmap_region(bitmap, x, y, 1, 1, bitmap->format, 36 ALLEGRO_LOCK_READONLY))) 37 { 38 memset(&color, 0, sizeof(ALLEGRO_COLOR)); 39 return color; 40 } 41 42 /* FIXME: check for valid pixel format */ 43 44 data = lr->data; 45 _AL_INLINE_GET_PIXEL(bitmap->format, data, color, false); 46 47 al_unlock_bitmap(bitmap); 48 } 49 50 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) 2 { 3 if(x < 0)return false; 4 if(y < 0)return false; 5 if(x > al_get_bitmap_width(map_bmp)-1)return false; 6 if(y > al_get_bitmap_height(map_bmp)-1)return false; 7 if(map_bmp == NULL)throw "shit storm"; 8 ALLEGRO_COLOR temp = al_get_pixel(map_bmp, x, y); 9 10 if(temp.r == 0 && temp.g == 0 && temp.b == 0) 11 { 12 return false; 13 }else{ 14 return true; 15 } 16 17 }

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)

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

SiegeLord
Member #7,827
October 2006
avatar

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

"For in much wisdom is much grief: and he that increases knowledge increases sorrow."-Ecclesiastes 1:18
[SiegeLord's Abode][Codes]:[DAllegro5]:[RustAllegro]

Chris Katko
Member #1,881
January 2002
avatar

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 2 { 3 //... 4 void virtual set_owner(object_type *obj) 5 { 6 assert(owner != NULL); 7 owner = obj; 8 is_connected_to_owner = true; 9 } 10 }; 11 12class man_type : public object_type 13 { 14 //... 15 }; 16 17class bird_type : public object_type 18 { 19 //... 20 }; 21 22int main() 23 { 24 //... 25 map_type map("data/map.png"); 26 27 man_type *temp_man = new man_type(200, 200, &map); 28 bird_type *temp_bird = new bird_type(200, 200, &map); 29 30 assert(temp_man != NULL); 31 assert(temp_bird != NULL); 32 33 temp_man ->set_owner(temp_bird); //FAILS on assert inside set_owner 34 temp_bird->set_owner(temp_man); 35 36 man_type *temp_man2 = new man_type(300, 300, &map); 37 bird_type *temp_bird2 = new bird_type(300, 300, &map); 38 39 assert(temp_man2 != NULL); 40 assert(temp_bird2 != NULL); 41 42 temp_man2 ->set_owner(temp_bird2); 43 temp_bird2->set_owner(temp_man2); 44 }

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

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

Edgar Reynaldo
Member #8,592
May 2007
avatar

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.

Chris Katko
Member #1,881
January 2002
avatar

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...

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

Edgar Reynaldo
Member #8,592
May 2007
avatar

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.

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.

Chris Katko
Member #1,881
January 2002
avatar

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.

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

Edgar Reynaldo
Member #8,592
May 2007
avatar

Chris Katko
Member #1,881
January 2002
avatar

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

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

bamccaig
Member #7,536
July 2006
avatar

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...

Thomas Fjellstrom
Member #476
June 2000
avatar

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

--
Thomas Fjellstrom - [website] - [email] - [Allegro Wiki] - [Allegro TODO]
"If you can't think of a better solution, don't try to make a better solution." -- weapon_S
"The less evidence we have for what we believe is certain, the more violently we defend beliefs against those who don't agree" -- https://twitter.com/neiltyson/status/592870205409353730

raynebc
Member #11,908
May 2010

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

GullRaDriel
Member #3,861
September 2003
avatar

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

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

Chris Katko
Member #1,881
January 2002
avatar

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.

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

Go to: