Conversation
eivindjahren
force-pushed
the
fix_util_clear_directory
branch
from
363784d to
b814384
Compare
eivindjahren
force-pushed
the
fix_util_clear_directory
branch
from
b814384 to
27b7b60
Compare
eivindjahren
force-pushed
the
fix_util_clear_directory
branch
from
27b7b60 to
b082dc7
Compare
eivindjahren
force-pushed
the
fix_util_clear_directory
branch
from
b082dc7 to
85cc2af
Compare
There was a problem hiding this comment.
Pull request overview
This PR resolves a code-scanning security finding by removing the bespoke recursive directory-deletion utility and switching test work-area cleanup to use std::filesystem.
Changes:
- Removed
util_clear_directoryimplementation (and its public header declaration). - Updated
TestAreateardown to delete its directory viastd::filesystem::remove_all. - Updated the lib build sources list to stop compiling the removed source file.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/util/util_getuid.cpp | Removes the legacy recursive directory-clearing implementation. |
| lib/util/test_work_area.cpp | Replaces cleanup with std::filesystem::remove_all guarded by exception handling. |
| lib/include/ert/util/util.hpp | Drops the util_clear_directory declaration from the installed public header. |
| lib/CMakeLists.txt | Removes the deleted util_getuid.cpp from the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TestArea::~TestArea() { | ||
| if (!this->store) | ||
| util_clear_directory(this->cwd.c_str(), true, true); | ||
| if (!this->store) { | ||
| try { | ||
| std::filesystem::remove_all(this->cwd); | ||
| } catch (std::filesystem::filesystem_error const &ex) { |
There was a problem hiding this comment.
std::filesystem::remove_all(this->cwd) is executed while the process is still chdir'd into this->cwd (you only change back to org_cwd afterwards). On POSIX this typically prevents removal of the directory itself (EBUSY), so cleanup may leave behind the (now empty) work directory. Consider changing back to org_cwd first (or otherwise ensure the current working directory is not inside this->cwd) before calling remove_all so the root directory can be deleted reliably.
There was a problem hiding this comment.
According to https://man7.org/linux/man-pages/man3/rmdir.3p.html this is unspecified, but could result in EBUSY. I changed this so that orig_cwd is restored first to avoid the issue.
This is different behavior in the case the function fails to remove a file, before it would silently fail.
The POSIX standard says that doing rmdir on the current working directory could result in EBUSY, but it is unspecified. So we move to orig_cwd before removing just in case.
eivindjahren
force-pushed
the
fix_util_clear_directory
branch
from
1872c0b to
174ecbf
Compare
| util_clear_directory(this->cwd.c_str(), true, true); | ||
| if (!this->store) { | ||
| std::error_code ec; | ||
| std::filesystem::remove_all(this->cwd, ec); |
There was a problem hiding this comment.
- The old implementation was called with
strict_uid=true. This would, as far as I can tell, only delete files owned by the user running the code.std::filesystem::remove_allwill remove all files that a user has access to even if they are not the owner. Is this change in behavior intended? - Apparently, this standard library implementation of
std::filesystem::remove_allhad the same vulnerability as resdata in the past (bug report for LLVM, and GCC). Do we need to verify that the compilers we use for building resdata have the fix? The issue is fixed in newer compilers and also has been backported.
There was a problem hiding this comment.
Ah, I missed that. That complicates this quite a bit so I think we will have to reconsider.
There was a problem hiding this comment.
I don't really know in which context the deletion is triggered. If this is only used in some kind of test cleanup it might be fine using the remove_all.
There was a problem hiding this comment.
Its probably fine since it is indeed test clean up. It is a really minor change in behavior and we could claim that changing the ownership inside the directory was never defined before or even that the test interface was never part of the public interface. I am leaning towards that we should also remove anything test related in the interface.
This should resolve https://github.com/equinor/resdata/security/code-scanning/6