nan.h error on node v0.10.22

[ashbrener]

We are experiencing a new error (very infrequently) since upgrading to node v0.10.22. The following message is displayed and the process exits immediately.

node: /opt/pre-publish/star/node_modules/orion/node_modules/geoip/node_modules/nan/nan.h:809: bool _NanGetExternalParts(v8::Handle<v8::Value>, const char**, size_t*): Assertion `val->IsString()' failed.
Aborted

I suspect it may have something to do with the infamous WalMart memory leak on closed handles fix.

[kuno]

thanks for report,

I'll take a look this weekend.

[kuno]

@ashleybrener

What the version of geoip you are using?

[kuno]

@ashleybrener

I've push some code in new unstable branch, can you give it a try?

[kuno]

Not repeatable, Closing.

[nfo]

For the ones having the issue, it can happen simply because the given IP is not a string, as said in the error message. For example null, a number, an object, a function, etc.
So it's way safer too always check the variable before calling lookup.

if (ip != null) && typeof ip === 'string'

But even with this check, I'm still having the issue randomly on geoip 0.5.0 to 0.5.2 and nodejs 0.8.18 and 0.10.21.

[kuno]

@nfo

Actually, some days ago, I've merged a pull request to try to solved this issue.

In short words, what this patch did is, if the given parameter is not a string, it just throw an error.

You can clone the repo and give it a try.

Also, since you mention, you've experienced some issues, can you give me some examples?

I'd live add them in the unit tests.

[nfo]

@kuno Thanks for the answer. I tried the pull request, which does more or less what I did by hand, and I still have crashes. It happens approximatively once per minute, in a system that handles several thousands requests per second. I think it would be easier for me to patch nan.h to make it return false instead of calling assert. I mean, ok, I probably sent a bad value, but at least I should be able to deal with the error. What do you think ?
I don't have time right now to replay the requests to see which IP could have cause the problem. Or worse, perhaps it happen in certain conditions, on a certain setup, on a certain load.

(by the way, it would be nice explaining why using synchronous methods is the recommended way, in usage.md)

[nfo]

Context: I still have the C++ assert (& abort) even with the last commits, the ones that check that the given ip or domain is a string with the function isValidIpORdomain().

So here is what I did to prevent my nodejs processes to crash: nfo@7c2330c
What I did is calling the function IsString() in the C code of geoip, before it calls nan.h. If IsString() returns false, then the lookup functions return a null value.
I don't make a pull request because I'm pretty sure you don't want this commit as is.

Apparently typeof val == 'string' in JS and IsString() in C do not always return the same thing. I process several millions of IPs per hour from all over the world. I had a crash every 10 minute or so. After 3 hours with this "fix", I don't have any crash anymore, and my dashboards show that the data are still ok.

[kuno]

@nfo

Thanks for you comments,
Yes, I am lacks the experiences that you have that to run millions lookups.

I'll definitely implement your idea into code.

[kuno]

@nfo

BTW, can you offer some details about the difference between

typeof val == 'string' in JS and IsString() in C

[nfo]

@kuno
I have to admit that I have no clue why IsString() is more restrictive, but I think it's not trolling if I say that we can never trust JS with these kind of stuff.

Perhaps you should call the IsString() function directly in isValidIpORdomain http://bespin.cz/~ondras/html/classv8_1_1Value.html#a14bd69255a9fd04fa641713188814958

Also I did not take the time trying to find which kind of value would make IsString() return false.

[kuno]

@nfo

good idea, I think we can expose a c++ function in js land to validate inputs.

Thanks.

[kuno]

@nfo

I've just add a natvie function isString to validate the put.

You can found committing details in here.

If you give it a try on your environment, I'll appreciated.

[kuno]

@nfo

By default, if the given input is not a string, it will throw an error or call the callback with the first argument is the error object.

But if you created the instance with a extra options object, and set the key silent to true, it just return null even with inputs is not valid string. e.g

var city  = new geoip.City('/path/to/db', {silent: true});

city.lookupSync(null) === null;

city.lookup(null, function(err, data) {
    // err is null
   // data is null
});