Skip to content

Memory leaks #44

Closed
chriso opened this Issue Sep 18, 2012 · 33 comments

4 participants

@chriso
chriso commented Sep 18, 2012

We've got a long running process using GeoIP that leaks memory.

Valgrind shows the following leaks on a minimal lookupSync.

var geoip = require('geoip')
  , city = new geoip.City('/etc/GeoIPCity.dat');

console.log(city.lookupSync('8.8.8.8'));
...
==28923== 14 bytes in 1 blocks are definitely lost in loss record 8 of 41
==28923==    at 0x4C2ABC8: malloc (vg_replace_malloc.c:263)
==28923==    by 0x7911E7D: _GeoIP_iso_8859_1__utf8 (GeoIP.c:434)
==28923==    by 0x76FF504: geoip::City::lookupSync(v8::Arguments const&) (city.cc:106)
==28923==    by 0x50E8C9D: ??? (in /usr/lib64/libv8.so.3.13.6)
...
==28923== 111 (88 direct, 23 indirect) bytes in 1 blocks are definitely lost in loss record 24 of 41
==28923==    at 0x4C2ABC8: malloc (vg_replace_malloc.c:263)
==28923==    by 0x791418F: _extract_record (GeoIPCity.c:61)
==28923==    by 0x76FF210: geoip::City::lookupSync(v8::Arguments const&) (city.cc:83)
==28923==    by 0x50E8C9D: ??? (in /usr/lib64/libv8.so.3.13.6)
...
==28923== 175 (40 direct, 135 indirect) bytes in 1 blocks are definitely lost in loss record 27 of 41
==28923==    at 0x4C2A75E: operator new(unsigned long) (vg_replace_malloc.c:287)
==28923==    by 0x76FED69: geoip::City::New(v8::Arguments const&) (city.cc:41)
==28923==    by 0x50E92BD: ??? (in /usr/lib64/libv8.so.3.13.6)
...

Here's what I dug up after having a look at city.cc

  • _GeoIP_iso_8859_1__utf8 leaks memory on this line - the char* it returns needs to be free()'d
  • GeoIPRecord_delete(record) should be called before returning from lookupSync.
  • close() (which contains the GeoIP_delete(db)) should be called in City::~City() or at least exposed

I didn't submit a pull request with the fixes because there's probably memory leaks elsewhere in the bindings and I haven't got the relevant GeoIP database versions to work with.

@sajal
sajal commented Oct 29, 2012

Same thing in geoip.Region

var regiondb = new geoipmodule.Region('/tmp/GeoIPRegion.dat')
var foo;
var st = new Date();for(i=0;i<10000000;i++) { foo = regiondb.lookupSync("107.20.181.99") };console.log(new Date() - st)

Run the last line a few times... and you see the memory usage of node process only go up...
FWIW im on v0.4.6

@kuno
Owner
kuno commented Oct 30, 2012

sh*t, I don't know how to fix it......

@sajal sajal added a commit to sajal/GeoIP that referenced this issue Nov 11, 2012
@sajal sajal use proper delete methods as suggested by chriso in #44 9aff74d
@robertpitt

Is this issue resolved ?

@kuno
Owner
kuno commented Dec 8, 2012

Sorry, not yet.

This probably need pretty heavy changes from current codes. Unfortunately, I am busying for money now.

Maybe next week I'll have some free time, but not sure.

@robertpitt

I understand, I would like use it for my project but I need something tested and stable, your geop library looks the most efficient and cleanest by far. Just need stability.

I will watch your repo and hopefully you find some time :)

Thanks

@kuno
Owner
kuno commented Dec 8, 2012

thx, but to be honest, for me, this project is just a platform for learning code on node and some open source project experience.

I will try my best to make it better over time, but not guarantee it will be totally production ready.

If you really need something very very stable, probably you can choose some alternatives.

@sajal
sajal commented Dec 8, 2012

deleted previous comment, didnt realized my pull was merged.... my bad

@robertpitt

Hey Sajal, did you say you have fixed the memory leaks in your pull

@kuno
Owner
kuno commented Dec 17, 2012

I just push some changes in master branch:

740b742

Can you guy give it a try, and return some feedback about memory leak.

thanks.

@kuno
Owner
kuno commented Dec 19, 2012

It seems there still some memory problems.

This is memory testing report for 0.4.6
https://gist.github.com/4333807

This is memory testing report for 0.4.7pre
https://gist.github.com/4333801

@chriso
chriso commented Dec 19, 2012

Some of the leaks are from node.js - you can ignore those.

The following leak still needs to be fixed but isn't a huge problem since the allocation only happens once. You just need to call GeoIP_delete(db); in the City destructor.

==1525== 201 (40 direct, 161 indirect) bytes in 1 blocks are definitely lost in loss record 20 of 29
==1525==    at 0x4C2B1C7: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1525==    by 0x703405C: geoip::City::New(v8::Arguments const&) (in /home/vagrant/GeoIP/build/Release/geoip.node)
==1525==    by 0x6CB766: ??? (in /usr/bin/nodejs)

