Skip to content

Fix util clear directory #1121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eivindjahren wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix util clear directory #1121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcabff8
Replace util_clear_directory with std::filesystem::remove_all()
eivindjahren Feb 13, 2026
174ecbf
Move to orig_cwd before cleanup to avoid EBUSY
eivindjahren Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
if(ERT_HAVE_GETUID AND ERT_HAVE_OPENDIR)
list(APPEND opt_srcs util/test_work_area.cpp util/util_getuid.cpp)
list(APPEND opt_srcs util/test_work_area.cpp)
endif()

if(ERT_HAVE_OPENDIR)
Expand Down
1 change: 0 additions & 1 deletion lib/include/ert/util/util.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ void util_make_path(const char *);
double util_file_difftime(const char *, const char *);
size_t util_file_size(const char *);
size_t util_fd_size(int fd);
void util_clear_directory(const char *path, bool strict_uid, bool unlink_root);
void util_strupr(char *);
Comment thread
eivindjahren marked this conversation as resolved.
bool util_string_equal(const char *s1, const char *s2);
char *util_alloc_strupr_copy(const char *);
Expand Down
11 changes: 7 additions & 4 deletions lib/util/test_work_area.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
#include <paths.h>
#endif

#include <ert/util/ert_api_config.hpp>

#include <cstdlib>
#include <cstdio>
#include <cstring>

#include <filesystem>

#include <ert/util/util.hpp>
#include <ert/util/test_work_area.hpp>

Expand Down Expand Up @@ -187,8 +187,11 @@ TestArea::TestArea(const std::string &test_name, bool store_area)
}

TestArea::~TestArea() {
if (!this->store)
util_clear_directory(this->cwd.c_str(), true, true);
if (!this->store) {
std::error_code ec;
std::filesystem::remove_all(this->cwd, ec);

@ajaust ajaust Apr 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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_all will 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_all had 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.

@eivindjahren eivindjahren Apr 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. That complicates this quite a bit so I think we will have to reconsider.

@ajaust ajaust Apr 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eivindjahren eivindjahren Apr 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ajaust ajaust Apr 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

// silently fail for backwards compatibility
}

util_chdir(this->org_cwd.c_str());
}
Expand Down
92 changes: 0 additions & 92 deletions lib/util/util_getuid.cpp
View file
Open in desktop

This file was deleted.