Skip to content
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

bootloader: replace stray MAX_PATH with PATH_MAX #5617

Merged
merged 3 commits into from Mar 11, 2021

Conversation

rokm
Copy link
Member

@rokm rokm commented Mar 8, 2021

On Windows, pyi_path_fopen() erroneously uses MAX_PATH (260) instead of PATH_MAX (4096) to convert the filename to wide characters, which causes MultiByteToWideChar() call in pyi_win32_utils_from_utf8() to fail with

MultiByteToWideChar: The data area passed to a system call is too small.

when we try to open a file with a long filename (> 260).

Due to lack of error checking, the resulting wfilename array ends up with random content, and the _wfopen() call either fails
(in read-only mode) or creates a randomly-named file (in write mode).

Fixes #5615.

@rokm
Copy link
Member Author

rokm commented Mar 8, 2021

We also really should go through the codebase and add checks for failed pyi_win32_utils_from_utf8 calls, especially in cases when the result array and its max size are given by the caller. Because if the call fails, the array will have its original content (most likely uninitialized) which will result in inconsistent behavior.

But that's a fight for another day, and another PR...

Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that didn't take long...

@bwoodsend bwoodsend added the merge-on-ci-pass This PR is ready to merge providing CI passes label Mar 8, 2021
@bwoodsend
Copy link
Member

I guess actually before I get too overexcited could you build a wheel containing rebuilt bootloaders and ask @hackenjoe to verify it? Or we could add a very long path test?

@rokm
Copy link
Member Author

rokm commented Mar 8, 2021

I'll add a long filename test. I'm beginning to suspect that #5615 has an unrelated underlying issue that has uncovered this path-length related bug...

Test whether filenames with paths longer than 260 (MAX_PATH on Windows)
are properly extracted from onefile build's CArchive.
@rokm rokm force-pushed the bootloader-carchive-longfilenames branch from d7e60f3 to 305789a Compare March 8, 2021 16:36
@rokm
Copy link
Member Author

rokm commented Mar 8, 2021

Alright, here's the revised version. The added test_onefile_longpath should fail on Windows before bootloader is rebuilt. I've also decoupled the commit from the issue (i.e., removed the reference from the commit message and tied the news fragment to PR number instead of issue number).

On Windows, `pyi_path_fopen()` erroneously uses `MAX_PATH` (260)
instead of `PATH_MAX` (4096) to convert the filename to wide
characters, which causes `MultiByteToWideChar()` call in
`pyi_win32_utils_from_utf8()` to fail with
```
MultiByteToWideChar: The data area passed to a system call is too
small.
```
when we try to open a file with a long filename (> 260).

Due to lack of error checking, the resulting `wfilename` array ends
up with random content, and the `_wfopen()` call either fails
(in read-only mode) or creates a randomly-named file (in write
mode).
@rokm rokm force-pushed the bootloader-carchive-longfilenames branch from 305789a to fc294f4 Compare March 8, 2021 18:03
@rokm
Copy link
Member Author

rokm commented Mar 9, 2021

The added test_onefile_longpath should fail on Windows before bootloader is rebuilt.

I guess it's comforting to know that the test also fails after bootloader is rebuilt if the test system doesn't have long names enabled...

It also explains the fopen: No such file or directory message from #5615 that I haven't been able to reproduce.

@rokm rokm force-pushed the bootloader-carchive-longfilenames branch from 8db4768 to dc4673e Compare March 10, 2021 12:06
@bwoodsend bwoodsend merged commit 81b0c1b into pyinstaller:develop Mar 11, 2021
@rokm rokm deleted the bootloader-carchive-longfilenames branch March 22, 2021 19:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge-on-ci-pass This PR is ready to merge providing CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--onefile does not work properly
4 participants