Skip to content

Commit

Permalink
Fix weakly_canonical with relative input paths.
Browse files Browse the repository at this point in the history 
When weakly_canonical was called with a relative input path, the operation
would test path elements for existence, which meant resolving them relative
to the current path instead of the base path specified in the call. To
mitigate this, make the source path absolute using the specified base path.

As a side effect, this fixes incorrect path produced on Windows if the
input path started with "..". The algorithm was unable to remove the last
element of the head path because there was none. As a result, the remaining
elements of the input path were appended to the full base path by canonical.

Fixes to #311.
  • Loading branch information
@Lastique
Lastique committed Jul 3, 2024
1 parent c8093ee commit 41a990e
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 62 deletions.
2 changes: 2 additions & 0 deletions doc/release_history.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ <h2>1.86.0</h2>
<li>On POSIX systems, <code>is_empty</code> now indicates error if invoked on a file other than a regular file or a directory.</li>
<li>On Windows, fixed <code>file_size</code> and <code>is_empty</code> operating on symlinks rather than the files the symlinks refer to. (<a href="https://github.com/boostorg/filesystem/issues/313">#313</a>)</li>
<li><code>directory_entry::refresh</code> no longer throws an exception if the file referenced by the entry doesn't exist. This makes <code>directory_entry::status</code> and <code>directory_entry::symlink_status</code>, as well as methods based on them, behave similarly to the equivalent standalone operations. The fact that the file does not exist is still indicated via the <code>error_code</code> returned by the corresponding <code>directory_entry::refresh</code> overload, or can be seen by testing if the file type returned by <code>directory_entry::status</code> or <code>directory_entry::symlink_status</code> calls is <code>file_type::file_not_found</code>. (<a href="https://github.com/boostorg/filesystem/issues/314">#314</a>)</li>
<li>Fixed <code>weakly_canonical</code> testing path elements for existence relative to the current path instead of the base path specified in the call, if the input path was a relative path.</li>
<li>On Windows, fixed <code>weakly_canonical</code> producing incorrect result path when the input path started with "..". (<a href="https://github.com/boostorg/filesystem/issues/311">#311</a>)</li>
</ul>

<h2>1.85.0</h2>
Expand Down
124 changes: 62 additions & 62 deletions src/operations.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2568,18 +2568,11 @@ path absolute_v4(path const& p, path const& base, system::error_code* ec)
BOOST_FILESYSTEM_DECL
path canonical_v3(path const& p, path const& base, system::error_code* ec)
{
if (ec)
ec->clear();

path source(p);
if (!p.is_absolute())
path source(detail::absolute_v3(p, base, ec));
if (ec && *ec)
{
source = detail::absolute_v3(p, base, ec);
if (ec && *ec)
{
return_empty_path:
return path();
}
return_empty_path:
return path();
}

system::error_code local_ec;
Expand Down Expand Up @@ -2700,18 +2693,11 @@ path canonical_v3(path const& p, path const& base, system::error_code* ec)
BOOST_FILESYSTEM_DECL
path canonical_v4(path const& p, path const& base, system::error_code* ec)
{
if (ec)
ec->clear();

path source(p);
if (!p.is_absolute())
path source(detail::absolute_v4(p, base, ec));
if (ec && *ec)
{
source = detail::absolute_v4(p, base, ec);
if (ec && *ec)
{
return_empty_path:
return path();
}
return_empty_path:
return path();
}

system::error_code local_ec;
Expand Down Expand Up @@ -4882,13 +4868,20 @@ path system_complete(path const& p, system::error_code* ec)
BOOST_FILESYSTEM_DECL
path weakly_canonical_v3(path const& p, path const& base, system::error_code* ec)
{
path source(detail::absolute_v3(p, base, ec));
if (ec && *ec)
{
return_empty_path:
return path();
}

system::error_code local_ec;
const path::iterator p_end(p.end());
const path::iterator source_end(source.end());

#if defined(BOOST_POSIX_API)

path::iterator itr(p_end);
path head(p);
path::iterator itr(source_end);
path head(source);
for (; !head.empty(); path_algorithms::decrement_v4(itr))
{
file_status head_status(detail::status_impl(head, &local_ec));
Expand All @@ -4898,7 +4891,7 @@ path weakly_canonical_v3(path const& p, path const& base, system::error_code* ec
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (head_status.type() != fs::file_not_found)
Expand All @@ -4925,18 +4918,18 @@ path weakly_canonical_v3(path const& p, path const& base, system::error_code* ec
// Also, avoid querying status of the root name such as \\?\c: as CreateFileW returns ERROR_INVALID_FUNCTION for
// such path. Querying the status of a root name such as c: is also not right as this path refers to the current
// directory on drive C:, which is not what we want to test for existence anyway.
path::iterator itr(p.begin());
path::iterator itr(source.begin());
path head;
if (p.has_root_name())
if (source.has_root_name())
{
BOOST_ASSERT(itr != p_end);
BOOST_ASSERT(itr != source_end);
head = *itr;
path_algorithms::increment_v4(itr);
}

if (p.has_root_directory())
if (source.has_root_directory())
{
BOOST_ASSERT(itr != p_end);
BOOST_ASSERT(itr != source_end);
// Convert generic separator returned by the iterator for the root directory to
// the preferred separator.
head += path::preferred_separator;
Expand All @@ -4952,36 +4945,36 @@ path weakly_canonical_v3(path const& p, path const& base, system::error_code* ec
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (head_status.type() == fs::file_not_found)
{
// If the root path does not exist then no path element exists
itr = p.begin();
itr = source.begin();
head.clear();
goto skip_head;
}
}

for (; itr != p_end; path_algorithms::increment_v4(itr))
for (; itr != source_end; path_algorithms::increment_v4(itr))
{
path const& p_elem = *itr;
path const& source_elem = *itr;

// Avoid querying status of paths containing dot and dot-dot elements, as this will break
// if the root name starts with "\\?\".
if (path_algorithms::compare_v4(p_elem, dot_p) == 0)
if (path_algorithms::compare_v4(source_elem, dot_p) == 0)
continue;

if (path_algorithms::compare_v4(p_elem, dot_dot_p) == 0)
if (path_algorithms::compare_v4(source_elem, dot_dot_p) == 0)
{
if (head.has_relative_path())
head.remove_filename_and_trailing_separators();

continue;
}

path_algorithms::append_v4(head, p_elem);
path_algorithms::append_v4(head, source_elem);

file_status head_status(detail::status_impl(head, &local_ec));
if (BOOST_UNLIKELY(head_status.type() == fs::status_error))
Expand All @@ -4990,7 +4983,7 @@ path weakly_canonical_v3(path const& p, path const& base, system::error_code* ec
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (head_status.type() == fs::file_not_found)
Expand All @@ -5006,7 +4999,7 @@ skip_head:;

path tail;
bool tail_has_dots = false;
for (; itr != p_end; path_algorithms::increment_v4(itr))
for (; itr != source_end; path_algorithms::increment_v4(itr))
{
path const& tail_elem = *itr;
path_algorithms::append_v4(tail, tail_elem);
Expand All @@ -5022,7 +5015,7 @@ skip_head:;
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (BOOST_LIKELY(!tail.empty()))
Expand All @@ -5031,7 +5024,7 @@ skip_head:;

// optimization: only normalize if tail had dot or dot-dot element
if (tail_has_dots)
return path_algorithms::lexically_normal_v4(head);
head = path_algorithms::lexically_normal_v4(head);
}

return head;
Expand All @@ -5040,13 +5033,20 @@ skip_head:;
BOOST_FILESYSTEM_DECL
path weakly_canonical_v4(path const& p, path const& base, system::error_code* ec)
{
path source(detail::absolute_v4(p, base, ec));
if (ec && *ec)
{
return_empty_path:
return path();
}

system::error_code local_ec;
const path::iterator p_end(p.end());
const path::iterator source_end(source.end());

#if defined(BOOST_POSIX_API)

path::iterator itr(p_end);
path head(p);
path::iterator itr(source_end);
path head(source);
for (; !head.empty(); path_algorithms::decrement_v4(itr))
{
file_status head_status(detail::status_impl(head, &local_ec));
Expand All @@ -5056,7 +5056,7 @@ path weakly_canonical_v4(path const& p, path const& base, system::error_code* ec
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (head_status.type() != fs::file_not_found)
Expand All @@ -5083,18 +5083,18 @@ path weakly_canonical_v4(path const& p, path const& base, system::error_code* ec
// Also, avoid querying status of the root name such as \\?\c: as CreateFileW returns ERROR_INVALID_FUNCTION for
// such path. Querying the status of a root name such as c: is also not right as this path refers to the current
// directory on drive C:, which is not what we want to test for existence anyway.
path::iterator itr(p.begin());
path::iterator itr(source.begin());
path head;
if (p.has_root_name())
if (source.has_root_name())
{
BOOST_ASSERT(itr != p_end);
BOOST_ASSERT(itr != source_end);
head = *itr;
path_algorithms::increment_v4(itr);
}

if (p.has_root_directory())
if (source.has_root_directory())
{
BOOST_ASSERT(itr != p_end);
BOOST_ASSERT(itr != source_end);
// Convert generic separator returned by the iterator for the root directory to
// the preferred separator.
head += path::preferred_separator;
Expand All @@ -5110,36 +5110,36 @@ path weakly_canonical_v4(path const& p, path const& base, system::error_code* ec
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (head_status.type() == fs::file_not_found)
{
// If the root path does not exist then no path element exists
itr = p.begin();
itr = source.begin();
head.clear();
goto skip_head;
}
}

for (; itr != p_end; path_algorithms::increment_v4(itr))
for (; itr != source_end; path_algorithms::increment_v4(itr))
{
path const& p_elem = *itr;
path const& source_elem = *itr;

// Avoid querying status of paths containing dot and dot-dot elements, as this will break
// if the root name starts with "\\?\".
if (path_algorithms::compare_v4(p_elem, dot_p) == 0)
if (path_algorithms::compare_v4(source_elem, dot_p) == 0)
continue;

if (path_algorithms::compare_v4(p_elem, dot_dot_p) == 0)
if (path_algorithms::compare_v4(source_elem, dot_dot_p) == 0)
{
if (head.has_relative_path())
head.remove_filename_and_trailing_separators();

continue;
}

path_algorithms::append_v4(head, p_elem);
path_algorithms::append_v4(head, source_elem);

file_status head_status(detail::status_impl(head, &local_ec));
if (BOOST_UNLIKELY(head_status.type() == fs::status_error))
Expand All @@ -5148,7 +5148,7 @@ path weakly_canonical_v4(path const& p, path const& base, system::error_code* ec
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (head_status.type() == fs::file_not_found)
Expand All @@ -5164,7 +5164,7 @@ skip_head:;

path tail;
bool tail_has_dots = false;
for (; itr != p_end; path_algorithms::increment_v4(itr))
for (; itr != source_end; path_algorithms::increment_v4(itr))
{
path const& tail_elem = *itr;
path_algorithms::append_v4(tail, tail_elem);
Expand All @@ -5180,7 +5180,7 @@ skip_head:;
BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

*ec = local_ec;
return path();
goto return_empty_path;
}

if (BOOST_LIKELY(!tail.empty()))
Expand All @@ -5189,7 +5189,7 @@ skip_head:;

// optimization: only normalize if tail had dot or dot-dot element
if (tail_has_dots)
return path_algorithms::lexically_normal_v4(head);
head = path_algorithms::lexically_normal_v4(head);
}

return head;
Expand Down
6 changes: 6 additions & 0 deletions test/operations_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2821,6 +2821,12 @@ void weakly_canonical_basic_tests()
BOOST_TEST_EQ(fs::weakly_canonical(dir / "no-such/foo/../bar"), dir / "no-such/bar");
BOOST_TEST_EQ(fs::weakly_canonical(dir / "../no-such/foo/../bar"), dir.parent_path() / "no-such/bar");
BOOST_TEST_EQ(fs::weakly_canonical(dir / "no-such/../f0"), dir / "f0"); // dir / "f0" exists, dir / "no-such" does not
BOOST_TEST_EQ(fs::weakly_canonical("f0", d1), d1 / "f0");
BOOST_TEST_EQ(fs::weakly_canonical("./f0", d1), d1 / "f0");
BOOST_TEST_EQ(fs::weakly_canonical("./foo", d1), d1 / "foo");
BOOST_TEST_EQ(fs::weakly_canonical("../f0", d1), dir / "f0");
BOOST_TEST_EQ(fs::weakly_canonical("../foo", d1), dir / "foo");
BOOST_TEST_EQ(fs::weakly_canonical("..//foo", d1), dir / "foo");

#ifdef BOOST_WINDOWS_API
BOOST_TEST_EQ(fs::weakly_canonical("c:/no-such/foo/bar"), fs::path("c:/no-such/foo/bar"));
Expand Down

0 comments on commit 41a990e

Please sign in to comment.