Skip to content

Mark buildFromIterator test as conflicting #11869

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.

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Aug 3, 2023

Commit 0b2e6bc started caching the directory entry type to improve performance. Shortly after, we've seen flaky failures of the buildFromIterator phar test.

When it fails, it's always a value error in the constructor of RecursiveDirectoryIterator::__construct() with a "no such file or directory" error. What's happening here is this:

  1. A parallel test creates a subdirectory in the current working dir.
  2. This test checks hasChildren() on a directory entry, the cached entry
    returns "yes" on the subdirectory.
  3. The parallel test finishes and removes the subdirectory.
  4. The constructor mentioned above is called, causing an exception
    because the directory is gone.

This race has always been possible, even before said commit. It's just that it was very hard to hit before: the expensive stat call made the race window hard to hit. The race is now easier to hit because of the caching that is fast.

Since there's many tests that modify the current working directory, it seems best to mark this as an "all" conflict. We cannot avoid every TOC-TOU race when working with files with these phar tests.

In particular, mounteddir.phpt caused every conflict I saw on CI, but there's more tests that create subdirectories in the current working directory.

EDIT: I actually should've targeted 8.1, I'll do the merge manually anyway so it doesn't really matter ig 🤷

Commit 0b2e6bc started caching the directory entry type to improve
performance. Shortly after, we've seen flaky failures of the
buildFromIterator phar test.

When it fails, it's always a value error in the constructor of
RecursiveDirectoryIterator::__construct() with a "no such file or
directory" error. What's happening here is this:
1) A parallel test creates a subdirectory in the current working dir.
2) This test checks hasChildren() on a directory entry, the cached entry
   returns "yes" on the subdirectory.
3) The parallel test finishes and removes the subdirectory.
4) The constructor mentioned above is called, causing an exception
   because the directory is gone.

This race has always been possible, even before said commit. It's just
that it was very hard to hit before: the expensive stat call made the
race window hard to hit. The race is now easier to hit because of the
caching that is fast.

Since there's many tests that modify the current working directory, it
seems best to mark this as an "all" conflict. We cannot avoid every
TOC-TOU race when working with files with these phar tests.

In particular, mounteddir.phpt caused every conflict I saw on CI, but
there's more tests that create subdirectories in the current working
directory.
@Girgias
Copy link
Member

Girgias commented Aug 4, 2023

Ah so that was the issue, I was finding it very confusing lol

@nielsdos nielsdos closed this in dc586b1 Aug 4, 2023
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
Commit 0b2e6bc started caching the directory entry type to improve
performance. Shortly after, we've seen flaky failures of the
buildFromIterator phar test.

When it fails, it's always a value error in the constructor of
RecursiveDirectoryIterator::__construct() with a "no such file or
directory" error. What's happening here is this:
1) A parallel test creates a subdirectory in the current working dir.
2) This test checks hasChildren() on a directory entry, the cached entry
   returns "yes" on the subdirectory.
3) The parallel test finishes and removes the subdirectory.
4) The constructor mentioned above is called, causing an exception
   because the directory is gone.

This race has always been possible, even before said commit. It's just
that it was very hard to hit before: the expensive stat call made the
race window hard to hit. The race is now easier to hit because of the
caching that is fast.

Since there's many tests that modify the current working directory, it
seems best to mark this as an "all" conflict. We cannot avoid every
TOC-TOU race when working with files with these phar tests.

In particular, mounteddir.phpt caused every conflict I saw on CI, but
there's more tests that create subdirectories in the current working
directory.

Closes phpGH-11869.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants