Skip to content

Windows: fix error condition when File.open fails#13235

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
HertzDevil:bug/windows-file-not-found-errno
Mar 28, 2023
Merged

Windows: fix error condition when File.open fails#13235
straight-shoota merged 1 commit intocrystal-lang:masterfrom
HertzDevil:bug/windows-file-not-found-errno

Conversation

@HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Mar 27, 2023

#13178 changed how files are opened on Windows, but it also inadvertently made it possible to create instances of File with invalid descriptors. This makes things like File.read("foo"), where foo is a non-existent file, crash with STATUS_STACK_BUFFER_OVERRUN = 0xC0000409_u32. The exact failure is due to a Windows C runtime invalid parameter handler being invoked:

lib LibC
  alias InvalidParameterHandler = LPWSTR, LPWSTR, LPWSTR, UInt, UInt64 ->
  fun _set_invalid_parameter_handler(pNew : InvalidParameterHandler) : InvalidParameterHandler
end

LibC._set_invalid_parameter_handler(->(expression, function, file, line, _reserved) do
  Crystal::System.print_error "CRT invalid parameter handler invoked!\n"
  caller.each do |frame|
    Crystal::System.print_error "  from %s\n", frame
  end
  LibC.exit(1)
end)

File.read("foo")
CRT invalid parameter handler invoked!
  from usr\test.cr:8 in '->'
  from minkernel\crts\ucrt\src\appcrt\misc\invalid_parameter.cpp:114 in '_invalid_parameter_internal'
  from minkernel\crts\ucrt\src\appcrt\misc\invalid_parameter.cpp:125 in '_invalid_parameter'
  from minkernel\crts\ucrt\src\appcrt\misc\invalid_parameter.cpp:131 in '_invalid_parameter_noinfo'
  from minkernel\crts\ucrt\src\appcrt\lowio\osfinfo.cpp:264 in '_get_osfhandle'
  from src\crystal\system\win32\file_descriptor.cr:57 in 'windows_handle'
  from src\crystal\system\win32\file_descriptor.cr:84 in 'system_info'
  from src\io\file_descriptor.cr:90 in 'info'
  from src\file.cr:846 in 'size'
  from src\file.cr:694 in 'read'
  from src\file.cr:685 in 'read'
  from usr\test.cr:63 in '__crystal_main'
  from src\crystal\main.cr:115 in 'main_user_code'
  from src\crystal\main.cr:101 in 'main'
  from src\crystal\main.cr:127 in 'main'
  from src\crystal\system\win32\wmain.cr:37 in 'wmain'
  from D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288 in '__scrt_common_main_seh'
  from C:\WINDOWS\System32\KERNEL32.DLL +75453 in 'BaseThreadInitThunk'
  from C:\WINDOWS\SYSTEM32\ntdll.dll +371192 in 'RtlUserThreadStart'

In absence of a user-defined handler, the default action invokes __fastfail, producing the above status code. It does not mean there is a stack overflow.

The culprit is:

handle = LibC.CreateFileW(
System.to_wstr(filename),
access,
LibC::DEFAULT_SHARE_MODE, # UNIX semantics
nil,
disposition,
attributes,
LibC::HANDLE.null
)
if handle == LibC::INVALID_HANDLE_VALUE
# Map ERROR_FILE_EXISTS to Errno::EEXIST to avoid changing semantics of other systems
return {-1, WinError.value.error_file_exists? ? Errno::EEXIST : Errno.value}
end

If the file is not found, WinError.value is ERROR_FILE_NOT_FOUND, but Errno is untouched; thus the method returns {-1, Errno::NONE}, obscuring the error. This PR ensures all WinError values, not just ERROR_FILE_EXISTS, are mapped to Errno values.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:files kind:regression Something that used to correctly work but no longer works labels Mar 27, 2023
@straight-shoota straight-shoota added this to the 1.8.0 milestone Mar 27, 2023
@straight-shoota straight-shoota merged commit e2f831e into crystal-lang:master Mar 28, 2023
@HertzDevil HertzDevil deleted the bug/windows-file-not-found-errno branch March 28, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants