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

ftruncate has unreachable error values that can be reached #22960

Open
Daimanta opened this issue Feb 20, 2025 · 6 comments · May be fixed by #23260
Open

ftruncate has unreachable error values that can be reached #22960

Daimanta opened this issue Feb 20, 2025 · 6 comments · May be fixed by #23260
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@Daimanta
Copy link

Zig Version

0.13.0

Steps to Reproduce and Observed Behavior

On Linux:
ftruncate(<my_valid_handle>, <max u64>)

This triggers the .INVAL error has which triggers an unreachable

Expected Behavior

From the manual page on man7.org

`The ftruncate() function shall fail if:

   EINTR  A signal was caught during execution.

   EINVAL The length argument was less than 0.

   EFBIG or EINVAL
          The length argument was greater than the maximum file size.

   EFBIG  The file is a regular file and length is greater than the
          offset maximum established in the open file description
          associated with fildes.

   EIO    An I/O error occurred while reading from or writing to a
          file system.

   EBADF or EINVAL
          The fildes argument is not a file descriptor open for
          writing.

`

As shown, there are three possible cases where an EINVAL is returned. The zig type system prevents negative length(passed type is u64), so we can ignore that. As the ftruncate function indicates // Handle not open for writing at .INVAL I can assume that is not possible. If that is the case, the only way to trigger EINVAL is by a length exceeding maximum file size. Therefore .INVAL should do the following: return error.FileTooBig, mirroring .FBIG behavior.

@Daimanta Daimanta added the bug Observed behavior contradicts documented or intended behavior label Feb 20, 2025
@alexrp alexrp added standard library This issue involves writing Zig code for the standard library. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. labels Feb 20, 2025
@alexrp
Copy link
Member

alexrp commented Feb 20, 2025

In principle you're supposed to be able to know the maximum file size by way of pathconf("/path/to/file", FILESIZEBITS). However, a mount change could obviously result in that value changing between the calls to pathconf and ftruncate, making such a check unreliable, so I think it's indeed fair to consider this an expected error.

@alexrp alexrp added this to the unplanned milestone Feb 20, 2025
@lsculv
Copy link
Contributor

lsculv commented Feb 22, 2025

#20524 is a related issue regarding "unexpected" error returns from POSIX functions.

@elijahimmer
Copy link
Contributor

elijahimmer commented Mar 12, 2025

I think this is an issue of negative length, although in zig this is a u64, the ftruncate function in glibc is defined as

int ftruncate(int fildes, off_t length)

(man 3p ftruncate)
where off_t is used for describing file sizes. It is a signed integer type. (man 3type off_t)
So the max u64 1111...1111 would actually be the value -1, and thus be invalid.

So I would think the zig function should take something like a u63 so that it does not overflow to a negative number when the number is too large, and thus it won't be invalid anymore.

rootbeer added a commit to rootbeer/zig that referenced this issue Mar 15, 2025
Add a test for std.fs.File's `setEndPos` (which is a simple wrapper around
`std.posix.ftruncate`) to exercise some success and failure paths.

Add errno handling to Windows path to map INVALID_PARAMETER to FileTooBig.

Fixes ziglang#22960
@rootbeer
Copy link
Contributor

@elijahimmer is correct, the EINVAL is from passing a "negative" length (really an unsigned value that gets @bitCast to a negative signed value). I wrote some test cases to exercise ftruncate. Currently for "positive" outlandish values (like 1PB), Linux systems return EFBIG which gets mapped to Zig's error.FileTooBig. On Windows both cases (outlandish and negative values) result in an INVALID_PARAMETER error, which currently falls into the windows.unexpectedStatus case. (I only tested Linux, Windows, Wasi and Wine, all on 64-bit machines).

Annoyingly, Linux will return EINVAL for several distinct cases: bad file descriptor, non-writable file descriptor, non-resizable file descriptor, and negative length.

Writing unit tests that exercise "unreachable" behavior is impossible (one cannot implement testing.expectUnreachable(...)). This seems like a short-coming of the current, aggressive "make it unreachable" error handling approach.

The length argument to ftruncate is a u64, the implementation (both Posix and Windows) bit-casts that into the signed value. E.g., errno(ftruncate_sym(fd, @bitCast(length))).

Oddly, on Wine, the NtSetFileInformation call fails with 0xc0000040 (STATUS_SECTION_TOO_BIG) which must be some Wine internal implementation detail leaking through.

The "fix" here to me isn't obvious. I see a bunch of options:

  • Leave this as-is, hitting unreachable for "negative" values (i.e., std.math.maxInt(u64))
  • @elijahimmer`s suggestion to make the length parameter a u63. Then values that might be interpretable as negative by the implementation would be caught by the compiler because they'd exceed the u63. However, I suspect this would be annoying as lots of u64-ish types would all of a sudden be incompatible. For example, then std.posix.lseek_CUR_get() should also return a u63? This would be more consistent with mapping EINVAL to unreachable (as the type system works to prevent that case).
  • Map all the EINVAL errnos to FileTooBig. This will be misleading for some cases on Linux (e.g., a non-resizable file descriptor, etc) where EINVAL can sometimes mean something else.
  • Stop "upgrading" errnos or making them unreachable, and always return errors.InvalidArgument for EINVAL.

I have a change that adds some unit tests and fixes the Windows error path: #23260. It otherwise leaves the behavior unchanged. Feel free to comment here or there if you'd like to make a case for a different change. (I'm not a Zig core team member, so I don't have any particular standing to make rulings about how to address this.)

@elijahimmer
Copy link
Contributor

Another option is to just check for size values greater than std.math.maxInt(u63) and return error.FileTooBig so the other invalid argument options still work. Although you would need this anytime you are casting a u64 into a i64.

Although I do think having the type system help avoid theses edge cases could be good, it would be a very large change to the posix layer that I would imagine needs a core team member to approve of.

@rootbeer
Copy link
Contributor

Another option is to just check for size values greater than std.math.maxInt(u63) and return error.FileTooBig so the other invalid argument options still work. Although you would need this anytime you are casting a u64 into a i64.

I like this idea! It keeps the length a u64. And I can implement it by just checking if the result of the @bitCast() to i64 has gone negative. Its not strictly POSIX, but neither is the existing unsigned length parameter. The "unreachable" error is gone this way too, and all the arguments of setEndPos are testable. I'll push a new version that does this shortly. Thanks!

rootbeer added a commit to rootbeer/zig that referenced this issue Mar 15, 2025
Add a test for std.fs.File's `setEndPos` (which is a simple wrapper around
`std.posix.ftruncate`) to exercise some success and failure paths.

Explicitly check that the `ftruncate` length isn't negative when
interpreted as a signed value.  This avoids having to decode overloaded
`EINVAL` errors.

Add errno handling to Windows path to map INVALID_PARAMETER to FileTooBig.

Fixes ziglang#22960
rootbeer added a commit to rootbeer/zig that referenced this issue Mar 16, 2025
Add a test for std.fs.File's `setEndPos` (which is a simple wrapper around
`std.posix.ftruncate`) to exercise some success and failure paths.

Explicitly check that the `ftruncate` length isn't negative when
interpreted as a signed value.  This avoids having to decode overloaded
`EINVAL` errors.

Add errno handling to Windows path to map INVALID_PARAMETER to FileTooBig.

Fixes ziglang#22960
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants