Skip to content

Android: add better nullability checks for nullability annotations added in NDK 26 #4850

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

Closed
wants to merge 1 commit into from
Closed

Android: add better nullability checks for nullability annotations added in NDK 26 #4850

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
7 changes: 6 additions & 1 deletion Sources/Foundation/FileHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,15 @@ open class FileHandle : NSObject {
if options.contains(.alwaysMapped) {
// Filesizes are often 64bit even on 32bit systems
let mapSize = min(length, Int(clamping: statbuf.st_size))
#if os(Android)
// Bionic mmap() now returns _Nonnull, so the force unwrap below isn't needed.
Copy link
Member Author

Choose a reason for hiding this comment

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

@parkera, is this the change you had in mind?

let data = mmap(nil, mapSize, PROT_READ, MAP_PRIVATE, _fd, 0)
#else
let data = mmap(nil, mapSize, PROT_READ, MAP_PRIVATE, _fd, 0)!
#endif
// Swift does not currently expose MAP_FAILURE
if data != UnsafeMutableRawPointer(bitPattern: -1) {
return NSData.NSDataReadResult(bytes: data!, length: mapSize) { buffer, length in
return NSData.NSDataReadResult(bytes: data, length: mapSize) { buffer, length in
munmap(buffer, length)
}
}
Expand Down
34 changes: 24 additions & 10 deletions Sources/Foundation/FileManager+POSIX.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,32 +741,38 @@ extension FileManager {
if rmdir(fsRep) == 0 {
return
} else if errno == ENOTEMPTY {
#if os(Android)
let ps = UnsafeMutablePointer<UnsafeMutablePointer<Int8>>.allocate(capacity: 2)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to duplicate this or can we get away with a rebinding of the memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed after your swiftlang/swift#74829, so removed in #5010.

ps.initialize(to: UnsafeMutablePointer(mutating: fsRep))
ps.advanced(by: 1).initialize(to: unsafeBitCast(0, to: UnsafeMutablePointer<Int8>.self))
#else
let ps = UnsafeMutablePointer<UnsafeMutablePointer<Int8>?>.allocate(capacity: 2)
ps.initialize(to: UnsafeMutablePointer(mutating: fsRep))
ps.advanced(by: 1).initialize(to: nil)
#endif
let stream = fts_open(ps, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR | FTS_NOSTAT, nil)
ps.deinitialize(count: 2)
ps.deallocate()

if stream != nil {
if let openStream = stream {
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply if let stream?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, removed in #5010, you can add it in a version of #4889 for 6.0.

defer {
fts_close(stream)
fts_close(openStream)
}

while let current = fts_read(stream)?.pointee {
let itemPath = string(withFileSystemRepresentation: current.fts_path, length: Int(current.fts_pathlen))
while let current = fts_read(openStream)?.pointee, let current_path = current.fts_path {
let itemPath = string(withFileSystemRepresentation: current_path, length: Int(current.fts_pathlen))
guard alreadyConfirmed || shouldRemoveItemAtPath(itemPath, isURL: isURL) else {
continue
}

do {
switch Int32(current.fts_info) {
case FTS_DEFAULT, FTS_F, FTS_NSOK, FTS_SL, FTS_SLNONE:
if unlink(current.fts_path) == -1 {
if unlink(current_path) == -1 {
throw _NSErrorWithErrno(errno, reading: false, path: itemPath)
}
case FTS_DP:
if rmdir(current.fts_path) == -1 {
if rmdir(current_path) == -1 {
throw _NSErrorWithErrno(errno, reading: false, path: itemPath)
}
case FTS_DNR, FTS_ERR, FTS_NS:
Expand Down Expand Up @@ -1085,10 +1091,18 @@ extension FileManager {
do {
guard fm.fileExists(atPath: _url.path) else { throw _NSErrorWithErrno(ENOENT, reading: true, url: url) }
_stream = try FileManager.default._fileSystemRepresentation(withPath: _url.path) { fsRep in
#if os(Android)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, can we do this by converting the type somehow at the call to fts_open instead of around all uses of the type before that point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe there is, I just don't know how to do it. This is simply the best I came up with after seeing these Swift type-casting issues for the first time, and I wanted to ask if there was a better way.

@glbrntt, you recently added similar code to NIO, wdyt?

Copy link

Choose a reason for hiding this comment

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

NIO wraps all of the syscalls and does any platform-specific shenanigans within the wrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, but that will still require adapting these types for nullability annotations: see my recent patch of your new NIO code for Android.

Copy link

Choose a reason for hiding this comment

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

re: your patch, does using an implicitly unwrapped optional work for both cases? e.g. [UnsafeMutablePointer<CInterop.PlatformChar>!]

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 get the following error with Swift 5.9.2 on Android AArch64 if I add that to the type signature of libc_fts_open():

swift-nio/Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift:407:14: error: using '!' is not allowed here; perhaps '?' was intended?
    _ path: [UnsafeMutablePointer<CInterop.PlatformChar>!],
             ^                                          ~
                                                        ?
error: fatalError

Copy link
Member Author

Choose a reason for hiding this comment

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

@glbrntt, is that type what you had in mind, or simply unwrapping the arguments when they are passed in inside ftsOpen()?

Copy link
Member

Choose a reason for hiding this comment

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

I think that I approached it similarly to what was being suggested - rebinding the memory around the call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

These fts_open() changes were no longer needed after swiftlang/swift#74829, so I removed them in #5010.

let ps = UnsafeMutablePointer<UnsafeMutablePointer<Int8>>.allocate(capacity: 2)
#else
let ps = UnsafeMutablePointer<UnsafeMutablePointer<Int8>?>.allocate(capacity: 2)
#endif
defer { ps.deallocate() }
ps.initialize(to: UnsafeMutablePointer(mutating: fsRep))
#if os(Android)
ps.advanced(by: 1).initialize(to: unsafeBitCast(0, to: UnsafeMutablePointer<Int8>.self))
#else
ps.advanced(by: 1).initialize(to: nil)
#endif
return fts_open(ps, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR | FTS_NOSTAT, nil)
}
if _stream == nil {
Expand Down Expand Up @@ -1135,14 +1149,14 @@ extension FileManager {
}

_current = fts_read(stream)
while let current = _current {
let filename = FileManager.default.string(withFileSystemRepresentation: current.pointee.fts_path, length: Int(current.pointee.fts_pathlen))
while let current = _current, let current_path = current.pointee.fts_path {
let filename = FileManager.default.string(withFileSystemRepresentation: current_path, length: Int(current.pointee.fts_pathlen))

switch Int32(current.pointee.fts_info) {
case FTS_D:
let (showFile, skipDescendants) = match(filename: filename, to: _options, isDir: true)
if skipDescendants {
fts_set(_stream, _current, FTS_SKIP)
fts_set(stream, current, FTS_SKIP)
}
if showFile {
return URL(fileURLWithPath: filename, isDirectory: true)
Expand Down Expand Up @@ -1315,7 +1329,7 @@ extension FileManager {
let finalErrno = originalItemURL.withUnsafeFileSystemRepresentation { (originalFS) -> Int32? in
return newItemURL.withUnsafeFileSystemRepresentation { (newItemFS) -> Int32? in
// This is an atomic operation in many OSes, but is not guaranteed to be atomic by the standard.
if rename(newItemFS, originalFS) == 0 {
if let newFS = newItemFS, let origFS = originalFS, rename(newFS, origFS) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply force unwrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's better, dropped this in #5010, you can do that in a version of #4889 for 6.0.

return nil
} else {
return errno
Expand Down
8 changes: 4 additions & 4 deletions Sources/Foundation/FileManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,13 +568,13 @@ open class FileManager : NSObject {
let attributes = try windowsFileAttributes(atPath: path)
let type = FileAttributeType(attributes: attributes, atPath: path)
#else
if let pwd = getpwuid(s.st_uid), pwd.pointee.pw_name != nil {
let name = String(cString: pwd.pointee.pw_name)
if let pwd = getpwuid(s.st_uid), let pwd_name = pwd.pointee.pw_name {
let name = String(cString: pwd_name)
result[.ownerAccountName] = name
}

if let grd = getgrgid(s.st_gid), grd.pointee.gr_name != nil {
let name = String(cString: grd.pointee.gr_name)
if let grd = getgrgid(s.st_gid), let grd_name = grd.pointee.gr_name {
let name = String(cString: grd_name)
result[.groupOwnerAccountName] = name
}

Expand Down
3 changes: 2 additions & 1 deletion Sources/Foundation/Host.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import WinSDK

// getnameinfo uses size_t for its 4th and 6th arguments.
private func getnameinfo(_ addr: UnsafePointer<sockaddr>?, _ addrlen: socklen_t, _ host: UnsafeMutablePointer<Int8>?, _ hostlen: socklen_t, _ serv: UnsafeMutablePointer<Int8>?, _ servlen: socklen_t, _ flags: Int32) -> Int32 {
return Glibc.getnameinfo(addr, addrlen, host, Int(hostlen), serv, Int(servlen), flags)
guard let saddr = addr else { return -1 }
Copy link
Member

Choose a reason for hiding this comment

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

I think that I prefer my approach of changing the signature of the private func.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about the delay, been meaning to get to this, hopefully this week.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing the signature would work too, dropped this in #5010, you can submit it in a version of #4889 for 6.0.

return Glibc.getnameinfo(saddr, addrlen, host, Int(hostlen), serv, Int(servlen), flags)
}

// getifaddrs and freeifaddrs are not available in Android 6.0 or earlier, so call these functions dynamically.
Expand Down
2 changes: 1 addition & 1 deletion Tests/Foundation/Tests/TestFileHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class TestFileHandle : XCTestCase {
#else
var fds: [Int32] = [-1, -1]
fds.withUnsafeMutableBufferPointer { (pointer) -> Void in
pipe(pointer.baseAddress)
pipe(pointer.baseAddress!)
}

close(fds[1])
Expand Down
2 changes: 1 addition & 1 deletion Tests/Foundation/Tests/TestTimeZone.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ class TestTimeZone: XCTestCase {
var lt = tm()
localtime_r(&t, &lt)
let zoneName = NSTimeZone.system.abbreviation() ?? "Invalid Abbreviation"
let expectedName = String(cString: lt.tm_zone, encoding: .ascii) ?? "Invalid Zone"
let expectedName = String(cString: lt.tm_zone!, encoding: .ascii) ?? "Invalid Zone"
XCTAssertEqual(zoneName, expectedName, "expected name \"\(expectedName)\" is not equal to \"\(zoneName)\"")
}
#endif
Expand Down