From 46060abeffd86ed80024a8b46e35f04519fff551 Mon Sep 17 00:00:00 2001 From: ThisIsFineTM <184989147+ThisIsFineTM@users.noreply.github.com> Date: Sun, 12 Jan 2025 23:47:46 -0500 Subject: [PATCH 1/2] Add test case which demonstrates #439 tools-test: Make codefactor happy with whitespace. --- test/tools-test.cpp | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/test/tools-test.cpp b/test/tools-test.cpp index 2651bfb4..9254c700 100644 --- a/test/tools-test.cpp +++ b/test/tools-test.cpp @@ -236,6 +236,10 @@ TEST(tools, isOutofBounds) TEST(tools, normalize_link) { + ASSERT_EQ(normalize_link("", ""), ""); + ASSERT_EQ(normalize_link("/", ""), ""); + ASSERT_EQ(normalize_link("", "/"), "/"); + ASSERT_EQ(normalize_link("/a", "/b"), "a"); // not absolute @@ -250,6 +254,28 @@ TEST(tools, normalize_link) // URI-decoding is performed ASSERT_EQ(normalize_link("/%41%62c", "/"), "Abc"); + + // #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", ""), ""); + + // ../test/tools-test.cpp:260: Failure + // Expected equality of these values: + // normalize_link("%", "/") + // Which is: "/\01bc" + // "/" + // + // ../test/tools-test.cpp:261: Failure + // Expected equality of these values: + // normalize_link("%1", "") + // Which is: "\x1" "1bc" + // "" + + // 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"); } TEST(tools, addler32) @@ -400,7 +426,6 @@ TEST(tools, getLinks) "abcd href = qwerty src={123} xyz", "" ); - } #undef EXPECT_LINKS From 491caf4d7bf546aec0767a89e234119359f63e9c Mon Sep 17 00:00:00 2001 From: ThisIsFineTM <184989147+ThisIsFineTM@users.noreply.github.com> Date: Mon, 13 Jan 2025 16:27:10 -0500 Subject: [PATCH 2/2] Fix #439 for '%XX' at the end of a link going off the end of the buffer. --- src/tools.cpp | 20 +++++++++++++------- test/tools-test.cpp | 8 ++++---- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/tools.cpp b/src/tools.cpp index 1459eb60..dfced863 100644 --- 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, "../", 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, "./", 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), "%2hhx", &ch); output += ch; p += 3; continue; diff --git a/test/tools-test.cpp b/test/tools-test.cpp index 9254c700..13b8eced 100644 --- a/test/tools-test.cpp +++ b/test/tools-test.cpp @@ -257,8 +257,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: @@ -274,8 +274,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)