libgeoip: add GeoIP_free() function

[bassosimone]

Recently I spent some time on Windows trying to debug a crash occurring in free() caused by the fact that libgeoip was using the MT release C runtime, while my library was using the debug version of the same runtime.

While that should have been avoidable by making sure I was compiling with the correct runtime, I think it should also be nice to have an API that calls the free() matching the malloc() that has been used by GeoIP to allocate memory.

This way, one can just use GeoIP_free() to free memory that was allocated by GeoIP without wondering about the runtime that is used. A similar concept is freeaddrinfo() in libc.

Does this approach make sense?

P.S. I know that I should migrate soon to libmaxminddb and I have that quite high in my TODO list but having this diff in libGeoIP will also probably be useful to me for a couple of months :).

Thanks!

[gvanem]

I even experienced the same crash in test/test-geoip.c (MaxMind's own code) when the pointer from GeoIP_database_info() was called with free(db_info).

It be great if you could update your PR for this case too.

[oschwald]

Given that this seems to be happening to multiple Windows users, I think we could merge this PR with two changes: (1) the change @pprindeville suggested, (2) updating the documentation to more carefully clarify that this should not be used in place of GeoIP_delete() given the similar names.

[bassosimone]

@oschwald:

(2) updating the documentation to more carefully clarify that this should not be used in place of GeoIP_delete() given the similar names.

Good point. I can perhaps give it a less ambiguous name, like GeoIP_free_string(). OK?

Side question. IIRC libmaxminddb does not have a similar functionality. Would object to me writing a similar diff for libmaxminddb?

[oschwald]

I don't think libmaxminddb allocates and returns anything where we don't provide a function to deallocate it. Were you thinking of something in particular?

Maybe we could call this GeoIP_string_delete to match all the other *_delete functions.

[bassosimone]

I don't think libmaxminddb allocates and returns anything where we don't provide a function to deallocate it. Were you thinking of something in particular?

I was guessing. I have not used libmaxminddb much so far, so I don' know it very well. Having read the docs (very nice docs, btw!) to double check, I can confirm that my guess was not correct.

Maybe we could call this GeoIP_string_delete to match all the other *_delete functions.

Sure, I'll make the changes now!

[bassosimone]

Maybe we could call this GeoIP_string_delete to match all the other *_delete functions.

Sure, I'll make the changes now!

Done, thank you!

[oschwald]

@pprindeville, it is now named GeoIP_string_delete and, as far as I can tell, that is the only thing this is needed for. The other things allocated on the heap and returned already have appropriate delete functions.

[bassosimone]

@oschwald: agreed, see 041b8f3

[pprindeville]

Okay

[oschwald]

Thanks!