unotools/qa/unit/configpaths.cxx | 21 +-------
unotools/source/config/configpaths.cxx | 86 +++++++++++++++++++--------------
2 files changed, 56 insertions(+), 51 deletions(-)
New commits:
commit 8cadfec386d0fb906dbe9b8ea8f918e61927e622
Author: Stephan Bergmann <sberg...@redhat.com>
AuthorDate: Fri Sep 23 16:08:05 2022 +0200
Commit: Stephan Bergmann <sberg...@redhat.com>
CommitDate: Fri Sep 23 19:59:36 2022 +0200
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 <sberg...@redhat.com>
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)
