Xxhash: xxhsum(v0.8.0): Directory checking doesn't work as intended

[t-mat]

Migrated from #109

https://github.com/Cyan4973/xxHash/issues/109#issuecomment-801571105

$ xxhsum -H128 *.*
xxhsum: $RECYCLE.BIN: Is a directory
94796d676519ea4c53e10e249f0ef219  2017-03-18 суббота.md
f91167e2b1937e78a6931748d1df4df3  idea-icon.png
xxhsum: Sync: Is a directory
xxhsum: System Volume Information: Is a directory
...
Error: Could not open 'Загрузки': Permission denied.

https://github.com/Cyan4973/xxHash/issues/109#issuecomment-802797854

$ mkdir Birds Vögel
$ xxhsum -H128 *.*
...
xxhsum: Birds: Is a directory
Error: Could not open 'Vögel': Permission denied.
                        ^------------------------------------------- the crux, perhaps?

[t-mat]

@sergeevabc, thanks for the info. Since I think this issue is not related to recursive directory traversal and wildcard expansion, I created new issue. If you don't think so, please let me know.

[t-mat]

@sergeevabc, could you provide more detail about your environment? I've tried WSL (Ubuntu 20) and MinGW64 (MSYS2) but each system doesn't show permission error.

If you can also do the following procedure, it helps us to resolve the issue. I inserted result of my MinGW64 for example.

$ uname -srm
MINGW64_NT-10.0-19042 3.1.7-340.x86_64 x86_64

$ cd
$ git clone https://github.com/Cyan4973/xxHash.git
$ cd xxHash
$ make
$ ./xxhsum.exe --version
xxhsum.exe 0.8.0 by Yann Collet
compiled as 64-bit x86_64 + SSE2 little endian with GCC 10.2.0
$ mkdir Birds Загрузки Vögel
$ ./xxhsum.exe Birds Загрузки Vögel
xxhsum: Birds: Is a directory
xxhsum: Загрузки: Is a directory
xxhsum: Vögel: Is a directory

[t-mat]

note:

• The message "Error: Could not open 'Загрузки': Permission denied." is caused by the failure of XSUM_open() in XSUM_hashFile().
• For some reason, it passes above XSUM_isDirectory().
• Determine which main() is actually used. There are 3 pathes.
  
  
  
  • XSUM_WIN32_USE_WMAIN != 0
    
    
    
    
    
    • path#1: wmain()(wchar_t) -> XSUM_wmain()(wchar_t) -> XSUM_main()(UTF-8)
    
    
    
  
  
  • XSUM_WIN32_USE_WMAIN == 0
    
    
    
    
    
    • path#2: main() -- (char, MBCS with codepage) --> (wchar_t) --> XSUM_wmain() -> XSUM_main()(UTF-8)
    
    
    
    • path#3: main() -- (char, MBCS without codepage) --> (char) --> XSUM_main()
    
    
    
  
  
• At MinGW32, xxhsum always uses main()

off-topic

• Since xxhsum don't support special device (socket, etc) other than stdin, perhaps new function XSUM_isFile() (which uses S_ISREG()) is just a bit safer than XSUM_isDirectory().

edit: I've tried MinGW32. It works fine.

$ uname -srm
MINGW32_NT-10.0-19042 3.1.7-340.x86_64 x86_64

