From 28f601e40b2be4adf75d8a8d06f5169e4f40bc1e Mon Sep 17 00:00:00 2001 From: Brian Foley Date: Tue, 15 Sep 2026 22:06:16 +0100 Subject: [PATCH] Handle `realpath()`/`getRealPath()` failures Found by `phpstan analyze --level 7` --- crontab/CleanDownloadTemp.inc | 2 +- crontab/CleanUploadsTrash.inc | 2 +- pinc/Project.inc | 6 +++-- pinc/misc.inc | 26 +++++++++++++------ tools/project_manager/remote_file_manager.php | 7 ++--- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/crontab/CleanDownloadTemp.inc b/crontab/CleanDownloadTemp.inc index d6c078bdb..2f7d0eba1 100644 --- a/crontab/CleanDownloadTemp.inc +++ b/crontab/CleanDownloadTemp.inc @@ -9,7 +9,7 @@ class CleanDownloadTemp extends BackgroundJob public function work(): void { $download_temp_dir = realpath(SiteConfig::get()->dyn_dir . "/download_tmp"); - if (! is_dir($download_temp_dir)) { + if ($download_temp_dir === false || !is_dir($download_temp_dir)) { $this->stop_message = "No download temp directory found, nothing to do."; return; } diff --git a/crontab/CleanUploadsTrash.inc b/crontab/CleanUploadsTrash.inc index f218de8bd..b42740798 100644 --- a/crontab/CleanUploadsTrash.inc +++ b/crontab/CleanUploadsTrash.inc @@ -9,7 +9,7 @@ class CleanUploadsTrash extends BackgroundJob public function work(): void { $trash_dir = realpath(SiteConfig::get()->uploads_dir . "/" . SiteConfig::get()->uploads_subdir_trash); - if (! is_dir($trash_dir)) { + if ($trash_dir === false || !is_dir($trash_dir)) { $this->stop_message = "No trash directory found, nothing to do."; return; } diff --git a/pinc/Project.inc b/pinc/Project.inc index d844c8848..56eb3ae62 100644 --- a/pinc/Project.inc +++ b/pinc/Project.inc @@ -1336,7 +1336,7 @@ class Project // TODO: This function would be better in a ProjectPage object, but we // don't have one of those. $image_path = realpath("{$this->dir}/$image"); - if (file_exists($image_path)) { + if ($image_path !== false && file_exists($image_path)) { return filesize($image_path); } else { return null; @@ -1803,7 +1803,9 @@ class Project private function delete_file(string $path): void { // ensure $path is inside $this->dir - if (!str_starts_with(realpath($path), realpath($this->dir))) { + $real_path = realpath($path); + $real_dir = realpath($this->dir); + if ($real_path === false || $real_dir === false || !str_starts_with($real_path, $real_dir)) { throw new UnexpectedValueException("$path is not in $this->dir"); } if (is_dir($path)) { diff --git a/pinc/misc.inc b/pinc/misc.inc index c0bd029b1..1b9dda485 100644 --- a/pinc/misc.inc +++ b/pinc/misc.inc @@ -1049,25 +1049,35 @@ function flatten_directory(string $directory_to_flatten): void $iterator = new RecursiveDirectoryIterator($directory_to_flatten, RecursiveDirectoryIterator::SKIP_DOTS); $files = new RecursiveIteratorIterator($iterator, RecursiveIteratorIterator::CHILD_FIRST); + // Note that this directory tree walk and rename/remove is decidedly + // non-atomic, and all sorts of things could happen in the background + // causing any of the FS operations or path building operations to fail, + // including another process removing or changing permissions on the + // containing directory, or the process running out of memory. + // There's no sane recovery mechanism in those cases, so just fail loudly + // so the sysadmin (sorry!) can see something has happened and can pick + // up the pieces. foreach ($files as $file) { + if (($real_path = $file->getRealPath()) === false) { + throw new RuntimeException(sprintf( + _("Failed to get absolute path for '%s'."), + $file + )); + } if ($file->isDir()) { - $result = rmdir($file->getRealPath()); - - if (!$result) { + if (!rmdir($real_path)) { throw new RuntimeException(sprintf( _("Could not remove directory '%1\$s' while flattening '%2\$s'."), - $file->getRealPath(), + $real_path, $directory_to_flatten )); } } else { $new_name = $directory_to_flatten . '/' . $file->getFilename(); - $result = rename($file->getRealPath(), $new_name); - - if (!$result) { + if (!rename($real_path, $new_name)) { throw new RuntimeException(sprintf( _("Could not move file '%1\$s' to '%2\$s' while flattening '%3\$s'."), - $file->getRealPath(), + $real_path, $new_name, $directory_to_flatten )); diff --git a/tools/project_manager/remote_file_manager.php b/tools/project_manager/remote_file_manager.php index ba9545fa0..4ff540330 100644 --- a/tools/project_manager/remote_file_manager.php +++ b/tools/project_manager/remote_file_manager.php @@ -754,7 +754,9 @@ function canonicalize_path(string $relpath): ?string function get_current_dir_relative_path(string $home_dirname): string { global $uploads_dir, $commons_rel_dir; - $abs_uploads_dir = realpath($uploads_dir); + if (($abs_uploads_dir = realpath($uploads_dir)) === false) { + fatal_error(_("Could not find absolute path for uploads dir")); + } // Default to home dir if the invocation didn't set cdrp. $cdrp = $_REQUEST['cdrp'] ?? $home_dirname; @@ -766,8 +768,7 @@ function get_current_dir_relative_path(string $home_dirname): string // information about what files/directories exist on the system. $error_message = sprintf(_("'%s' does not exist, or is not a folder"), html_safe($cdrp)); - $abspath = realpath("$abs_uploads_dir/$cdrp"); - if ($abspath === false) { + if (($abspath = realpath("$abs_uploads_dir/$cdrp")) === false) { // (It's possible a user could get this without URL-tweaking, // if they deleted a directory but still had an old directory listing // in another browser window.)