Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#668 closed New feature (fixed)

piwik_log_visit.location_ip is negative on 32 bit systems

Reported by: rjmacy Owned by:
Priority: normal Milestone: Piwik 0.4
Component: Core Keywords:
Cc: Sensitive:

Description

In the visit.php file of Piwik_Tracker, use the function below instead of ip2long to get the right number .

function myip2long($ip){
if (is_numeric($ip)){
return sprintf("%u", floatval($ip));
} else {
return sprintf("%u", floatval(ip2long($ip)));
}
}

Attachments (2)

0.2.35.php (519 bytes) - added by vipsoft 5 years ago.
668.diffs (1.7 KB) - added by vipsoft 5 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 in reply to: ↑ description Changed 5 years ago by skenderback

Replying to rjmacy:

In the visit.php file of Piwik_Tracker, use the function below instead of ip2long to get the right number .

function myip2long($ip){
if (is_numeric($ip)){
return sprintf("%u", floatval($ip));
} else {
return sprintf("%u", floatval(ip2long($ip)));
}
}

comment:2 Changed 5 years ago by rjmacy

The new myip2long() function works on both 32 and 64 bit systems.

comment:3 Changed 5 years ago by matt (mattab)

what's the issue with the php core one?
what's the consequences in piwik?

comment:4 Changed 5 years ago by rjmacy

Matt, thanks for getting back to me. I've been using Piwik for a few weeks now and love it. I hope to produce a Plugin and write an article about it soon.
The issue is that on a 32 bit system, the ip2long() function overflows the signed int when we deal with an IP address with a high first octet. Consequently, a negative number gets placed in the DB in the location_ip field. One solution is to tell installers to use only 64 bit systems. Another solution is to implement a replacementment function for ip2long().

I suggest doing something like
function myip2long($ip){ $test = ip2long($ip);
if ( $test < 0)
{if (is_numeric($ip)){ return sprintf("%u", floatval($ip)); } else { return sprintf("%u", floatval(ip2long($ip))); } }
else return $test;}

comment:5 Changed 5 years ago by matt (mattab)

as long as long2ip is bijective (ie. negative int give back the same IP) this is not an issue is it?

comment:6 Changed 5 years ago by rjmacy

It is an issue because the number in the database is negative and the SQL function inet_ntoa() reports a NULL value when used on it. The location_ip column should be an unsigned int and we should be using inet_aton() to store the data there.
I am writing a big plugin and had to perform extra modifications to the Piwik code to get everything to work.

comment:7 Changed 5 years ago by vipsoft (robocoder)

  • Priority changed from major to normal
  • Summary changed from location_ip error on 32 bit systems to piwik_log_visit.location_ip is negative on 32 bit systems
  • Type changed from Bug to New feature

Matt's point is that the issue isn't impacting Piwik because PHP's long2ip() is able to handle the negative int -- arising from the fact that PHP doesn't have an unsigned int type.

However, I'm not opposed to the proposed change if it make its easier for plugin authors and/or when accessing the database directly. However, changing location_ip to "int unsigned" isn't exactly non-trivial. For example, the following SQL replaces negative numbers with 0:

ALTER TABLE piwik_log_visit CHANGE location_ip location_ip INT UNSIGNED;

Assuming the aforementioned roadblock is overcome, the ip2long() wrapper can probably be simplified as:

function myip2long($ip) {
    return sprintf("%u", ip2long($ip));
}

comment:8 Changed 5 years ago by rjmacy

I agree that this change is not simple, but currently I have to store the IP address in a new colomn in dotted decimal form in order to get data from MySQL into other systems. This means I have two columns that contain basically the same data, but one is in unusable form. My manager needs the data in a usable form.

I'm introducing ten programmers to Piwik and we plan on making it our company's standard for this type of data collection. As the user base of Piwik grows I think we would want to be able to pull SQL reports out of the DB directly and not have to rely on the front end for everything. BTW, you guys are doing a great job!

comment:9 Changed 5 years ago by vipsoft (robocoder)

Ok, working in our favor is that location_ip is a BIGINT. We'll want to keep this as a bigint so we can store ipv6 addresses (someday).

Perhaps something like the following?

UPDATE log_visit SET location_ip=location_ip+POW(2,32) WHERE location_ip < 0;
ALTER TABLE log_visit CHANGE location_ip location_ip BIGINT UNSIGNED;

comment:10 Changed 5 years ago by vipsoft (robocoder)

And for consistency, the change should also be applied to caller_ip.

Changed 5 years ago by vipsoft (robocoder)

Changed 5 years ago by vipsoft (robocoder)

comment:11 Changed 5 years ago by vipsoft (robocoder)

matt: the update script has to be renamed, but otherwise I believe this patch is good to go. Please review.

Before update:

+-------------+
| location_ip |
+-------------+
|          -1 | 
| -1062731519 | 
|  2130706433 | 
+-------------+

After update:

+-------------+
| location_ip |
+-------------+
|  4294967295 | 
|  3232235777 | 
|  2130706433 | 
+-------------+

+------------------------+
| inet_ntoa(location_ip) |
+------------------------+
| 255.255.255.255        | 
| 192.168.1.1            | 
| 127.0.0.1              | 
+------------------------+

comment:12 Changed 5 years ago by rjmacy

It works great. Thanks.

comment:13 Changed 5 years ago by vipsoft (robocoder)

BTW the -1 was dummy data that I inserted for testing.

comment:14 Changed 5 years ago by matt (mattab)

vipsoft, getDottedIp could be renamed getIpString.

Will the PHP call: long2ip( $visitorlocation_ip? ) - used in plugins/Live/Visitor.php - still return the original IP as expected?

comment:15 Changed 5 years ago by matt (mattab)

  • Milestone changed from 1- RobotRock to 1 - Piwik 0.4

comment:16 Changed 5 years ago by vipsoft (robocoder)

Ok. I'll rename it.

Yup, long2ip() works as expected, eg long2ip(-1) and long2ip(4294967295) both return 255.255.255.255.

comment:17 Changed 5 years ago by matt (mattab)

lgtm

("looks good to me" that is sometimes used for successful patch reviews)

comment:18 Changed 5 years ago by vipsoft (robocoder)

  • Resolution set to fixed
  • Status changed from new to closed

(In [1140]) fixes #668 - store location_ip as unsigned

comment:19 Changed 5 years ago by alivenk

comment:20 Changed 5 years ago by vipsoft (robocoder)

(In [1350]) refs #668 - fix inconsistency between fresh install and upgrade path

Note: See TracTickets for help on using tickets.