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

File#truncate raises File::AccessDeniedError on Windows when file was opened in append mode #14702

Open
lachlan opened this issue Jun 12, 2024 · 3 comments · May be fixed by #14706
Open

File#truncate raises File::AccessDeniedError on Windows when file was opened in append mode #14702

lachlan opened this issue Jun 12, 2024 · 3 comments · May be fixed by #14706
Labels
kind:bug kind:regression Something that used to correctly work but no longer works platform:windows topic:stdlib:files

Comments

@lachlan
Copy link
Contributor

lachlan commented Jun 12, 2024

Opening a file in append mode on Windows and then calling File#truncate on the opened file used to work prior to Crystal version 1.12.0.

Pull request 14316 changed the Windows implementation of File#truncate to use SetEndOfFile, whose docs, if I'm reading it right, say the file handle must be opened in write mode, which would explain why truncate no longer works in append mode:

Parameters
[in] hFile

A handle to the file to be extended or truncated.

The file handle must be created with the GENERIC_WRITE access right. For more information, see File Security and Access Rights.

The following code:

filename = "test.txt"

begin
  file = File.open(filename, "a")
  file.puts "hello"
  file.flush

  puts "before truncate size = #{File.size(filename)}"

  file.truncate

  puts "after truncate size = #{File.size(filename)}"
ensure
  file.try &.close
end

Throws a File::AccessDeniedError exception, rather than truncating the file, on Windows and Crystal 1.12.2:

C:\Temp>crystal -v
Crystal 1.12.2 [04998c0] (2024-05-31)

LLVM: 18.1.1
Default target: x86_64-pc-windows-msvc

C:\Temp>test_truncate
before truncate size = 6
Unhandled exception: Error truncating file: 'test.txt': Access is denied. (File::AccessDeniedError)
  from C:\Users\username\scoop\apps\crystal\current\src\crystal\system\win32\file.cr:461 in 'system_truncate'
  from C:\Users\username\scoop\apps\crystal\current\src\file.cr:928 in 'truncate'
  from C:\Users\username\scoop\apps\crystal\current\src\file.cr:926 in 'truncate'
  from test_truncate.cr:10 in '__crystal_main'
  from C:\Users\username\scoop\apps\crystal\current\src\crystal\main.cr:129 in 'main_user_code'
  from C:\Users\username\scoop\apps\crystal\current\src\crystal\main.cr:115 in 'main'
  from C:\Users\username\scoop\apps\crystal\current\src\crystal\main.cr:141 in 'main'
  from C:\Users\username\scoop\apps\crystal\current\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 +75133 in 'BaseThreadInitThunk'
  from C:\windows\SYSTEM32\ntdll.dll +371272 in 'RtlUserThreadStart
@straight-shoota straight-shoota added topic:stdlib:files kind:regression Something that used to correctly work but no longer works labels Jun 12, 2024
@straight-shoota
Copy link
Member

straight-shoota commented Jun 12, 2024

This is odd. The handle should be created with GENERIC_WRITE access right from File.open(filename, "a").

Apparently that's not the case. The actual access mode value used is 0x120114. This seems wrong.

I'm also confused about both LibC::GENERIC_WRITE and LibC::FILE_GENERIC_WRITE (with different values; neither is in the actual access mode), and similar.

The documentation for CreateFileW mentions GENERIC_WRITE etc. should be used for access rights. But the Crystal implementation (posix_to_open_opts) uses FILE_GENERIC_WRITE etc. 🤔

@straight-shoota
Copy link
Member

https://learn.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants

FILE_APPEND_DATA (4): For a file object, the right to append data to the file. (For local files, write operations will not overwrite existing data if this flag is specified without FILE_WRITE_DATA.)

So FILE_WRITE_DATA is removed in order to prevent overwriting existing data. This is necessary to ensure the same behaviour as POSIX a mode (and makes a lot of sense anyway).
However, that also prevents truncate from working. I suppose it's not wrong to consider that overwriting existing data.
But in POSIX, truncate works with a file descriptor opened in append mode.

We're missing specs for both behaviours, this is how they could look like:

it "does not overwrite existing content in append mode" do
  with_tempfile("append-override.txt") do |filename|
    File.write(filename, "0123456789")

    File.open(filename, "a") do |file|
      file.seek(5)
      file.write "abcd".to_slice
    end

    File.read(filename).should eq "0123456789abcd"
  end
end

it "truncates file opened in append mode (#14702)" do
  with_tempfile("truncate-append.txt") do |path|
    File.write(path, "0123456789")

    File.open(path, "a") do |file|
      file.truncate(4)
    end

    File.read(path).should eq "0123"
  end
end

On Windows, we need FILE_WRITE_DATA unset for the first one to succeed, and set for the second one to succeed.

@HertzDevil
Copy link
Contributor

HertzDevil commented Jun 12, 2024

It seems CRT does not actually use FILE_APPEND_DATA to implement _O_APPEND, as those file descriptors always contain FILE_WRITE_DATA | FILE_APPEND_DATA in their access masks:

@[Link("ntdll")]
lib LibC
  alias NTSTATUS = Long
  alias ACCESS_MASK = DWORD

  enum OBJECT_INFORMATION_CLASS
    ObjectBasicInformation = 0
  end

  struct PUBLIC_OBJECT_BASIC_INFORMATION
    attributes : ULong
    grantedAccess : ACCESS_MASK
    handleCount : ULong
    pointerCount : ULong
    reserved : ULong[10]
  end

  fun NtQueryObject(
    handle : HANDLE, objectInformationClass : OBJECT_INFORMATION_CLASS,
    objectInformation : Void*, objectInformationLength : ULong, returnLength : ULong*
  ) : NTSTATUS
end

# same with or without `LibC::O_APPEND`
fd = LibC._wopen("README.md".to_utf16, LibC::O_WRONLY | LibC::O_APPEND | LibC::O_BINARY)
handle = LibC::HANDLE.new(LibC._get_osfhandle(fd))
pobi = uninitialized LibC::PUBLIC_OBJECT_BASIC_INFORMATION
LibC.NtQueryObject(handle, LibC::OBJECT_INFORMATION_CLASS::ObjectBasicInformation,
  pointerof(pobi), sizeof(typeof(pobi)), out _)
pobi.grantedAccess.to_s(16) # => "120196"

Every write simply seeks to the end of the file first. Wine does the same.

Also #lock probably has the same issue. We might have to keep track of the append flag in Crystal ourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works platform:windows topic:stdlib:files
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

4 participants