Skip to content
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

Single file: Guard against partial cleanup of extracted files #32649

Merged
merged 1 commit into from
Feb 25, 2020
Merged

Single file: Guard against partial cleanup of extracted files #32649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 52 additions & 3 deletions src/installer/corehost/cli/apphost/bundle/dir_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,23 @@ void dir_utils_t::remove_directory_tree(const pal::string_t& path)

for (const pal::string_t &dir : dirs)
{
remove_directory_tree(dir);
pal::string_t dir_path = path;
append_path(&dir_path, dir.c_str());

remove_directory_tree(dir_path);
}

std::vector<pal::string_t> files;
pal::readdir(path, &files);

for (const pal::string_t &file : files)
{
if (!pal::remove(file.c_str()))
pal::string_t file_path = path;
append_path(&file_path, file.c_str());

if (!pal::remove(file_path.c_str()))
{
trace::warning(_X("Failed to remove temporary file [%s]."), file.c_str());
trace::warning(_X("Failed to remove temporary file [%s]."), file_path.c_str());
}
}

Expand All @@ -91,3 +97,46 @@ void dir_utils_t::fixup_path_separator(pal::string_t& path)
}
}
}

// Retry the rename operation with some wait in between the attempts.
// This is an attempt to workaround for possible file locking caused by AV software.

bool dir_utils_t::rename_with_retries(pal::string_t& old_name, pal::string_t& new_name, bool& dir_exists)
{
for (int retry_count=0; retry_count < 500; retry_count++)
{
if (pal::rename(old_name.c_str(), new_name.c_str()) == 0)
{
return true;
}
bool should_retry = errno == EACCES;
lpereira marked this conversation as resolved.
Show resolved Hide resolved

if (pal::directory_exists(new_name))
{
// Check directory_exists() on each run, because a concurrent process may have
// created the new_name directory.
//
// The rename() operation above fails with errono == EACCESS if
// * Directory new_name already exists, or
// * Paths are invalid paths, or
// * Due to locking/permission problems.
// Therefore, we need to perform the directory_exists() check again.

dir_exists = true;
return false;
}

if (should_retry)
{
trace::info(_X("Retrying Rename [%s] to [%s] due to EACCES error"), old_name.c_str(), new_name.c_str());
pal::sleep(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop will run for at most 500 times, waiting at most 50s before trying. This is also called by commit_file(), so this can take up to 50s per file in the bundle. It looks to me that this is a bit too much...

There might be other files in the bundle that need renaming, so maybe instead of retrying them here, add it to a queue to try later?

Maybe keep track of the total amount of time taken to rename these things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpereira I agree that the worst case looks bad -- but it is extremely unlikely.
The long wait time can only happen if the rename succeeds close to the 500th iteration for each file. That is, when there is a real failure with EACCESS - the extraction aborts after the first file (after 50s).

The case when recovery is triggered is rare, and is not a path to be optimized. If the AV blocks for a long time after writing each file, then the recovery will need to wait for it. Still, it is not expected to take the full minute after each file is written.

While the wait time could be used to write other files that need to be recovered, the time gained is rather negligible, since the time to write these files is a few milliseconds, compared to the minute spent waiting for the AV.

I actually tried out writing the change you suggested. Please take a look at this diff. But I'm not sure it is work to take in that complexity in a servicing fix, given that this case is unlikely, and that such a case is hard to test. If you still think we need to take the change, I can add it on to the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a servicing fix and this case is rare, we can keep the current behaviour from this PR. However, keep the code from this diff somewhere, should we need it for the next servicing release (it looks pretty clean and makes the worst case scenario more manageable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lpereira.
Do you have any further comments/concerns on this PR?

continue;
}
else
{
return false;
}
}

return false;
}
1 change: 1 addition & 0 deletions src/installer/corehost/cli/apphost/bundle/dir_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace bundle
static void remove_directory_tree(const pal::string_t &path);
static void create_directory_tree(const pal::string_t &path);
static void fixup_path_separator(pal::string_t& path);
static bool rename_with_retries(pal::string_t& old_name, pal::string_t& new_name, bool &new_dir_exists);
};
}

Expand Down
237 changes: 152 additions & 85 deletions src/installer/corehost/cli/apphost/bundle/extractor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,53 +10,63 @@

using namespace bundle;

