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

[zaaarf]

Solves the problem described in #21992 (OP phrased it with more detail than I could come up with).

I didn't add a test because testing this problem kind of requires touching machine state (i.e. creating a dangling link somewhere), but let me know if I should still do it.

[LamentXU123]

I didn't add a test because testing this problem kind of requires touching machine state (i.e. creating a dangling link somewhere),

There is a --clean-- section you can fill in to clean your tmp symlinks. See if this test works:

  --TEST--
  GH-21992: symlink() must not resolve the final dangling link component
  --SKIPIF--
  <?php
  if (PHP_OS_FAMILY === 'Windows') {
      die('skip not for Windows');
  }
  ?>
  --FILE--
  <?php
  $dir = __DIR__ . '/gh21992';
  $targetDir = $dir . '/target_dir';
  $deadLink = $dir . '/dead_link.txt';
  $ghost = $targetDir . '/ghost.txt';
  $newTarget = $dir . '/something';

  mkdir($dir);
  mkdir($targetDir);

  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';

  @unlink($ghost);
  @unlink($deadLink);
  @rmdir($targetDir);
  @rmdir($dir);
  ?>
  --EXPECTF--
  bool(true)
  string(%d) "%s/gh21992/target_dir/ghost.txt"

  Warning: symlink(): File exists in %s on line %d
  bool(false)
  bool(true)
  string(%d) "%s/gh21992/target_dir/ghost.txt"
  bool(false)

[zaaarf]

There is a --clean-- section you can fill in to clean your tmp symlinks. See if this test works:

Oh cool, thanks, good to know. I'll do it in a bit.

[LamentXU123]

You may also need to add a new entry in NEWS and UPGRADING since this is a (minor) bug fix :)
The implementation makes sense (?) See if the CI is happy after the regression test.

[zaaarf]

needs a bit of work, found inconsistent behaviour

[zaaarf]

Wrote something that actually makes sense this time, and wrote a test.

Sorry about earlier. That's what I get for trying to venture in PHP codebase at 5 in the morning.

I do want to say though, I don't really like that the specific warning we are emitting for this case is "File exists". Could we go with something more specific, like, "Dangling symlink"?

[zaaarf]

Oh, yeah @LamentXU123 want me to mark you as co author of the test?

[LamentXU123]

I don't run the suggested test locally so it seems like I forgot to check the logic to deal with Windows

Oh, yeah @LamentXU123 want me to mark you as co author of the test?

You can if you want to, I don't have write access. Weilin Du 108666168+LamentXU123@users.noreply.github.com No worries about that I am happy to help :)

Oh, seems like You've fixed the test already :) Hmmmm, I think the test fail just because it didn't start with skip. Or maybe I am forgetting something. Thanks!

[LamentXU123]

Suggested change

	<?php
	if (PHP_OS_FAMILY === 'Windows') {
	die('not for Windows');
	}
	?>
	<?php
	if (PHP_OS_FAMILY === 'Windows') {
	die('skip not for Windows');
	}
	?>

SKIPIF section must have a output starting from skip. Now this may fix the CI on Windows.

[zaaarf]

That's on me I guess. For now I tried pasting the skipif from another symlink-related test, let's see if that works.

[LamentXU123]

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.

[zaaarf]

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

[LamentXU123]

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.

[zaaarf]

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.

[LamentXU123]

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

[zaaarf]

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.

[LamentXU123]

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

[LamentXU123]

I do want to say though, I don't really like that the specific warning we are emitting for this case is "File exists". Could we go with something more specific, like, "Dangling symlink"?

I don't like this either. This is actually misleading and frankly, wrong. The doc reads:

"The function fails, and issues E_WARNING, if link already exists."

So, it will surely be better to specify the error message. But lets wait for @devnexen 's opinion on this.

[zaaarf]

The doc reads

Fair enough, I took OP's word on that. As long as we respect the warning level, we should be respecting the docs, then, no matter what the message is.

[LamentXU123]

@zaaarf The CI is failing again but this is a known issue #21919. You can force push this again to rerun CI. The test now seems to be successfully passed except for the unreleated one. Thank you.

Also, you may need to remove the compile_commands.json you (accidentally) uploaded.

[zaaarf]

Lol, yeah. Will do. I'm usually more careful, not sure why I'm messing up so much in these.