Merge pull request #39 from maxmind/greg/fix-shared-mem-reads

Fix reading records at end of the database when using shared mem
maxmind · May 16, 2016 · 2053a85 · 2053a85
2 parents afa4597 + fb03442
commit 2053a85
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 11 deletions.
diff --git a/ChangeLog.md b/ChangeLog.md
@@ -1,5 +1,12 @@
 # Change Log #
 
+## 1.17 (2016-05-16)
+
+* Previously data records at the end of the database were incorrectly returned
+  as `null` values when using shared memory. This was due to attempting to
+  read beyond the end of the database. This bug only affected users using
+  `GEOIP_SHARED_MEMORY`.
+
 ## 1.16 (2016-01-29)
 
 * Fixed issue that could cause a notice about using a property on a non-object

diff --git a/src/geoip.inc b/src/geoip.inc
@@ -1397,12 +1397,12 @@ function _setup_segments($gi)
     $gi->databaseType = GEOIP_COUNTRY_EDITION;
     $gi->record_length = STANDARD_RECORD_LENGTH;
     if ($gi->flags & GEOIP_SHARED_MEMORY) {
-        $offset = @shmop_size($gi->shmid) - 3;
+        $offset = shmop_size($gi->shmid) - 3;
         for ($i = 0; $i < STRUCTURE_INFO_MAX_SIZE; $i++) {
-            $delim = @shmop_read($gi->shmid, $offset, 3);
+            $delim = shmop_read($gi->shmid, $offset, 3);
             $offset += 3;
             if ($delim == (chr(255) . chr(255) . chr(255))) {
-                $gi->databaseType = ord(@shmop_read($gi->shmid, $offset, 1));
+                $gi->databaseType = ord(shmop_read($gi->shmid, $offset, 1));
                 if ($gi->databaseType >= 106) {
                     $gi->databaseType -= 105;
                 }
@@ -1432,7 +1432,7 @@ function _setup_segments($gi)
                     || ($gi->databaseType == GEOIP_ASNUM_EDITION_V6)
                 ) {
                     $gi->databaseSegments = 0;
-                    $buf = @shmop_read($gi->shmid, $offset, SEGMENT_RECORD_LENGTH);
+                    $buf = shmop_read($gi->shmid, $offset, SEGMENT_RECORD_LENGTH);
                     for ($j = 0; $j < SEGMENT_RECORD_LENGTH; $j++) {
                         $gi->databaseSegments += (ord($buf[$j]) << ($j * 8));
                     }
@@ -1494,6 +1494,7 @@ function _setup_segments($gi)
                     || ($gi->databaseType == GEOIP_ASNUM_EDITION_V6)
                 ) {
                     $gi->databaseSegments = 0;
+
                     $buf = fread($gi->filehandle, SEGMENT_RECORD_LENGTH);
                     for ($j = 0; $j < SEGMENT_RECORD_LENGTH; $j++) {
                         $gi->databaseSegments += (ord($buf[$j]) << ($j * 8));
@@ -1525,12 +1526,20 @@ function _setup_segments($gi)
     return $gi;
 }
 
+# This should be only used for variable-length records where
+# $start + $maxLength may be greater than the shared mem size
+function _sharedMemRead($gi, $start, $maxLength)
+{
+    $readLength = min(shmop_size($gi->shmid) - $start, $maxLength);
+    return shmop_read($gi->shmid, $start, $readLength);
+}
+
 function geoip_open($filename, $flags)
 {
     $gi = new GeoIP;
     $gi->flags = $flags;
     if ($gi->flags & GEOIP_SHARED_MEMORY) {
-        $gi->shmid = @shmop_open(GEOIP_SHM_KEY, "a", 0, 0);
+        $gi->shmid = shmop_open(GEOIP_SHM_KEY, "a", 0, 0);
     } else {
         $gi->filehandle = fopen($filename, "rb") or trigger_error("GeoIP API: Can not open $filename\n", E_USER_ERROR);
         if ($gi->flags & GEOIP_MEMORY_CACHE) {
@@ -1686,8 +1695,7 @@ function _geoip_seek_country_v6($gi, $ipnum)
                 2 * $gi->record_length
             );
         } elseif ($gi->flags & GEOIP_SHARED_MEMORY) {
-            $buf = @shmop_read(
-                $gi->shmid,
+            $buf = _sharedMemRead($gi,
                 2 * $gi->record_length * $offset,
                 2 * $gi->record_length
             );
@@ -1733,8 +1741,8 @@ function _geoip_seek_country($gi, $ipnum)
                 2 * $gi->record_length
             );
         } elseif ($gi->flags & GEOIP_SHARED_MEMORY) {
-            $buf = @shmop_read(
-                $gi->shmid,
+            $buf = _sharedMemRead(
+                $gi,
                 2 * $gi->record_length * $offset,
                 2 * $gi->record_length
             );
@@ -1769,7 +1777,7 @@ function _common_get_org($gi, $seek_org)
 {
     $record_pointer = $seek_org + (2 * $gi->record_length - 1) * $gi->databaseSegments;
     if ($gi->flags & GEOIP_SHARED_MEMORY) {
-        $org_buf = @shmop_read($gi->shmid, $record_pointer, MAX_ORG_RECORD_LENGTH);
+        $org_buf = _sharedMemRead($gi, $record_pointer, MAX_ORG_RECORD_LENGTH);
     } else {
         fseek($gi->filehandle, $record_pointer, SEEK_SET);
         $org_buf = fread($gi->filehandle, MAX_ORG_RECORD_LENGTH);

diff --git a/src/geoipcity.inc b/src/geoipcity.inc
@@ -65,7 +65,7 @@ function _common_get_record($gi, $seek_country)
     if ($gi->flags & GEOIP_MEMORY_CACHE) {
         $record_buf = substr($gi->memory_buffer, $record_pointer, FULL_RECORD_LENGTH);
     } elseif ($gi->flags & GEOIP_SHARED_MEMORY) {
-        $record_buf = @shmop_read($gi->shmid, $record_pointer, FULL_RECORD_LENGTH);
+        $record_buf = _sharedMemRead($gi, $record_pointer, FULL_RECORD_LENGTH);
     } else {
         fseek($gi->filehandle, $record_pointer, SEEK_SET);
         $record_buf = fread($gi->filehandle, FULL_RECORD_LENGTH);

diff --git a/tests/CityTest.php b/tests/CityTest.php
@@ -36,4 +36,18 @@ public function testCity()
             geoip_country_code_by_addr($gi, "64.17.254.216")
         );
     }
+
+    public function testCityWithSharedMemory()
+    {
+        // HHVM doesn't support shared memory
+        if (defined('HHVM_VERSION')) {
+            $this->markTestSkipped();
+        }
+        geoip_load_shared_mem("tests/data/GeoIPCity.dat");
+
+        $gi = geoip_open("tests/data/GeoIPCity.dat", GEOIP_SHARED_MEMORY);
+        $record = geoip_record_by_addr($gi, "222.230.136.0");
+
+        $this->assertEquals('Setagaya', $record->city);
+    }
 }
diff --git a/tests/NetspeedcellTest.php b/tests/NetspeedcellTest.php
@@ -11,4 +11,20 @@ public function testNetspeedcell()
             geoip_org_by_addr($gi, "2.125.160.1")
         );
     }
+
+    public function testNetspeedcellWithSharedMemory()
+    {
+        // HHVM doesn't support shared memory
+        if (defined('HHVM_VERSION')) {
+            $this->markTestSkipped();
+        }
+        geoip_load_shared_mem("tests/data/GeoIPNetSpeedCell.dat");
+
+        $gi = geoip_open("tests/data/GeoIPNetSpeedCell.dat", GEOIP_SHARED_MEMORY);
+
+        $this->assertEquals(
+            'Dialup',
+            geoip_name_by_addr($gi, "2.125.160.1")
+        );
+    }
 }