Skip to content

Fix GH-21992: ext/standard: Fix symlink trying to resolve dangling links #22024

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
zaaarf wants to merge 5 commits into
base: PHP-8.4
Choose a base branch
from
+86 −0
Open

Fix GH-21992: ext/standard: Fix symlink trying to resolve dangling links #22024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a11da6f
ext/standard: Fix symlink trying to resolve dangling links
zaaarf May 12, 2026
7bd502d
try to fix test
zaaarf May 13, 2026
5b7b6d8
actually fix windows test
zaaarf May 13, 2026
cd6ddb8
better warning message
zaaarf May 13, 2026
b8b1b39
fix test for windows
zaaarf May 13, 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
10 changes: 10 additions & 0 deletions ext/standard/link.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,21 @@ PHP_FUNCTION(symlink)
char dirname[MAXPATHLEN];
size_t len;

zend_stat_t v_lstat = {0};
zend_stat_t v_stat = {0};

ZEND_PARSE_PARAMETERS_START(2, 2)
Z_PARAM_PATH(topath, topath_len)
Z_PARAM_PATH(frompath, frompath_len)
ZEND_PARSE_PARAMETERS_END();

// dangling link should be treated as existing file
ret = VCWD_LSTAT(frompath, &v_lstat);
if (!ret && S_ISLNK(v_lstat.st_mode) && VCWD_STAT(frompath, &v_stat)) {
php_error_docref(NULL, E_WARNING, "Dangling symlink at %s", frompath);
RETURN_FALSE;
}

Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 May 13, 2026

Choose a reason for hiding this comment

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

Now, I think there is a edge case,

You see, your check happens before we resolve the path from expand_filepath and since this is a file operation, we must consider race condition. A file can pass your check and suddenly be changed into dangling links to cause a bug.

I hope this make sense to you.

Copy link
Copy Markdown
Author

@zaaarf zaaarf May 13, 2026

Choose a reason for hiding this comment

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

I suppose, but I don't think there's a way to make the check + symlink operation atomic, is there?

Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there's a way to make the check + symlink operation atomic, is there?

AFAIK there may be, but only by letting the kernel do it, ideally via symlink() on the unmodified final path or symlinkat() on an opened parent directory.

Copy link
Copy Markdown
Author

@zaaarf zaaarf May 13, 2026

Choose a reason for hiding this comment

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

or symlinkat() on an opened parent directory.

Would that check for the symlink being dangling though? If so, that's pretty convenient. We have zend_dirname to get tofd and its length, at least.

Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

or symlinkat() on an opened parent directory.

Would that check for the symlink being dangling though? If so, that's pretty convenient. We have zend_dirname to get tofd and its length, at least.

Yes.

  • if the final name does not exist, it creates the new symlink
  • if the final name already exists as anything at all, including a dangling symlink, it fails with EEXIST

Copy link
Copy Markdown
Author

@zaaarf zaaarf May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Okay well, are you sure? I tried with symlinkat just now and I'm getting the same buggy behaviour described by OP (at least, I think so? The test is failing because symlink now returns true on PHP). I removed my dangling check, and I'm replacing the php_sys_symlink call with this:

const char *basename = source_p + len + 1; // +1 to remove the slash
int dirfd = open(dirname, O_RDONLY | O_DIRECTORY);
ret = symlinkat(topath, dirfd, basename);
close(dirfd);

I'm going to sleep for tonight now, so don't worry if I don't reply for a while.

Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 May 13, 2026

Choose a reason for hiding this comment

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

This seems fine. Let's wait for David's opinion :)

if (!expand_filepath(frompath, source_p)) {
php_error_docref(NULL, E_WARNING, "No such file or directory");
RETURN_FALSE;
Expand Down
76 changes: 76 additions & 0 deletions ext/standard/tests/gh21992t.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
--TEST--
GH-21992: symlink() must not resolve the final dangling link component
--SKIPIF--
<?php
if (PHP_OS_FAMILY === 'Windows') {
die('skip (not for Windows)');
}
?>
Comment thread
zaaarf marked this conversation as resolved.
--FILE--
<?php
$dir = __DIR__ . '/gh21992';
$targetDir = $dir . '/target_dir';

// dangling link to nonexisting $ghost
$deadLink = $dir . '/dead_link.txt';
$ghost = $targetDir . '/ghost.txt';
$newTarget = $dir . '/nothing';

// real link to $realFile
$realLink = $dir . '/real_link.txt';
$realFile = $targetDir . '/real.txt';
$realTarget = $dir . '/something';

mkdir($dir);
mkdir($targetDir);
touch($realFile); // ensure $realFile exists

// verify the real file works fine
var_dump(symlink($realFile, $realLink));
var_dump(readlink($realLink));
var_dump(symlink($realTarget, $realLink));
var_dump(is_link($realLink));
var_dump(readlink($realLink));
var_dump(is_link($realFile));

// verify the ghost file works as intended
var_dump(symlink($ghost, $deadLink));
var_dump(readlink($deadLink));
var_dump(symlink($newTarget, $deadLink));
var_dump(is_link($deadLink));
var_dump(readlink($deadLink));
var_dump(is_link($ghost));
?>
--CLEAN--
<?php
$dir = __DIR__ . '/gh21992';
$targetDir = $dir . '/target_dir';
$deadLink = $dir . '/dead_link.txt';
$ghost = $targetDir . '/ghost.txt';
$realLink = $dir . '/real_link.txt';
$realFile = $targetDir . '/real.txt';

@unlink($ghost);
@unlink($deadLink);
@unlink($realLink);
@unlink($realFile);
@rmdir($targetDir);
@rmdir($dir);
?>
--EXPECTF--
bool(true)
string(%d) "%s%egh21992%etarget_dir%ereal.txt"

Warning: symlink(): File exists in %s on line %d
bool(false)
bool(true)
string(%d) "%s%egh21992%etarget_dir%ereal.txt"
bool(false)
bool(true)
string(%d) "%s%egh21992%etarget_dir%eghost.txt"

Warning: symlink(): Dangling symlink at %s on line %d
bool(false)
bool(true)
string(%d) "%s%egh21992%etarget_dir%eghost.txt"
bool(false)
Loading