Skip to content

[executor_manager] Move process stdout/stderr logs to top_dir #361

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

Merged
k82cn merged 1 commit into from
Mar 4, 2026
Merged

[executor_manager] Move process stdout/stderr logs to top_dir #361

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
10 changes: 4 additions & 6 deletions executor_manager/src/shims/host_shim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,11 +227,9 @@ impl HostShim {
// Spawn child process
let mut cmd = tokio::process::Command::new(&command);

// Use app_dir for logs and temp files (per-instance isolation)
// Use app_dir for temp files (per-instance isolation)
let app_work_dir = work_dir.app_dir();
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Choose a reason for hiding this comment

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

security-critical critical

The app_work_dir is obtained via work_dir.app_dir(), which returns a path constructed from the unsanitized app.name. If app.name contains path traversal sequences (e.g., ../../../../etc), this path can point to arbitrary locations outside the intended working directory. This is a critical security risk because the ExecutorWorkDir implementation (in mod.rs) recursively deletes this directory when it is dropped (fs::remove_dir_all(&self.app_dir)), which could lead to the destruction of sensitive system directories if a malicious application name is provided.

// Use process_dir for the actual process working directory
// - User-specified: runs in top_dir (to find pyproject.toml, etc.)
// - Auto-generated: runs in app_dir
// Use process_dir for actual process working directory and stdout/stderr logs
let process_work_dir = work_dir.process_dir();

// Setup working directory and tmp (per-instance)
Expand All @@ -254,15 +252,15 @@ impl HostShim {
.read(true)
.write(true)
.truncate(true)
.open(app_work_dir.join(format!("{}.out", executor.id)))
.open(process_work_dir.join(format!("{}.out", executor.id)))
.map_err(|e| FlameError::Internal(format!("failed to open stdout log file: {e}")))?;

let log_err = OpenOptions::new()
.create(true)
.read(true)
.write(true)
.truncate(true)
.open(app_work_dir.join(format!("{}.err", executor.id)))
.open(process_work_dir.join(format!("{}.err", executor.id)))
Comment on lines +255 to +263
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Choose a reason for hiding this comment

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

security-medium medium

The stdout and stderr log file paths are constructed by joining process_work_dir with a filename derived from executor.id. Since executor.id is not sanitized, an attacker can use path traversal sequences (e.g., ..) to create or overwrite files outside of the intended directory. Additionally, process_work_dir is derived from app.working_directory, which is also not validated and could be set to a sensitive absolute path. By moving the logs from app_work_dir to process_work_dir (which is higher in the directory hierarchy), this PR increases the potential scope of this path traversal vulnerability.

.map_err(|e| FlameError::Internal(format!("failed to open stderr log file: {e}")))?;

let child = cmd
Expand Down
6 changes: 3 additions & 3 deletions executor_manager/src/shims/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ pub type ShimPtr = Arc<Mutex<dyn Shim>>;

/// Represents the executor's working directory with cleanup management.
/// Directory structure:
/// top_dir/ - Process working directory (user-specified or auto-generated)
/// top_dir/work/<app_name>/ - App-specific directory for logs, tmp, cache
/// top_dir/ - Process working directory, stdout/stderr logs
/// top_dir/work/<app_name>/ - App-specific directory for tmp, cache
/// top_dir/work/<executor_id>.sock - Socket for gRPC communication
/// Cleanup:
/// - top_dir: cleaned up only if auto-generated
Expand Down Expand Up @@ -106,7 +106,7 @@ impl ExecutorWorkDir {
})
}

/// Returns the application working directory path (for logs, tmp, cache).
/// Returns the application working directory path (for tmp, cache).
pub fn app_dir(&self) -> &Path {
&self.app_dir
Comment on lines +109 to 111
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Choose a reason for hiding this comment

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

security-critical critical

The app_dir path returned by this method is constructed using the unsanitized app.name field. This allows for path traversal vulnerabilities where app_dir can point to arbitrary locations on the filesystem. This is particularly dangerous because the Drop implementation for ExecutorWorkDir calls fs::remove_dir_all on this path, potentially leading to the recursive deletion of critical system directories if a malicious application name is used.

}
Expand Down
27 changes: 15 additions & 12 deletions sdk/python/src/flamepy/runner/runpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,22 +187,25 @@ def _install_package_from_url(self, url: str) -> None:
except Exception as e:
raise RuntimeError(f"Failed to create storage backend: {e}")

# Get the working directory and tmp directory
working_dir = os.getcwd()
tmp_dir = os.path.join(working_dir, "tmp")
tmp_dir = os.environ.get("TMP", os.path.join(os.getcwd(), "tmp"))
os.makedirs(tmp_dir, exist_ok=True)

local_package_path = os.path.join(tmp_dir, filename)
try:
self._storage_backend.download(filename, local_package_path)
logger.info(f"Downloaded package to: {local_package_path}")
except Exception as e:
logger.error(f"Failed to download package from storage: {e}")
raise RuntimeError(f"Failed to download package from storage: {e}")
extract_dir = os.path.join(tmp_dir, f"extracted_{filename.split('.')[0]}")

logger.info("Package is an archive file, extracting...")
extract_dir = os.path.join(working_dir, f"extracted_{os.path.basename(local_package_path).split('.')[0]}")
extracted_dir = self._extract_archive(local_package_path, extract_dir)
if os.path.exists(extract_dir):
logger.info(f"Package already extracted at: {extract_dir}, skipping download")
extracted_dir = extract_dir
else:
try:
self._storage_backend.download(filename, local_package_path)
logger.info(f"Downloaded package to: {local_package_path}")
except Exception as e:
logger.error(f"Failed to download package from storage: {e}")
raise RuntimeError(f"Failed to download package from storage: {e}")

logger.info("Package is an archive file, extracting...")
extracted_dir = self._extract_archive(local_package_path, extract_dir)

install_path = extracted_dir
else:
Expand Down
Loading