$ cd
$ git clone https://github.com/Cyan4973/xxHash.git
$ cd xxHash/
$ git log -1 --pretty=oneline --abbrev-commit
a9054f3 (HEAD -> dev, origin/dev, origin/HEAD) Small improvements (#515)
$ make
$ ./xxhsum.exe --version
xxhsum.exe 0.8.0 by Yann Collet
compiled as 32-bit i386 little endian with GCC 10.2.0

$ mkdir Birds Загрузки Vögel
$ ./xxhsum.exe Birds Загрузки Vögel
xxhsum: Birds: Is a directory
xxhsum: Загрузки: Is a directory
xxhsum: Vögel: Is a directory

[sergeevabc]

@t-mat

$ uname -srm
MSYS_NT-6.1-7601 3.1.7-340.x86_64 x86_64

$ xxhsum --version
xxhsum 0.8.0 by Yann Collet 
compiled as 64-bit x86_64 autoVec little endian with Clang 10.0.0
(https://github.com/msys2/MINGW-packages.git 7211ffb882cc3b7e7583c518aad45a22b278bc81) 

[t-mat]

Thanks. I've successfully reproduced your issue by the following procedure in MinGW64 on Windows 10 system.

$ uname -srm
MINGW64_NT-10.0-19042 3.1.7-340.x86_64 x86_64

$ pacman -S $MINGW_PACKAGE_PREFIX-xxhash

$ cd
$ mkdir test
$ cd test
$ mkdir Birds Загрузки Vögel
$ which xxhsum
/mingw64/bin/xxhsum
$ xxhsum Birds Загрузки Vögel
xxhsum: Birds: Is a directory
Error: Could not open 'Загрузки': Permission denied.
Error: Could not open 'Vögel': Permission denied.

[t-mat]

The issue

• Both 64bit and 32bit xxhsum from msys2 pacman registry causes this issue. (Call it as msys2-xxhsum)
• When msys2-xxhsum processes a directory name which has Unicode character, this issue occurs.
• But when msys2-xxhsum processes a file name which has Unicode character, it works fine.
• Package build script is msys2/MINGW-packages/mingw-w64-xxhash

Verify the issue

On MinGW64, I cmake the xxhsum from github repo, it works fine.

$ uname -srm
MINGW64_NT-10.0-19042 3.1.7-340.x86_64 x86_64

$ cd
$ git clone https://github.com/Cyan4973/xxHash.git
$ cd xxHash/
$ mkdir build
$ cd build
$ cmake ../cmake_unofficial
$ cmake --build .

$ ./xxhsum.exe --version
xxhsum.exe 0.8.0 by Yann Collet
compiled as 64-bit x86_64 + SSE2 little endian with GCC 10.2.0

$ mkdir Birds Загрузки Vögel

$ ./xxhsum Birds Загрузки Vögel
xxhsum: Birds: Is a directory
xxhsum: Загрузки: Is a directory
xxhsum: Vögel: Is a directory

But msys2-xxhsum doesn't work.

$ pacman -S $MINGW_PACKAGE_PREFIX-xxhash
$ which xxhsum
/mingw64/bin/xxhsum

$ /mingw64/bin/xxhsum --version
xxhsum.exe 0.8.0 by Yann Collet
compiled as 64-bit x86_64 + SSE2 little endian with GCC 10.2.0

$ /mingw64/bin/xxhsum Birds Загрузки Vögel
xxhsum: Birds: Is a directory
Error: Could not open 'Загрузки': Permission denied.
Error: Could not open 'Vögel': Permission denied.

Using definitive Unicode name 🐈🐈🐈 to check it's Unicode issue or not.

$ mkdir 🐈🐈🐈
$ ./xxhsum 🐈🐈🐈
xxhsum: 🐈🐈🐈: Is a directory
$ /mingw64/bin/xxhsum 🐈🐈🐈
Error: Could not open '🐈🐈🐈': Permission denied.

Interestingly, msys2-xxhsum works fine with Unicode 🐱 as a file name.

$ cp ../README.md 🐱
$ ./xxhsum 🐱
66dd982c4822c73d  🐱
$ /mingw64/bin/xxhsum 🐱
66dd982c4822c73d  🐱

[Cyan4973]

Thanks for the thorough investigation @t-mat !

Could it be a difference in commit number ? like dev branch vs v0.8.0 tag ?

If not, is the behavior difference attributable solely to msys2 build script ?

[t-mat]

Could it be a difference in commit number ? like dev branch vs v0.8.0 tag ?

Good point. And you're right. v0.8.0 has this issue. HEAD of dev doesn't have it.

$ uname -srm
MINGW64_NT-10.0-19042 3.1.7-340.x86_64 x86_64

$ cd
$ git clone --depth 1 --branch v0.8.0 https://github.com/Cyan4973/xxHash.git xxhash-v0.8.0
$ cd xxhash-v0.8.0
$ git log -1 --pretty=oneline --abbrev-commit
94e5f23 (grafted, HEAD, tag: v0.8.0) Merge pull request #441 from Cyan4973/dev

$ make
$ mkdir Birds Загрузки Vögel
$ ./xxhsum Birds Загрузки Vögel
xxhsum: Birds: Is a directory
Error: Could not open 'Загрузки': Permission denied.
Error: Could not open 'Vögel': Permission denied.

Let's find the commit which resolves the issue

$ cd
$ git clone https://github.com/Cyan4973/xxHash.git xxhash-bisect
$ cd xxhash-bisect
$ mkdir Birds Загрузки Vögel

...

$ git reset --hard 7ee691763bf55669217d8b74846e4992a558fe9c
$ git log -1 --pretty=oneline --abbrev-commit && make clean && make && ./xxhsum Birds Загрузки Vögel
7ee6917 (HEAD -> dev) refactor xxh3 presentation in code comments
...
xxhsum: Birds: Is a directory
Error: Could not open 'Загрузки': Permission denied.
Error: Could not open 'Vögel': Permission denied.

$ git reset --hard 1c5402c4f5ca1a1a97b5df794cf5aafe8afc883c
$ git log -1 --pretty=oneline --abbrev-commit && make clean && make && ./xxhsum Birds Загрузки Vögel
1c5402c (HEAD -> dev) xxhsum: Split most OS/wrapper functions
...
xxhsum: Birds: Is a directory
xxhsum: Загрузки: Is a directory
xxhsum: Vögel: Is a directory

So, 7ee6917 has the issue and 1c5402c doesn't. Actually, Unicode version of XSUM_isDirectory() resolved it.

[t-mat]

What should we do?

(1) Release new version.
(2) Ask MINGW-packages team to update their package.
(3) Setup AppVeyor CI. And add Unicode file/directory name test.

I think we should do (1) and (2) asap.
But it's okay not to do (3) for now. When xxhsum will have new functionality which depends file/directory names and structure, we'll need it though.

[sergeevabc]

What should we do? (1), (2), (3)…

Dear @t-mat, have you made a decision after all? Or it’s up to @Cyan4973?

[t-mat]

Hi @sergeevabc, basically it's up to @Cyan4973.

But for now, since there's active branch NO_XXH3 / PR #523 which is not merged yet, it seems we should wait for its resolution.
I think all other active issues/PRs are not major blocker of the next release.

[Cyan4973]

The issue is fixed in current dev branch.
What is being asked here is to cut a release, so that an official v0.8.1 exist which includes this fix.
The scope of v0.8.1 is supposed to be larger than this fix, and reaching the intended scope will require some work.
Alternatively, one could imagine cutting down the feature list of v0.8.1 to reach release status faster.
Even in this case, there is still some work left, as I don't consider the current code base ready for a release.
In all cases, my main problem is finding some time to work on the topic.
I'm not highly optimistic on this last point, as I'm currently overwhelmed by other (unrelated) activities.