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

Fix get_file_size behavior inconsistency for folders #561

Merged
merged 2 commits into from
Apr 21, 2023
Merged

Conversation

zeux
Copy link
Owner

@zeux zeux commented Apr 15, 2023

Different OSes have different behavior when trying to fopen/fseek/ftell a folder. On Linux, some systems return 0 size, some systems return an error, and some systems return LONG_MAX. LONG_MAX is particularly problematic because that causes spurious OOMs under address sanitizer.

Using fstat manually cleans this up, however it introduces a new dependency on platform specific headers that we didn't have before, and also has unclear behavior on 64-bit systems wrt 32-bit sizes which will need to be tested further as I'm not certain if the behavior needs to be special-cased only for MSVC/MinGW, which are currently not handled by this path (unless MinGW defines unix...)

Fixes #560.

Different OSes have different behavior when trying to fopen/fseek/ftell
a folder. On Linux, some systems return 0 size, some systems return an
error, and some systems return LONG_MAX. LONG_MAX is particularly
problematic because that causes spurious OOMs under address sanitizer.

Using fstat manually cleans this up, however it introduces a new
dependency on platform specific headers that we didn't have before, and
also has unclear behavior on 64-bit systems wrt 32-bit sizes which will
need to be tested further as I'm not certain if the behavior needs to be
special-cased only for MSVC/MinGW, which are currently not handled by
this path (unless MinGW defines __unix__...)
@zeux
Copy link
Owner Author

zeux commented Apr 21, 2023

The same approach works on MSVC but is a little bit higher overhead compared to fseek/ftell we use right now, so MSVC will have to stick with fseek/ftell (and it requires special handling for large offsets anyhow, as fstat in MSVC also uses 32-bit sizes).

@zeux zeux merged commit f4b8946 into master Apr 21, 2023
@zeux zeux deleted the file-size branch April 21, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OOM when load_file for special folder
1 participant