Regression in fs.readdirSync on AIX introduced in 18.17.0

[lux01]

Version

v18.17.0

Platform

AIX huritmdemo 3 7 00F70A434C00

Subsystem

fs

What steps will reproduce the bug?

1. Create a directory
2. Create a file under that directory whose name is exactly the same as the directory name
3. Create a second file under the directory whose name is not the same
4. Run the following script passing in the path to the folder

const fs = require('node:fs')
const path = require('node:path')

for (const entry of fs.readdirSync(process.argv[2], { withFileTypes: true  })) {
  console.log('Found entry', path.join(process.argv[2], entry.name))
  console.log('Is file?', entry.isFile())
  console.log('Is directory?', entry.isDirectory())
  console.log('')
}

How often does it reproduce? Is there a required condition?

This reproduce is stable and does not depend on the precise name of the file/folder.

What is the expected behavior? Why is that the expected behavior?

The expected behaviour is that the file with the same name as the folder is correctly reported as a file

-bash-5.1$ ls -ld SameNameForFileAndFolder
drwx------    2 will     staff           256 05 Sep 11:10 SameNameForFileAndFolder
-bash-5.1$ ls -l SameNameForFileAndFolder/
total 0
-rw-------    1 will     staff             0 05 Sep 11:10 AnotherEntry
-rw-------    1 will     staff             0 05 Sep 11:09 SameNameForFileAndFolder
-bash-5.1$ ./node-v18.16.1-aix-ppc64/bin/node recreate.js SameNameForFileAndFolder
Found entry SameNameForFileAndFolder/AnotherEntry
Is file? true
Is directory? false

Found entry SameNameForFileAndFolder/SameNameForFileAndFolder
Is file? true
Is directory? false

What do you see instead?

The actual behaviour is that the file with the same name as the folder is incorrectly reported to be a directory

-bash-5.1$ ls -ld SameNameForFileAndFolder
drwx------    2 will     staff           256 05 Sep 11:10 SameNameForFileAndFolder
-bash-5.1$ ls -l SameNameForFileAndFolder/
total 0
-rw-------    1 will     staff             0 05 Sep 11:10 AnotherEntry
-rw-------    1 will     staff             0 05 Sep 11:09 SameNameForFileAndFolder
-bash-5.1$ ./node-v18.17.1-aix-ppc64/bin/node recreate.js SameNameForFileAndFolder
Found entry SameNameForFileAndFolder/AnotherEntry
Is file? true
Is directory? false

Found entry SameNameForFileAndFolder/SameNameForFileAndFolder
Is file? false
Is directory? true

Additional information

No response

[lux01]

Looking at the changelog for 18.17.0 I would expect the root cause to be #41439 but my git bisect is still running...

[lux01]

git bisect confirms that #41439 introduced the issue, specifically commit 7273ef5.

[lux01]

If I revert this specific part of the commit

diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
index 23865845ba..7b88ae5574 100644
--- a/lib/internal/fs/utils.js
+++ b/lib/internal/fs/utils.js
@@ -232,7 +231,7 @@ function join(path, name) {
   }

   if (typeof path === 'string' && typeof name === 'string') {
-    return pathModule.basename(path) === name ? path : pathModule.join(path, name);
+    return pathModule.join(path, name);
   }

Then my issue seems to go away. @Ethan-Arrowood do you recall why this particular change was necessary? The Uint8Array branch of the function unconditionally joins the paths and does not do this conditional checking of the basename.

[lux01]

Well attempting to run the tests shows that there's probably a pretty good reason that this was introduced! 🙃

node:fs:1668
  handleErrorFromBinding(ctx);
  ^

Error: ENOTDIR: not a directory, lstat '/home/will/node/test/addons/.docbuildstamp/.docbuildstamp'
    at Object.lstatSync (node:fs:1668:3)
    at getDirent (node:internal/fs/utils:313:32)
    at Dir.processReadResult (node:internal/fs/dir:154:9)
    at req.oncomplete (node:internal/fs/dir:131:14) {
  errno: -20,
  syscall: 'lstat',
  code: 'ENOTDIR',
  path: '/home/will/node/test/addons/.docbuildstamp/.docbuildstamp'
}

Node.js v18.17.0
make[1]: *** [Makefile:413: test/addons/.buildstamp] Error 1
make: *** [Makefile:330: test] Error 2
-bash-5.1$

[Ethan-Arrowood]

I introduced that so that recursive readdir would work. I think I remember AIX being a trouble platform to get it to play well. Look and see what's being returned from lower level calls (such as the bindings). There's a good chance AIX is leaving off valuable info about a file and so the code is making a best guess (and getting it wrong)

[lux01]

I've managed to recreate this issue on macOS/arm64 by forcing the ReadDir and ScanDir native bindings to always return a file type of UV_DIRENT_UNKNOWN, to mimic what happens on AIX. This triggers an extra branch in getDirent() where we need to lstat the file.

My hunch in #49499 (comment) on the suspicious behaviour in join being the root cause was correct, but there was knock on issues that needed to be addressed. Firstly I had to back-port 27cadf5 to the v18.x branch, and then I had to fix up readSyncRecursive and processReadResult in internal/fs/dir.js as they were assuming that the path member of a dirent is the path to the file itself rather than the path to the folder containing the file.

PR #49603 opened with the proposed fix.