// Compute the final extraction location as:
// m_extraction_dir = $DOTNET_BUNDLE_EXTRACT_BASE_DIR/<app>/<id>/...
//
// If DOTNET_BUNDLE_EXTRACT_BASE_DIR is not set in the environment, the
// base directory defaults to $TMPDIR/.net
void extractor_t::determine_extraction_dir()
pal::string_t& extractor_t::extraction_dir()
{
if (!pal::getenv(_X("DOTNET_BUNDLE_EXTRACT_BASE_DIR"), &m_extraction_dir))
if (m_extraction_dir.empty())
{
if (!pal::get_default_bundle_extraction_base_dir(m_extraction_dir))
// Compute the final extraction location as:
// m_extraction_dir = $DOTNET_BUNDLE_EXTRACT_BASE_DIR/<app>/<id>/...
//
// If DOTNET_BUNDLE_EXTRACT_BASE_DIR is not set in the environment,
// a default is choosen within the temporary directory.

if (!pal::getenv(_X("DOTNET_BUNDLE_EXTRACT_BASE_DIR"), &m_extraction_dir))
{
trace::error(_X("Failure processing application bundle."));
trace::error(_X("Failed to determine location for extracting embedded files."));
trace::error(_X("DOTNET_BUNDLE_EXTRACT_BASE_DIR is not set, and a read-write temp-directory couldn't be created."));
throw StatusCode::BundleExtractionFailure;
if (!pal::get_default_bundle_extraction_base_dir(m_extraction_dir))
{
trace::error(_X("Failure processing application bundle."));
trace::error(_X("Failed to determine location for extracting embedded files."));
trace::error(_X("DOTNET_BUNDLE_EXTRACT_BASE_DIR is not set, and a read-write temp-directory couldn't be created."));
throw StatusCode::BundleExtractionFailure;
}
}
}

pal::string_t host_name = strip_executable_ext(get_filename(m_bundle_path));
append_path(&m_extraction_dir, host_name.c_str());
append_path(&m_extraction_dir, m_bundle_id.c_str());
pal::string_t host_name = strip_executable_ext(get_filename(m_bundle_path));
append_path(&m_extraction_dir, host_name.c_str());
append_path(&m_extraction_dir, m_bundle_id.c_str());

trace::info(_X("Files embedded within the bundled will be extracted to [%s] directory."), m_extraction_dir.c_str());
}

trace::info(_X("Files embedded within the bundled will be extracted to [%s] directory."), m_extraction_dir.c_str());
return m_extraction_dir;
}

// Compute the working extraction location for this process, before the
// extracted files are committed to the final location
// m_working_extraction_dir = $DOTNET_BUNDLE_EXTRACT_BASE_DIR/<app>/<proc-id-hex>
void extractor_t::determine_working_extraction_dir()
pal::string_t& extractor_t::working_extraction_dir()
{
m_working_extraction_dir = get_directory(extraction_dir());
pal::char_t pid[32];
pal::snwprintf(pid, 32, _X("%x"), pal::get_pid());
append_path(&m_working_extraction_dir, pid);
if (m_working_extraction_dir.empty())
{
// Compute the working extraction location for this process,
// before the extracted files are committed to the final location
// working_extraction_dir = $DOTNET_BUNDLE_EXTRACT_BASE_DIR/<app>/<proc-id-hex>

dir_utils_t::create_directory_tree(m_working_extraction_dir);
m_working_extraction_dir = get_directory(extraction_dir());
pal::char_t pid[32];
pal::snwprintf(pid, 32, _X("%x"), pal::get_pid());
append_path(&m_working_extraction_dir, pid);

trace::info(_X("Temporary directory used to extract bundled files is [%s]."), m_working_extraction_dir.c_str());
}

trace::info(_X("Temporary directory used to extract bundled files is [%s]."), m_working_extraction_dir.c_str());
return m_working_extraction_dir;
}

// Create a file to be extracted out on disk, including any intermediate sub-directories.
FILE* extractor_t::create_extraction_file(const pal::string_t& relative_path)
{
pal::string_t file_path = m_working_extraction_dir;
pal::string_t file_path = working_extraction_dir();
append_path(&file_path, relative_path.c_str());

// m_working_extraction_dir is assumed to exist,
// working_extraction_dir is assumed to exist,
// so we only create sub-directories if relative_path contains directories
if (dir_utils_t::has_dirs_in_path(relative_path))
{
Expand Down Expand Up @@ -92,29 +102,6 @@ void extractor_t::extract(const file_entry_t &entry, reader_t &reader)
fclose(file);
}

pal::string_t& extractor_t::extraction_dir()
{
if (m_extraction_dir.empty())
{
determine_extraction_dir();
}

return m_extraction_dir;
}

bool extractor_t::can_reuse_extraction()
{
// In this version, the extracted files are assumed to be
// correct by construction.
//
// Files embedded in the bundle are first extracted to m_working_extraction_dir
// Once all files are successfully extracted, the extraction location is
// committed (renamed) to m_extraction_dir. Therefore, the presence of
// m_extraction_dir means that the files are pre-extracted.

return pal::directory_exists(extraction_dir());
}

void extractor_t::begin()
{
// Files are extracted to a specific deterministic location on disk
Expand All @@ -126,58 +113,138 @@ void extractor_t::begin()
//
// In order to solve these issues, we implement a extraction as a two-phase approach:
// 1) Files embedded in a bundle are extracted to a process-specific temporary
// extraction location (m_working_extraction_dir)
// 2) Upon successful extraction, m_working_extraction_dir is renamed to the actual
// extraction location (m_extraction_dir)
// extraction location (working_extraction_dir)
// 2) Upon successful extraction, working_extraction_dir is renamed to the actual
// extraction location (extraction_dir)
//
// This effectively creates a file-lock to protect against races and failed extractions.

determine_working_extraction_dir();

dir_utils_t::create_directory_tree(working_extraction_dir());
}

void extractor_t::clean()
{
dir_utils_t::remove_directory_tree(working_extraction_dir());
}

void extractor_t::commit()
void extractor_t::commit_dir()
{
// Commit files to the final extraction directory
// Commit an entire new extraction to the final extraction directory
// Retry the move operation with some wait in between the attempts. This is to workaround for possible file locking
// caused by AV software. Basically the extraction process above writes a bunch of executable files to disk
// and some AV software may decide to scan them on write. If this happens the files will be locked which blocks
// our ablity to move them.
int retry_count = 500;
while (true)

bool extracted_by_concurrent_process = false;
bool extracted_by_current_process =
dir_utils_t::rename_with_retries(working_extraction_dir(), extraction_dir(), extracted_by_concurrent_process);

if (extracted_by_concurrent_process)
{
if (pal::rename(m_working_extraction_dir.c_str(), m_extraction_dir.c_str()) == 0)
break;
// Another process successfully extracted the dependencies
trace::info(_X("Extraction completed by another process, aborting current extraction."));
clean();
}

bool should_retry = errno == EACCES;
if (can_reuse_extraction())
{
// Another process successfully extracted the dependencies
trace::info(_X("Extraction completed by another process, aborting current extraction."));
if (!extracted_by_current_process && !extracted_by_concurrent_process)
{
trace::error(_X("Failure processing application bundle."));
trace::error(_X("Failed to commit extracted files to directory [%s]."), extraction_dir().c_str());
throw StatusCode::BundleExtractionFailure;
}

dir_utils_t::remove_directory_tree(m_working_extraction_dir);
break;
}
trace::info(_X("Completed new extraction."));
}

if (should_retry && (retry_count--) > 0)
{
trace::info(_X("Retrying extraction due to EACCES trying to rename the extraction folder to [%s]."), m_extraction_dir.c_str());
pal::sleep(100);
continue;
}
else
{
trace::error(_X("Failure processing application bundle."));
trace::error(_X("Failed to commit extracted files to directory [%s]."), m_extraction_dir.c_str());
throw StatusCode::BundleExtractionFailure;
}
void extractor_t::commit_file(const pal::string_t& relative_path)
{
// Commit individual files to the final extraction directory.

pal::string_t working_file_path = working_extraction_dir();
append_path(&working_file_path, relative_path.c_str());

pal::string_t final_file_path = extraction_dir();
append_path(&final_file_path, relative_path.c_str());

if (dir_utils_t::has_dirs_in_path(relative_path))
{
dir_utils_t::create_directory_tree(get_directory(final_file_path));
}

bool extracted_by_concurrent_process = false;
bool extracted_by_current_process =
dir_utils_t::rename_with_retries(working_file_path, final_file_path, extracted_by_concurrent_process);

if (extracted_by_concurrent_process)
{
// Another process successfully extracted the dependencies
trace::info(_X("Extraction completed by another process, aborting current extraction."));
}

if (!extracted_by_current_process && !extracted_by_concurrent_process)
{
trace::error(_X("Failure processing application bundle."));
trace::error(_X("Failed to commit extracted files to directory [%s]."), extraction_dir().c_str());
throw StatusCode::BundleExtractionFailure;
}

trace::info(_X("Extraction recovered [%s]"), relative_path.c_str());
}

void extractor_t::extract(const manifest_t& manifest, reader_t& reader)
void extractor_t::extract_new(reader_t& reader)
{
begin();
for (const file_entry_t& entry : manifest.files) {
for (const file_entry_t& entry : m_manifest.files)
{
extract(entry, reader);
}
commit();
commit_dir();
}

// Verify an existing extraction contains all files listed in the bundle manifest.
// If some files are missing, extract them individually.
void extractor_t::verify_recover_extraction(reader_t& reader)
{
pal::string_t& ext_dir = extraction_dir();
bool recovered = false;

for (const file_entry_t& entry : m_manifest.files)
{
pal::string_t file_path = ext_dir;
append_path(&file_path, entry.relative_path().c_str());

if (!pal::file_exists(file_path))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experimented using readdir() / FindFirstFile()/FindNextFile() APIs in place of file_exists() checks on each file.
That is, create a dictionary of files in the the extraction location (and its subdirectories), and check that it has all entries in the manifest.

However, there wasn't a significant difference in startup time due to the change -- especially since the startup impact is already small. Given this and the fact that .net 5 will focus on moving away from extraction, I kept the file_exists checks to keep the logic simple.

{
if (!recovered)
{
recovered = true;
begin();
}

extract(entry, reader);
commit_file(entry.relative_path());
}
}

if (recovered)
{
clean();
}
}

pal::string_t& extractor_t::extract(reader_t& reader)
{
if (pal::directory_exists(extraction_dir()))
{
trace::info(_X("Reusing existing extraction of application bundle."));
verify_recover_extraction(reader);
}
else
{
trace::info(_X("Starting new extraction of application bundle."));
extract_new(reader);
}

return m_extraction_dir;
}
Loading