[libc++] Fix UB in filesystem::__copy for non-existent destination.

[ivafanas]

lstat/stat/fstat has no guarantee whether struct stat buffer is changed or not on failure. filesystem::__copy function assumes, that struct stat buffer is not updated on lstat/stat/fstat failure, which is not true.

It appears that for non-existing destination detail::posix_lstat(to, t_st, &m_ec1) returns failure indicator and overwrites struct stat buffer with garbage values, which accidentally equal to the f_st from stack internals from the previous detail::posix_lstat(from, f_st, &m_ec1) call.

file_type::not_found is a known status, so check against if (not status_known(t)) is passed and execution continues further. Then __copy function returns errc::function_not_supported, because stats are accidentally equivalent, which is a mistake.

Before checking for detail::stat_equivalent, need to make sure, that lstat/stat/fstat functions returned success.

As soon as f_st and t_st access without checking for the lstat/stat/fstat success indicator is an UB, it is not needed to zero-initialize them.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Afanasyev Ivan (ivafanas)

Changes

lstat/stat/fstat has no guarantee whether struct stat buffer is chaged or not on failure. filesystem::__copy functon assumes, that struct stat buffer is not updated on lstat/stat/fstat failure, which is not true.

It appears that for non-existing destination
detail::posix_lstat(to, t_st, &m_ec1) returns failure indicator and overwrites struct stat buffer with a garbage values, which accidentially equal to the f_st from the stack internals from the previous
detail::posix_lstat(from, f_st, &m_ec1) call.

file_type::not_found is a known status, so check against if (not status_known(t)) is passed and execution continues further. Then __copy function returns errc::function_not_supported, because stats are accidentially equivalent, which is a mistake.

Before checking for detail::stat_equivalent, need to make sure, that lstat/stat/fstat functions returned success.

As soon as, f_st and t_st usage without checking for the lstat/stat/fstat success indicator is an UB, it is not needed to zero-initialize them.

---

Full diff: https://github.com/llvm/llvm-project/pull/87615.diff

1 Files Affected:

• (modified) libcxx/src/filesystem/operations.cpp (+3-3)

diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 62bb248d5eca9b..87b38fbdecf4ed 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -109,20 +109,20 @@ void __copy(const path& from, const path& to, copy_options options, error_code*
   const bool sym_status2 = bool(options & copy_options::copy_symlinks);
 
   error_code m_ec1;
-  StatT f_st = {};
+  StatT f_st;
   const file_status f =
       sym_status || sym_status2 ? detail::posix_lstat(from, f_st, &m_ec1) : detail::posix_stat(from, f_st, &m_ec1);
   if (m_ec1)
     return err.report(m_ec1);
 
-  StatT t_st          = {};
+  StatT t_st;
   const file_status t = sym_status ? detail::posix_lstat(to, t_st, &m_ec1) : detail::posix_stat(to, t_st, &m_ec1);
 
   if (not status_known(t))
     return err.report(m_ec1);
 
   if (!exists(f) || is_other(f) || is_other(t) || (is_directory(f) && is_regular_file(t)) ||
-      detail::stat_equivalent(f_st, t_st)) {
+      (exists(t) && detail::stat_equivalent(f_st, t_st))) {
     return err.report(errc::function_not_supported);
   }
 

[ivafanas]

@EricWF, could you please review the request.

I do not have commit access. If there are no objections, could you please merge the request into repo.
Thanks.

[ivafanas]

Could anybody please review the PR.

[ivafanas]

Could anybody please review the PR?
Any comments?

[4-rodrigo-salazar]

I've reviewed this change and it looks good to me. I'm not an approver in this repo though :)

Relatedly, I have gone over the rest of this file to find other potentially incorrect error_code paths in #88341 and found a couple more issues (less serious than the bug fix in this diff).

[ivafanas]

Ping.

Please, note that the issue is not the theoretical research.
We've actually triggered this bug on some linux distribution.

[EricWF]

Are there tests for this? If so, I wonder why MSAN didn't catch it.

Could you please confirm or add a test?

[ivafanas]

It is existing test failure:

std/input.output/filesystems/fs.op.funcs/fs.op.copy/copy.pass.cpp
TEST_CASE(from_is_regular_file)

Issue depends on fstat/lstat/stat posix functions implementation.

At first, I thought, that problem is in linux itself, but documentation states that struct stat state is undefined if fstat/lstat/stat returned "failure" indicator. So, it happens that bug is in fs::__copy. It must not assume that struct stat state is saved after fstat/lstat/stat call.

[ivafanas]

Ping.

Failure depends on posix functions implementation, that's why it isn't visible in regular CI.
We've caught the issue on the already existing tests on specific Linux version.

[ivafanas]

Hi.
Any comments?

We can reject the ticket if it is unclear.

[mordante]

Hi. Any comments?

We can reject the ticket if it is unclear.

I'll ping Eric for you. (Missing messages in the GitHub queue is not hard :-/)

[EricWF]

At first, I thought, that problem is in linux itself, but documentation states that struct stat state is undefined if fstat/lstat/stat returned "failure" indicator.

Which documentation are you referring to?

[EricWF]

LGTM once the questions are answered.

What OS and version are you running specifically to trigger this?

I'll leave it to @mordante to commit.

[ivafanas]

Hi!

I've searched for docs here:
https://linux.die.net/man/2/stat
https://man7.org/linux/man-pages/man2/stat.2.html

It does not state that buf parameter is not affected if error is returned. It is quite strange to update buf on failure, but, well, documentation allows it.

Problem is found on kernel 5.4.0 for the custom linux provided by our customer. Have no idea whether posix implementation of stat function is standard or customized.

[ivafanas]

Hi!

Let's close this pull request if nobody is interested.

I do not want to track it anymore. We can keep the fix in our customer fork without sharing it to community.

[ldionne]

I went through the PR explanation and the discussions since the start of the PR, and reviewed the code itself.

This seems like a real problem to me, only one that we can't realistically find in our CI testing environments because of how our OSes implement these low level functions. I'm happy to hear that our existing tests would actually catch this issue on an OS where the stat functions are implemented in that particular (but legal) way.

I think there's no reason to hold off with this patch any longer. Thanks for the report, explanation and the fix @ivafanas .

[ivafanas]

Louis, thank you!