Fix #439 for '%XX' at the end of a link going off the end of the buffer.

openzim · Jan 13, 2025 · 167d8a8 · 167d8a8
1 parent 65226ca
commit 167d8a8
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 11 deletions.
diff --git a/src/tools.cpp b/src/tools.cpp
@@ -404,23 +404,24 @@ std::string normalize_link(const std::string& input, const std::string& baseUrl)
     output.reserve(baseUrl.size() + input.size() + 1);
 
     bool check_rel = false;
-    const char* p = input.c_str();
+    auto p = input.cbegin();
     if ( *(p) == '/') {
       // This is an absolute url.
       p++;
     } else {
       //This is a relative url, use base url
       output = baseUrl;
-      if (!output.empty() && output.back() != '/')
+      if (!output.empty() && output.back() != '/') {
           output += '/';
+      }
       check_rel = true;
     }
 
     //URL Decoding.
-    while (*p)
+    while (p < input.cend())
     {
         if ( check_rel ) {
-            if (strncmp(p, "../", 3) == 0) {
+            if (std::strncmp(p.base(), "../", 3) == 0) {
                 // We must go "up"
                 // Remove the '/' at the end of output.
                 output.resize(output.size()-1);
@@ -432,7 +433,7 @@ std::string normalize_link(const std::string& input, const std::string& baseUrl)
                 check_rel = false;
                 continue;
             }
-            if (strncmp(p, "./", 2) == 0) {
+            if (std::strncmp(p.base(), "./", 2) == 0) {
                 // We must simply skip this part
                 // Simply move after the ".".
                 p += 2;
@@ -449,8 +450,13 @@ std::string normalize_link(const std::string& input, const std::string& baseUrl)
 
         if ( *p == '%')
         {
-            char ch;
-            sscanf(p+1, "%2hhx", &ch);
+            if( (p+3) >= input.cend()){
+                // if the %XX token would go off the end of the string, just break
+                break;
+            }
+            // hhx only officially supports hex unsigned char
+            unsigned char ch = 0;
+            std::sscanf((p+1).base(), "%2hhx", &ch);
             output += ch;
             p += 3;
             continue;

diff --git a/test/tools-test.cpp b/test/tools-test.cpp
@@ -258,8 +258,8 @@ TEST(tools, normalize_link)
 
     // #439: normalized link reading off end of buffer
     // small-string-opt sizes, so sanitizers and valgrind don't pick this up
-    EXPECT_EQ(normalize_link("%", "/"), "/");
-    EXPECT_EQ(normalize_link("%1", ""), "");
+    ASSERT_EQ(normalize_link("%", "/"), "/");
+    ASSERT_EQ(normalize_link("%1", ""), "");
 
     // ../test/tools-test.cpp:260: Failure
     // Expected equality of these values:
@@ -275,8 +275,8 @@ TEST(tools, normalize_link)
 
     // test outside of small-string-opt
     // valgrind will pick up on the error in this one
-    EXPECT_EQ(normalize_link("qrstuvwxyz%", "/abcdefghijklmnop"), "/abcdefghijklmnop/qrstuvwxyz");
-    EXPECT_EQ(normalize_link("qrstuvwxyz%1", "/abcdefghijklmnop"), "/abcdefghijklmnop/qrstuvwxyz");
+    ASSERT_EQ(normalize_link("qrstuvwxyz%", "/abcdefghijklmnop"), "/abcdefghijklmnop/qrstuvwxyz");
+    ASSERT_EQ(normalize_link("qrstuvwxyz%1", "/abcdefghijklmnop"), "/abcdefghijklmnop/qrstuvwxyz");
 }
 
 TEST(tools, addler32)