Android: update pread/pwrite for nullability annotations in NDK 26

[finagolfin]

This is needed because Bionic recently added a bunch of these annotations, specifically marking these two buffers as _Nonnull. I made sure this pull doesn't break anything by testing it on linux x86_64 and with the previous NDK 25c too. I used this patch with others to build the Swift toolchain and this package for my Android CI, finagolfin/swift-android-sdk#122.

[finagolfin]

@glessard, would you review?

[glessard]

@swift-ci please test

[glessard]

I'm afraid we can't do it this way. This pointer comes from a caller-supplied buffer, whose baseAddress could well be nil. For example:

    let fd = FileDescriptor.standardOutput
    let empty = UnsafeRawBufferPointer(start: nil, count: 0)
    let written = try fd.write(toAbsoluteOffset: 0, empty) // crash at runtime with the suggested force-unwrap

We don't have that precise case covered in a test, but we should!

[finagolfin]

OK, I will check it instead.

[glessard]

@swift-ci please test

[finagolfin]

@glessard, I made a different change to this locally, I just need to add the tests you asked for. I will push that here soon and let you know.

[glessard]

No problem! I wasn't sure about the status, and this solution came to mind.
You can go ahead and force-push when you are ready.

[glessard]

After reading the documentation for these functions in POSIX, I wonder if we should simply return 0 whenever presented with an empty buffer. It is not required to detect errors when nbyte is zero for any of read, pread, write or pwrite. This would not be the transparent behaviour, but it would avoid having to materialize a valid address for buf.

[finagolfin]

This is what I had so far, I've just been putting off writing the tests:

diff --git a/Sources/System/Internals/Syscalls.swift b/Sources/System/Internals/Syscalls.swift
index b22d6a3..b734af2 100644
--- a/Sources/System/Internals/Syscalls.swift
+++ b/Sources/System/Internals/Syscalls.swift
@@ -69,7 +69,8 @@ internal func system_pread(
 #if ENABLE_MOCKING
   if mockingEnabled { return _mockInt(fd, buf, nbyte, offset) }
 #endif
-  return pread(fd, buf, nbyte, offset)
+  guard let buffer = buf else { return -1 }
+  return pread(fd, buffer, nbyte, offset)
 }

 // lseek
@@ -99,7 +100,8 @@ internal func system_pwrite(
 #if ENABLE_MOCKING
   if mockingEnabled { return _mockInt(fd, buf, nbyte, offset) }
 #endif
-  return pwrite(fd, buf, nbyte, offset)
+  guard let buffer = buf else { return -1 }
+  return pwrite(fd, buffer, nbyte, offset)
 }

 internal func system_dup(_ fd: Int32) -> Int32 {

[glessard]

Returning -1 is wrong, because it implies an actual error occurred. It is normal to return zero bytes!
In all likelihood if the code checks errno after this, it will contain a stale value from an older call. (The policy is to not modify errno when there is no error, so it tends to have an old value at any one time.)
read/pread man page per posix: https://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html
It may be interesting to see the android-specific version; I don't understand why android's header declared these pointers were declared as non-nullable, for example.

The posix docs state that an implementation doesn't need to check for errors if nbyte is 0, and that made me think that it might be okay to skip the syscall when that happens. This doesn't fit with the goal of the package, though, which is to behave as closely as possible as the underlying syscall.

[finagolfin]

Returning -1 is wrong, because it implies an actual error occurred. It is normal to return zero bytes!

That's not the issue here though: this issue is that the buffer read or written into cannot be null. That's definitely an error.

In all likelihood if the code checks errno after this, it will contain a stale value from an older call.

Yes, checking the errno would not work with my patch, as pread()/pwrite() are never actually called. We could run those anyway, just to set the errno.

I don't understand why android's header declared these pointers were declared as non-nullable, for example.

You can't do anything if the buffer you want to pread/pwrite into is null.

The posix docs state that an implementation doesn't need to check for errors if nbyte is 0, and that made me think that it might be okay to skip the syscall when that happens.

Oh, that's what you were relying on with this change of yours? I wondered why you were swallowing the error altogether. If you want to rely on that clause, I just tried this pull on Android and it works: I'm fine with merging this.

Alternately and after looking into this more, the system_pread()/system_pwrite() wrappers calling these C functions are internal functions only available in this library, each called in only one place, ie _read()/_write(), which pass in a baseAddress that could be null. I could just check buffer.baseAddress in _read()/_write() and make sure it's not null there.

If you are worried that there might be more internal callers of these system_pread()/system_pwrite() methods that need the option to pass in null, your current approach or something different to surface the errno would be required. I'm fine with whatever you want, as long as this compilation error on Android is fixed.

[glessard]

That's not the issue here though: this issue is that the buffer read or written into cannot be null. That's definitely an error.

It would be a programmer error to pass a nil pointer to the call with an expectation that 1 or more bytes be read, but that is not something that will happen here. We are not editing a public function, but an internal one that gets called in a very specific way. That specific way includes throwing an error if the function does not return zero.

Also: according to the man page, passing 0 for nbyte is not an error. read, pread, write and pwrite may actually check for error conditions in that situation however. I don't know what the Linux and/or Android implementations do, but I am tempted to add Android-only code in an #if os(Android) branch, which would materialize a valid pointer to pass to the call. If any other OSs follow in making this parameter non-null, then they can be added to the #if statement.

[finagolfin]

It would be a programmer error to pass a nil pointer to the call with an expectation that 1 or more bytes be read, but that is not something that will happen here. We are not editing a public function, but an internal one that gets called in a very specific way.

Heh, OK, but you are the one who originally raised that nil pointer concern a couple months ago.

I don't know what the Linux and/or Android implementations do, but I am tempted to add Android-only code in an #if os(Android) branch, which would materialize a valid pointer to pass to the call. If any other OSs follow in making this parameter non-null, then they can be added to the #if statement.

Nah, I am fine with your current approach in this pull, which works on Android too, please go ahead and squash and merge. If you prefer something else, just let me know.

[finagolfin]

Ping @glessard, anything holding this up?

[glessard]

I'd like to try a different approach, where we can make the syscall regardless on platforms where the parameter has the typical nullability. I don't love the idea of skipping the syscall on android either, fwiw.

[finagolfin]

I don't love the idea of skipping the syscall on android either, fwiw.

I didn't either, but once you pointed out that "The posix docs state that an implementation doesn't need to check for errors if nbyte is 0, and that made me think that it might be okay to skip the syscall when that happens," I was fine with it. Since we cannot depend on the call to the underlying C function erroring if we pass in that edge case, it's fine to skip that edge case and maybe raise a Swift error ourselves.

[glessard]

@swift-ci please test

[glessard]

@swift-ci please test

[finagolfin]

Doesn't work:

/data/data/com.termux/files/home/swift-system/Sources/System/Internals/Syscalls.swift:78:22: error: cannot convert value of type 'UnsafeMutablePointer<UInt8>' to expected argument type 'UnsafeMutableRawPointer'
    pread(fd, buf ?? $0, nbyte, offset)
                     ^
/data/data/com.termux/files/home/swift-system/Sources/System/Internals/Syscalls.swift:116:23: error: cannot convert value of type 'UnsafeMutablePointer<UInt8>' to expected argument type 'UnsafeRawPointer'
    pwrite(fd, buf ?? $0, nbyte, offset)
                      ^

[glessard]

@swift-ci please test

[finagolfin]

Compiles and all tests pass again on Android now.