Fix utl::splitLastFromConfigurationPath

* Fix regressions introduced with 5edefc801fb48559c8064003f23d22d838710ee4 "use
more string_view in unotools".  (Notably, misuses of two-argument std
string_view rfind are something to watch out for, see the commit message of
93e234c45c62af9d57041de676d888f7695ac0e8 "Fix a misuse of two-argument std
string_view rfind" for details.)

* Bring the implementation some more in accordance with the documentation, by
being stricter about handling invalid paths, and making sure to really assign
all of the input _sInPath to the output _rsLocalName in case of an invalid
path.

* Only &...;-decode the names of set elements in ['...'] and ["..."], not
anything else.

Change-Id: If01f4b34af42b0a594994b732d54f26695329286
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/140493
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

2 files changed, 56 insertions, 51 deletions

diff --git a/unotools/qa/unit/configpaths.cxx b/unotools/qa/unit/configpaths.cxx
index e0a92f37c894..7d9907d9e34d 100644
--- a/unotools/qa/unit/configpaths.cxx
+++ b/unotools/qa/unit/configpaths.cxx
@@ -47,9 +47,6 @@ public:
             CPPUNIT_ASSERT_EQUAL(OUString(""), path);
             CPPUNIT_ASSERT_EQUAL(OUString("foo"), last);
         }
-        // TODO: Broken since 5edefc801fb48559c8064003f23d22d838710ee4 "use more string_view in
-        // unotools" (expected "" vs. actual "/foo"):
-        if ((false))
         {
             // Already prior to 5edefc801fb48559c8064003f23d22d838710ee4 "use more string_view in
             // unotools", and in discordance with the documentation, this returned true (but
@@ -65,9 +62,6 @@ public:
             CPPUNIT_ASSERT_EQUAL(OUString("/foo/bar"), path);
             CPPUNIT_ASSERT_EQUAL(OUString("baz"), last);
         }
-        // TODO: Broken since 5edefc801fb48559c8064003f23d22d838710ee4 "use more string_view in
-        // unotools" (expected "/foo/bar" vs. actual "/foo/bar/baz"):
-        if ((false))
         {
             // Trailing slash accepted for backwards compatibility (cf
             // . "for backwards compatibility, ignore a final slash" comment in
@@ -77,9 +71,6 @@ public:
             CPPUNIT_ASSERT_EQUAL(OUString("/foo/bar"), path);
             CPPUNIT_ASSERT_EQUAL(OUString("baz"), last);
         }