Have a look at this commit from our fork which fixed the lookupSync() leaks.

As for the Mismatched free() / delete / delete [] error, try replacing delete name with free(name)

const char * name = _GeoIP_iso_8859_1__utf8(record->city);
data->Set(String::NewSymbol("city"), String::New(name));
delete name; //replace with free(name);
@kuno
Owner
kuno commented Dec 20, 2012

@chriso

Hey, I test your code, it seems the memory leak is gone (at least in valgrind). awesome !

Can you make a pull request?

@chriso
chriso commented Dec 20, 2012

@kuno I explained in the initial bug report why I didn't submit a pull request. My commit only fixes leaks in lookupSync() since this is the only function that we rely on. All of the other async/sync variations using other databases will still leak. I don't have access to other GeoIP databases so I can't fix it for you.

I also explained how to close the leaks in the initial report

  • Call free() after you're done with the result of _GeoIP_iso_8859_1__utf8()
  • Call GeoIPRecord_delete(record) when you're done with a record
  • Call GeoIP_delete(db) when you're finished with the database (usually in the destructor)
@kuno
Owner
kuno commented Dec 22, 2012

Thanks, I follow the way you suggested.

It seems the most memory are gong, but seem like the memory leak when new a object still occurs.

Here is the report, https://gist.github.com/4357339.

Anyway, I decide to release current code as 0.4.7, as it will be better than 0.4.6 for most user.

@kuno
Owner
kuno commented May 2, 2013

@chriso

I made some change to code, make this module self-hosted. When I use valgrind to test memory leak, it seem there are no leak any more. But I am not sure...

See, https://gist.github.com/kuno/4357339

Can you help me to confirm that?

@chriso
chriso commented May 2, 2013

A few issues I can see (looking at city.cc only). On these lines you free the same memory twice. You need to remove the delete call.

GeoIP_delete(c->db);    // free()'s the gi reference & closes its fd
delete c->db;

Another thing, if a library is allocating memory and returning a pointer then you need to free the memory with the equivalent free function provided by the library.

I see a few cases where you call GeoIP_record_by_ipnum(c->db, ipnum); and then later free the pointer with delete record;. This is only working because they're using the same memory allocator, but keep in mind that the pointer might point to a struct with additional allocations. You need to replace the delete with GeoIPRecord_delete(record);.

@chriso
chriso commented May 2, 2013

@kuno we're running a fork of the library (https://github.com/sydneystockholm/GeoIP) with everything stripped out except for city.lookupSync.

Have a look at the code here => https://github.com/sydneystockholm/GeoIP/blob/master/src/city.cc

@kuno
Owner
kuno commented May 2, 2013

@chriso

Thanks for your suggestions, what about the situation of memory leak, as you might saw?

@chriso
chriso commented May 2, 2013

@kuno it's missing the summary output from valgrind?

@kuno
Owner
kuno commented May 2, 2013

@chriso

Yeah, it is a bit strange for me, too.

But it is actually no summary output from valgrind..., I am doing something wrong?

@chriso
chriso commented May 2, 2013

@kuno Are you running a mac? I've had a similar issue before. Try running through linux if you can

@kuno
Owner
kuno commented May 2, 2013

@chriso

No, I am running in a ubuntu 12.04 virtualbox vm.

It works fine previously.

@kuno
Owner
kuno commented May 2, 2013

@chriso

Did you saw some gdb, vgdb things in the output ? I suspect this might be the reason of what we saw?

@chriso
chriso commented May 2, 2013

@kuno Ok, not sure then. Does echo $? show valgrind exiting with 0?

@chriso
chriso commented May 2, 2013

@kuno The vgdb thing lets you attach a gdb process for debugging - just ignore it

@kuno
Owner
kuno commented May 2, 2013

@chriso

Yes, echo $? show 0 after running valgrind --leak-check=full -v test/memory_leak.js

@chriso
chriso commented May 2, 2013

@kuno No clue then, sorry

@kuno
Owner
kuno commented May 2, 2013

@chriso

Anyway, thx

@kuno
Owner
kuno commented May 3, 2013

@chriso

Do I need manually delete a pointer of instance it self, like:

City6 * c = ObjectWrap::Unwrap<geoip::City6>(args.This()); 
delete c; // ???

or v8's garbage collector will do the job?

@chriso
chriso commented May 3, 2013

@kuno don't delete it. You're getting a reference to the pointer when you need it but you want to keep it around (e.g. if you're calling city.lookupSync() you want the pointer to be around when you call city.lookupSync() again). It should be the destructors job (~City()) to cleanup the pointer when the object is being collected.

@chriso
chriso commented May 3, 2013

@kuno Send me an email directly if you want more help, cohara87@gmail.com

@kuno
Owner
kuno commented May 3, 2013

@chriso

Ok, thanks. very helpful.

@kuno
Owner
kuno commented Jan 13, 2014

Closing.

@kuno kuno closed this Jan 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.