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.

Sign up for GitHub

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

[5.10] Android: add better nullability checks for nullability annotations added in NDK 26 (#444) #457

Merged
merged 1 commit into from
May 11, 2024
Merged

[5.10] Android: add better nullability checks for nullability annotations added in NDK 26 (#444) #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions Sources/TSCBasic/FileSystem.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,7 @@ private struct LocalFileSystem: FileSystem {

func readFileContents(_ path: AbsolutePath) throws -> ByteString {
// Open the file.
let fp = fopen(path.pathString, "rb")
if fp == nil {
guard let fp = fopen(path.pathString, "rb") else {
throw FileSystemError(errno: errno, path)
}
defer { fclose(fp) }
Expand Down Expand Up @@ -521,8 +520,7 @@ private struct LocalFileSystem: FileSystem {

func writeFileContents(_ path: AbsolutePath, bytes: ByteString) throws {
// Open the file.
let fp = fopen(path.pathString, "wb")
if fp == nil {
guard let fp = fopen(path.pathString, "wb") else {
throw FileSystemError(errno: errno, path)
}
defer { fclose(fp) }
Expand Down
10 changes: 9 additions & 1 deletion Sources/TSCBasic/Process.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ public final class Process {

/// The current OS does not support the workingDirectory API.
case workingDirectoryNotSupported

/// The stdin could not be opened.
case stdinUnavailable
}

public enum OutputRedirection {
Expand Down Expand Up @@ -677,7 +680,10 @@ public final class Process {
var stdinPipe: [Int32] = [-1, -1]
try open(pipe: &stdinPipe)

let stdinStream = try LocalFileOutputByteStream(filePointer: fdopen(stdinPipe[1], "wb"), closeOnDeinit: true)
guard let fp = fdopen(stdinPipe[1], "wb") else {
throw Process.Error.stdinUnavailable
}
let stdinStream = try LocalFileOutputByteStream(filePointer: fp, closeOnDeinit: true)
Comment on lines -680 to +686
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to not change this on 5.10, ie. force unwrap like in the other cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of adding a guard let here is that it's much safer and actually checks for the optional that the recent version of Bionic passes in. Why go back to a force unwrap then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I kind of agree with keeping the guard-let here too. If it's about accepting risk, then a force-unwrap would be a worse outcome than an error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bar for 5.10 is extremely high at this point. If you'd like this change in, it needs to have zero impact outside of fixing the compilation errors. I'm all for the guard on main (and ideally, these would all be actually checks instead of force unwraps).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this passes that bar: the guard-let's are obviously fine, as they only make the code safer. The two remaining force unwraps: one is of a baseAddress access, which should never fail unless someone passes in a nil to that internal function intentionally, and the other is in a test support function.

This patch was merged into the main branch a month ago and we've had zero problems with it: I think the same will be true on 5.10 and it will keep this toolchain compiling on linux with the newest versions of glibc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mixed up which thing this branch was targeting. Short of finding something like a security bug or memory issue like that, the changes can't have any impact on macOS at all. Even changing when something throws or aborts is more than what is acceptable for the 5.10 branch at this point. Sorry about that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this passes that bar too, as these were all raw accesses on macOS before, so if they were going to fail, they would fail regardless. All we're changing on macOS is the way that fails, and I don't think that qualifies as "Even changing when something throws or aborts" as technically these all fail at exactly the same point on macOS after this patch, they just give better errors and fix compilation with glibc and bionic.


// Dupe the read portion of the remote to 0.
posix_spawn_file_actions_adddup2(&fileActions, stdinPipe[0], 0)
Expand Down Expand Up @@ -1258,6 +1264,8 @@ extension Process.Error: CustomStringConvertible {
return "could not find executable for '\(program)'"
case .workingDirectoryNotSupported:
return "workingDirectory is not supported in this platform"
case .stdinUnavailable:
return "could not open stdin on this platform"
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/TSCBasic/WritableByteStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ public final class LocalFileOutputByteStream: FileOutputByteStream {
override final func writeImpl(_ bytes: ArraySlice<UInt8>) {
bytes.withUnsafeBytes { bytesPtr in
while true {
let n = fwrite(bytesPtr.baseAddress, 1, bytesPtr.count, filePointer)
let n = fwrite(bytesPtr.baseAddress!, 1, bytesPtr.count, filePointer)
if n < 0 {
if errno == EINTR { continue }
errorDetected(code: errno)
Expand Down
2 changes: 1 addition & 1 deletion Sources/TSCTestSupport/PseudoTerminal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public final class PseudoTerminal {
if openpty(&primary, &secondary, nil, nil, nil) != 0 {
return nil
}
guard let outStream = try? LocalFileOutputByteStream(filePointer: fdopen(secondary, "w"), closeOnDeinit: false) else {
guard let outStream = try? LocalFileOutputByteStream(filePointer: fdopen(secondary, "w")!, closeOnDeinit: false) else {
return nil
}
self.outStream = outStream
Expand Down
2 changes: 1 addition & 1 deletion Tests/TSCBasicTests/PathShimTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class WalkTests : XCTestCase {
var expected: [AbsolutePath] = [
"\(root)/usr",
"\(root)/bin",
"\(root)/xbin"
"\(root)/etc"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the test change for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this case years ago, back when some Android versions had this directory, matching sbin/ below, but it looks like none do now. Updating this test to another random directory gets it to work again, and this is perfectly safe since it's scoped to just Android.

]
#else
let root = ""
Expand Down