-        // TODO: Broken since 5edefc801fb48559c8064003f23d22d838710ee4 "use more string_view in
-        // unotools" (expected true vs. actual false return):
-        if ((false))
         {
             OUString path, last;
             CPPUNIT_ASSERT(utl::splitLastFromConfigurationPath(
@@ -93,18 +84,14 @@ public:
             CPPUNIT_ASSERT_EQUAL(OUString(""), path);
             CPPUNIT_ASSERT_EQUAL(OUString("foo"), last);
         }
-        // TODO: Broken since 5edefc801fb48559c8064003f23d22d838710ee4 "use more string_view in
-        // unotools" (expected false vs. actual true return):
-        if ((false))
         {
-            // Already prior to 5edefc801fb48559c8064003f23d22d838710ee4 "use more string_view in
-            // unotools", and in discordance with the documentation, this set last to "foo" rather
-            // than "foo/" (but "If <var>_sInPath</var> could not be parsed as a valid configuration
-            // path, this is set to <var>_sInPath</var>"):
+            // In accordance with the documentation, this sets last to "foo/" ("If
+            // <var>_sInPath</var> could not be parsed as a valid configuration path, this is set to
+            // <var>_sInPath</var>"):
             OUString path, last;
             CPPUNIT_ASSERT(!utl::splitLastFromConfigurationPath(u"foo/", path, last));
             CPPUNIT_ASSERT_EQUAL(OUString(""), path);
-            CPPUNIT_ASSERT_EQUAL(OUString("foo"), last);
+            CPPUNIT_ASSERT_EQUAL(OUString("foo/"), last);
         }
         {
             // Some broken input missing a leading slash happens to be considered OK:
diff --git a/unotools/source/config/configpaths.cxx b/unotools/source/config/configpaths.cxx
index 4e5002238d93..4c626d96b4e0 100644
--- a/unotools/source/config/configpaths.cxx
+++ b/unotools/source/config/configpaths.cxx
@@ -19,6 +19,7 @@
 
 #include <sal/config.h>
 
+#include <cassert>
 #include <string_view>
 
 #include <unotools/configpaths.hxx>
@@ -78,70 +79,87 @@ bool splitLastFromConfigurationPath(std::u16string_view _sInPath,
 {
     size_t nStart,nEnd;
 
-    size_t nPos = _sInPath.size()-1;
+    size_t nPos = _sInPath.size();
 
-    // strip trailing slash
-    if (nPos != std::u16string_view::npos && nPos > 0 && _sInPath[ nPos ] == '/')
+    // for backwards compatibility, strip trailing slash
+    if (nPos > 1 && _sInPath[ nPos - 1 ] == '/')
     {
-        OSL_FAIL("Invalid config path: trailing '/' is not allowed");
         --nPos;
     }
 
-    // check for predicate ['xxx'] or ["yyy"]
-    if (nPos != std::u16string_view::npos && nPos > 0 && _sInPath[ nPos ] == ']')
+    // check for set element ['xxx'] or ["yyy"]
+    bool decode;
+    if (nPos > 0 && _sInPath[ nPos - 1 ] == ']')
     {
-        sal_Unicode chQuote = _sInPath[--nPos];
+        decode = true;
+        if (nPos < 3) { // expect at least chQuote + chQuote + ']' at _sInPath[nPos-3..nPos-1]
+            goto invalid;
+        }
+        nPos -= 2;
+        sal_Unicode chQuote = _sInPath[nPos];
 
         if (chQuote == '\'' || chQuote == '\"')
         {
             nEnd = nPos;
-            nPos = _sInPath.find(chQuote,nEnd);
+            nPos = _sInPath.rfind(chQuote,nEnd - 1);
+            if (nPos == std::u16string_view::npos) {
+                goto invalid;
+            }
             nStart = nPos + 1;
-            if (nPos != std::u16string_view::npos)
-                --nPos; // nPos = rInPath.lastIndexOf('[',nPos);
         }
-        else // allow [xxx]
+        else
         {
-            nEnd = nPos + 1;
-            nPos = _sInPath.rfind('[',nEnd);
-            nStart = nPos + 1;
+            goto invalid;
         }
 
-        OSL_ENSURE(nPos != std::u16string_view::npos && _sInPath[nPos] == '[', "Invalid config path: unmatched quotes or brackets");
-        if (nPos != std::u16string_view::npos && _sInPath[nPos] == '[')
+        OSL_ENSURE(nPos > 0 && _sInPath[nPos - 1] == '[', "Invalid config path: unmatched quotes or brackets");
+        if (nPos > 1 && _sInPath[nPos - 1] == '[')
+            // expect at least '/' + '[' at _sInPath[nPos-2..nPos-1]
         {
-            nPos =  _sInPath.rfind('/',nPos);
+            nPos =  _sInPath.rfind('/',nPos - 2);
+            if (nPos == std::u16string_view::npos) {
+                goto invalid;
+            }
         }
-        else // defined behavior for invalid paths
+        else
         {
-            nStart = 0;
-            nEnd = _sInPath.size();
-            nPos = std::u16string_view::npos;
+            goto invalid;
         }
 
     }
     else
     {
-        nEnd = nPos+1;
-        nPos = _sInPath.rfind('/',nEnd);
+        decode = false;
+        nEnd = nPos;
+        if (nEnd == 0) {
+            goto invalid;
+        }
+        nPos = _sInPath.rfind('/',nEnd - 1);
+        if (nPos == std::u16string_view::npos) {
+            goto invalid;
+        }
         nStart = nPos + 1;
     }
-    OSL_ASSERT( (nPos == std::u16string_view::npos ||
-                nPos < nStart) &&
-                nStart < nEnd &&
-                nEnd <= _sInPath.size() );
+    assert( nPos != std::u16string_view::npos &&
+            nPos < nStart &&
+            nStart <= nEnd &&
+            nEnd <= _sInPath.size() );
 
-    OSL_ASSERT(nPos == std::u16string_view::npos || _sInPath[nPos] == '/');
+    assert(_sInPath[nPos] == '/');
     OSL_ENSURE(nPos != 0 , "Invalid config child path: immediate child of root");
 
     _rsLocalName = _sInPath.substr(nStart, nEnd-nStart);
-    if (nPos > 0 && nPos != std::u16string_view::npos)
-        _rsOutPath = _sInPath.substr(0,nPos);
-    else
-        _rsOutPath.clear();
-    lcl_resolveCharEntities(_rsLocalName);
+    _rsOutPath = (nPos > 0) ? OUString(_sInPath.substr(0,nPos)) : OUString();
+    if (decode) {
+        lcl_resolveCharEntities(_rsLocalName);
+    }
+
+    return true;
 
-    return nPos != std::u16string_view::npos;
+invalid:
+    _rsOutPath.clear();
+    _rsLocalName = _sInPath;
+    return false;
 }
 
 OUString extractFirstFromConfigurationPath(OUString const& _sInPath, OUString* _sOutPath)