bugfix: out-of-bounds memory writing may happen in chash_point_sort. openresty#41

Author: doujiang24

.travis.yml

@@ -21,32 +21,52 @@ addons:
     - liburi-perl
     - libwww-perl
     - perl
+    - valgrind
 
 cache:
   directories:
   - download-cache
 
 env:
   global:
-  - JOBS=2
-  - OPENRESTY_PREFIX=/usr/local/openresty
-  - OPENRESTY_VER=1.11.2.3
+    - JOBS=2
+    - OPENRESTY_PREFIX=/usr/local/openresty
+    - OPENRESTY_VER=1.19.9.1
+  jobs:
+    - TEST_NGINX_USE_VALGRIND=0
+    - TEST_NGINX_USE_VALGRIND=1
 
 install:
 - if [ ! -f download-cache/openresty-$OPENRESTY_VER.tar.gz ]; then
     wget -P download-cache https://openresty.org/download/openresty-$OPENRESTY_VER.tar.gz;
   fi
 - git clone https://github.com/openresty/test-nginx.git ../test-nginx
 
+# install openresty-openssl111-dev
+- sudo apt-get -y install --no-install-recommends wget gnupg ca-certificates
+- wget -O - https://openresty.org/package/pubkey.gpg | sudo apt-key add -
+- echo "deb http://openresty.org/package/ubuntu $(lsb_release -sc) main"
+  | sudo tee /etc/apt/sources.list.d/openresty.list
+- sudo apt-get update || echo 'apt-get update failed, but ignore it'
+- sudo apt-get -y install --no-install-recommends openresty-openssl111 openresty-openssl111-dev
+
 script:
+- if [ TEST_NGINX_USE_VALGRIND = 1 ]; then export luajit_xcflags='-DLUAJIT_USE_VALGRIND -DLUAJIT_USE_SYSMALLOC'; fi
 - tar xzf download-cache/openresty-$OPENRESTY_VER.tar.gz &&
     cd openresty-$OPENRESTY_VER
-- ./configure --prefix=$OPENRESTY_PREFIX -j$JOBS
-    > build.log 2>&1 || (cat build.log && exit 1)
+- ./configure --prefix=$OPENRESTY_PREFIX
+    --with-cc-opt="-I/usr/local/openresty/openssl111/include"
+    --with-ld-opt="-L/usr/local/openresty/openssl111/lib -Wl,-rpath,/usr/local/openresty/openssl111/lib"
+    --with-luajit-xcflags="$luajit_xcflags"
+    -j$JOBS
+        > build.log 2>&1 || (cat build.log && exit 1)
 - make -j$JOBS > build.log 2>&1 ||
     (cat build.log && exit 1)
 - sudo make install > build.log 2>&1 ||
     (cat build.log && exit 1)
 - cd ..
 - export PATH=$OPENRESTY_PREFIX/nginx/sbin:$PATH
-- make test jobs=$JOBS
+- make test jobs=$JOBS > build.log 2>&1 ||
+    (cat build.log && exit 1)
+- cat build.log
+- if [ `grep -c '== Invalid' build.log` -gt 0 ]; then echo 'valgrind complaining' && exit 1; fi

chash.c

@@ -1,6 +1,8 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <math.h>
+#include <assert.h>
+
 #include "chash.h"
 
 
@@ -164,7 +166,9 @@ chash_point_sort(chash_point_t arr[], uint32_t n)
 
     for (i = 0; i < n; i++) {
         node = &arr[i];
-        index = node->hash / step; // can not bigger than m
+        index = node->hash / step;
+
+        assert(index < m); // index must less than m
 
         for (end = index; end >= 0; end--) {
             if (points[end].id == 0) {
@@ -188,7 +192,10 @@ chash_point_sort(chash_point_t arr[], uint32_t n)
 
                 /* left shift after end when node->hash is bigger than them */
                 /* only end == index can match this */
-                while (points[end + 1].id != 0 && points[end + 1].hash < node->hash) {
+                while (end + 1 < m
+                       && points[end + 1].id != 0
+                       && points[end + 1].hash < node->hash)
+                {
                     points[end].hash = points[end + 1].hash;
                     points[end].id = points[end + 1].id;
                     end += 1;
@@ -223,6 +230,8 @@ chash_point_sort(chash_point_t arr[], uint32_t n)
         }
 
 insert:
+        assert(end < m && end >= 0);
+
         points[end].id = node->id;
         points[end].hash = node->hash;
     }

t/chash.t

@@ -356,3 +356,48 @@ size: 480
 --- no_error_log
 [error]
 --- timeout: 30
+
+
+
+=== TEST 6: random key fuzzer
+--- http_config eval: $::HttpConfig
+--- config
+    location /t {
+        content_by_lua_block {
+            math.randomseed(ngx.now())
+
+            local ffi = require "ffi"
+            local resty_chash = require "resty.chash"
+
+            local function random_string()
+                local len = math.random(10, 100)
+                local buf = ffi.new("char [?]", len)
+                for i = 0, len - 1 do
+                    buf[i] = math.random(0, 255)
+                end
+
+                return ffi.string(buf, len)
+            end
+
+            for i = 1, 30 do
+                local servers = {}
+
+                local len = math.random(1, 100)
+                for j = 1, len do
+                    local key = random_string()
+                    servers[key] = math.random(1, 100)
+                end
+
+                local chash = resty_chash:new(servers)
+            end
+
+            ngx.say("done")
+        }
+    }
+--- request
+GET /t
+--- response_body
+done
+--- no_error_log
+[error]
+--- timeout: 30