Merge pull request #4096 from mapnik/fix-out-of-bounds-array-access

avoid potential out-of-bounds array access (undefined behaviour)

.circleci/config.yml

@@ -18,8 +18,9 @@ jobs:
     # To see the list of pre-built images that CircleCI provides for most common languages see
     # https://circleci.com/docs/2.0/circleci-images/
     docker:
+      # https://discuss.circleci.com/t/aug-13-14-unable-to-run-the-job-runner-issue/31958
     - image: circleci/build-image:ubuntu-14.04-XXL-upstart-1189-5614f37
-      command: /sbin/init
+      # command: /sbin/init
     steps:
     - checkout
     # Prepare for artifact and test results  collection equivalent to how it was done on 1.0.

include/mapnik/text/scrptrun.hpp

@@ -17,12 +17,16 @@
 #ifndef __SCRPTRUN_H
 #define __SCRPTRUN_H
 
+#include <mapnik/config.hpp>
 #pragma GCC diagnostic push
 #include <mapnik/warning_ignore.hpp>
 #include <unicode/utypes.h>
 #include <unicode/uobject.h>
 #include <unicode/uscript.h>
 #pragma GCC diagnostic pop
+#include <vector>
+
+const unsigned int STACK_SIZE = 1 << 7; // 2^n
 
 struct ScriptRecord
 {
@@ -33,11 +37,13 @@ struct ScriptRecord
 
 struct ParenStackEntry
 {
+    ParenStackEntry(int32_t pairIndex_, UScriptCode scriptCode_)
+        : pairIndex(pairIndex_), scriptCode(scriptCode_) {}
     int32_t pairIndex = 0;
     UScriptCode scriptCode = USCRIPT_INVALID_CODE;
 };
 
-class ScriptRun : public icu::UObject {
+class MAPNIK_DECL ScriptRun : public icu::UObject {
 public:
     ScriptRun();
 
@@ -85,7 +91,7 @@ private:
     int32_t scriptEnd;
     UScriptCode scriptCode;
 
-    ParenStackEntry parenStack[128];
+    std::vector<ParenStackEntry> parenStack;
     int32_t parenSP;
 
     static int8_t highBit(int32_t value);
@@ -105,16 +111,19 @@ private:
 
 inline ScriptRun::ScriptRun()
 {
+    parenStack.reserve(STACK_SIZE);
     reset(nullptr, 0, 0);
 }
 
 inline ScriptRun::ScriptRun(const UChar chars[], int32_t length)
 {
+    parenStack.reserve(STACK_SIZE);
     reset(chars, 0, length);
 }
 
 inline ScriptRun::ScriptRun(const UChar chars[], int32_t start, int32_t length)
 {
+    parenStack.reserve(STACK_SIZE);
     reset(chars, start, length);
 }
 

src/text/scrptrun.cpp

@@ -22,7 +22,11 @@
 
 #include <mapnik/text/scrptrun.hpp>
 
-#define ARRAY_SIZE(array) (sizeof(array) / sizeof(array[0]))
+template <class T, std::size_t N>
+constexpr std::size_t ARRAY_SIZE(const T (&array)[N]) noexcept
+{
+    return N;
+}
 
 const char ScriptRun::fgClassID=0;
 
@@ -156,8 +160,9 @@ UBool ScriptRun::next()
         // characters above it on the stack will be poped.
         if (pairIndex >= 0) {
             if ((pairIndex & 1) == 0) {
-                parenStack[++parenSP].pairIndex = pairIndex;
+                ++parenSP;
-                parenStack[parenSP].scriptCode  = scriptCode;
+                parenStack.emplace_back(pairIndex, scriptCode);
+                startSP = parenSP;
             } else if (parenSP >= 0) {
                 int32_t pi = pairIndex & ~1;
 

test/build.py

@@ -21,6 +21,7 @@ else:
     test_env.AppendUnique(CXXFLAGS='-g')
     test_env['CXXFLAGS'] = copy(test_env['LIBMAPNIK_CXXFLAGS'])
     test_env.Append(CPPDEFINES = env['LIBMAPNIK_DEFINES'])
+
     if test_env['HAS_CAIRO']:
         test_env.PrependUnique(CPPPATH=test_env['CAIRO_CPPPATHS'])
         test_env.Append(CPPDEFINES = '-DHAVE_CAIRO')

test/unit/text/script_runs.cpp

@@ -0,0 +1,48 @@
+#include "catch.hpp"
+#include <unicode/unistr.h>
+#include <mapnik/unicode.hpp>
+#include <mapnik/text/scrptrun.hpp>
+
+TEST_CASE("nested script runs")
+{
+    mapnik::value_unicode_string text("Nested text runs(первый(second(третий)))"); //mixed scripts
+    ScriptRun runs(text.getBuffer(), text.length());
+    std::size_t count = 0;
+    std::size_t size = 0;
+    while (runs.next())
+    {
+        if (count & 1) CHECK(runs.getScriptCode() == USCRIPT_CYRILLIC);
+        else CHECK(runs.getScriptCode() == USCRIPT_LATIN);
+        size += runs.getScriptEnd() - runs.getScriptStart();
+        ++count;
+    }
+    REQUIRE(count == 7);
+    REQUIRE(size == text.length());
+}
+
+TEST_CASE("many punctuation chars")
+{
+    mapnik::value_unicode_string text((std::string(791, '(') + "test").data());
+    ScriptRun runs(text.getBuffer(), text.length());
+    while (runs.next())
+    {
+        CHECK(runs.getScriptCode() == 25);
+        CHECK(runs.getScriptStart() == 0);
+        CHECK(runs.getScriptEnd() == text.length());
+    }
+}
+
+TEST_CASE("empty runs")
+{
+    mapnik::value_unicode_string text("()text");
+    ScriptRun runs(text.getBuffer(), text.length());
+    std::size_t count = 0;
+    std::size_t size = 0;
+    while (runs.next())
+    {
+        size += runs.getScriptEnd() - runs.getScriptStart();
+        ++count;
+    }
+    REQUIRE(count == 1);
+    REQUIRE(size == text.length());
+}