Memory leaks

[chriso]

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]

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]

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

[robertpitt]

Is this issue resolved ?

[kuno]

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]

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]

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]

I just push some changes in master branch:

740b7427fca24cbb5e7af14e717472e6538ac4ce

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

thanks.

[kuno]

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]

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]

@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]

@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]

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]

@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]

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]

@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]

@chriso

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

[chriso]

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

[kuno]

@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]

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

[kuno]

@chriso

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

It works fine previously.

[kuno]

@chriso

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

[chriso]

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

[chriso]

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

[kuno]

@chriso

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

[chriso]

@kuno No clue then, sorry

[kuno]

@chriso

Anyway, thx

[kuno]

@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]

@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]

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

[kuno]

@chriso

Ok, thanks. very helpful.

[kuno]

Closing.