Skip to content

Handle realpath()/getRealPath() failures #1663

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
bpfoley wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle realpath()/getRealPath() failures #1663

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
6 changes: 4 additions & 2 deletions pinc/Project.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down
13 changes: 5 additions & 8 deletions pinc/misc.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,24 +1050,21 @@ function flatten_directory(string $directory_to_flatten): void
$files = new RecursiveIteratorIterator($iterator, RecursiveIteratorIterator::CHILD_FIRST);

foreach ($files as $file) {
$real_path = $file->getRealPath();

@cpeel cpeel Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If $real_path is false here the file doesn't exist (which I'm unclear on how that can actually be possible given that RecursiveIteratorIterator just told us about it, but let's humor PHPstan's paranoia about race conditions) and it would be clearer to continue than try to do something with the file / directory.

Changes like this seem like we're trading readability for PHPstan's paranoia.

@bpfoley bpfoley Sep 30, 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.

PHPStan knows nothing about race conditions or the inner workings of PHP's realpath, all it knows is the function signature in one of its stub files. Unfortunately, the underlying libc function realpath() can fail for a variety of (admittedly unlikely) reasons even if the file/directory does exist. PHP returns false in those cases. See https://man7.org/linux/man-pages/man3/realpath.3.html#ERRORS

I'm not sure silently suppressing the error with if ($real_path === false) continue; is the right thing to do, especially if the reason is that something else on the system has caused the directory to be inaccessible (eg a sysadmin doing chmod 0 on the uploads directory). In that case realpath would silently fail multiple times potentially leaving an unflattened directory behind it. But given the aim is to get PHPstan level 7 working so we can detect more useful missing error handling, I don't really mind too much what way we handle this.

if ($file->isDir()) {
$result = rmdir($file->getRealPath());

if (!$result) {
if ($real_path === false || !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 ($real_path === false || !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
));
Expand Down
7 changes: 4 additions & 3 deletions tools/project_manager/remote_file_manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.)
Expand Down