unionfs icon indicating copy to clipboard operation
unionfs copied to clipboard

The readdirSync and readdir functions both have a bug when one filesystem has an empty directory

Open williamstein opened this issue 3 years ago • 8 comments

The readdirSync and readdir functions both have a bug when there's 2 filesystems with this.fss[1] having the directory but it is empty and this.fss[0] not having the directory. In that case checking the size doesn't work to know if the directory exists. In particular, the fix at https://github.com/streamich/unionfs/pull/348 is incorrect. This PR fixes both bugs and adds examples to the test suite.

williamstein avatar Aug 19 '22 04:08 williamstein

@magarcia do you have any thoughts on this?

G-Rath avatar Aug 19 '22 05:08 G-Rath

I'm working on getting the cpython test suite to pass (as part of https://github.com/sagemathinc/python-wasm which uses unionfs), and this revealed another bug in unionfs, which I've also fixed here (and added tests).

This test in the python test suite:

import os; os.listdir('test_exceptions.py')

was supposed to raise

NotADirectoryError: [Errno 54] Not a directory: 'test_exceptions.py''

but was instead raising

FileNotFoundError: [Errno 44] No such file or directory: 'test_exceptions.py'

I traced this (through WebAssembly, WASI, etc.) to how exceptions are raised in unionfs. Basically, unionfs assume that if there is any exception doing anything then it is because of ENOENT. However, there are other reasons to get an error, e.g., ENOTDIR is thrown when you try to open a file as a directory (as is the case in the test above). unionfs is just treating that as a file not found situation and looking further. I think that's the wrong behavior -- instead if an error other than ENOENT is raised, then immediately stop searching through the union and throw that exception. At least that addresses the above problem. I implemented this for readdir, readdirSync, and also asyncMethod and syncMethod. I don't know if that's enough to fully do this right.

Anyway, I hope this is helpful. I case this never gets looked at and somebody else hits this, I'm maintaining a temporary fork here with these fixes: https://www.npmjs.com/package/@wapython/unionfs

-- William

williamstein avatar Aug 23 '22 05:08 williamstein

I pushed another commit that limits the scope of this change to only ENOTDIR errors. Otherwise, the latest change would cause other problems.

williamstein avatar Aug 23 '22 16:08 williamstein

And also EEXIST errors, as I found when running the cpython test_gzip.py part of their test suite...

williamstein avatar Aug 23 '22 17:08 williamstein

Just want to let you know I've not forgotten about this, but it looks like a potentially significant change and we're blocked on doing releases due to #741 so I feel its a bit moot to dive into until that is resolved.

I'm going to try and reach out to @streamich on other channels to see if I can get us unblocked for all the *fs packages.

G-Rath avatar Oct 27 '22 04:10 G-Rath

Thanks. I understand, and am fine using my slight fork in the meantime.

williamstein avatar Oct 27 '22 06:10 williamstein

@williamstein we've able to do releases again, if you want to pick this back up :)

G-Rath avatar Jun 02 '23 23:06 G-Rath

A simplified version of this fix, without ENOTDIR handling, was merged: https://github.com/streamich/unionfs/pull/787/files

dy-dx avatar Mar 19 '24 20:03 dy-dx