Merge pull request #2858 from niw/fix_nsdata_ignores_umask

Create a file with permissions that respects umask

Author: millenomi

Sources/Foundation/NSData.swift

@@ -434,8 +434,7 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
         }
 
         let fm = FileManager.default
-        // The destination file path may not exist so provide a default file permissions of RW user only
-        let permissions = (try? fm._permissionsOfItem(atPath: path)) ?? 0o600
+        let permissions = try? fm._permissionsOfItem(atPath: path)
 
         if writeOptionsMask.contains(.atomic) {
             let (newFD, auxFilePath) = try _NSCreateTemporaryFile(path)
@@ -446,7 +445,9 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
                 // requires that there be no open handles to the file
                 fh.closeFile()
                 try _NSCleanupTemporaryFile(auxFilePath, path)
-                try fm.setAttributes([.posixPermissions: NSNumber(value: permissions)], ofItemAtPath: path)
+                if let permissions = permissions {
+                    try fm.setAttributes([.posixPermissions: NSNumber(value: permissions)], ofItemAtPath: path)
+                }
             } catch {
                 let savedErrno = errno
                 try? fm.removeItem(atPath: auxFilePath)
@@ -458,10 +459,23 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
                 flags |= O_EXCL
             }
 
-            guard let fh = FileHandle(path: path, flags: flags, createMode: permissions) else {
+            // NOTE: Each flag such as `S_IRUSR` may be literal depends on the system.
+            // Without explicity type them as `Int`, type inference will not complete in reasonable time
+            // and the compiler will throw an error.
+#if os(Windows)
+            let createMode = Int(ucrt.S_IREAD) | Int(ucrt.S_IWRITE)
+#elseif canImport(Darwin)
+            let createMode = Int(S_IRUSR) | Int(S_IWUSR) | Int(S_IRGRP) | Int(S_IWGRP) | Int(S_IROTH) | Int(S_IWOTH)
+#else
+            let createMode = Int(Glibc.S_IRUSR) | Int(Glibc.S_IWUSR) | Int(Glibc.S_IRGRP) | Int(Glibc.S_IWGRP) | Int(Glibc.S_IROTH) | Int(Glibc.S_IWOTH)
+#endif
+            guard let fh = FileHandle(path: path, flags: flags, createMode: createMode) else {
                 throw _NSErrorWithErrno(errno, reading: false, path: path)
             }
             try doWrite(fh)
+            if let permissions = permissions {
+                try fm.setAttributes([.posixPermissions: NSNumber(value: permissions)], ofItemAtPath: path)
+            }
         }
     }
 

Sources/Foundation/NSPathUtilities.swift

@@ -760,7 +760,10 @@ internal func _NSCreateTemporaryFile(_ filePath: String) throws -> (Int32, Strin
     let maxLength = Int(PATH_MAX) + 1
     var buf = [Int8](repeating: 0, count: maxLength)
     let _ = template._nsObject.getFileSystemRepresentation(&buf, maxLength: maxLength)
-    let fd = mkstemp(&buf)
+    guard let name = mktemp(&buf) else {
+        throw _NSErrorWithErrno(errno, reading: false, path: filePath)
+    }
+    let fd = open(name, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)
     if fd == -1 {
         throw _NSErrorWithErrno(errno, reading: false, path: filePath)
     }

Tests/Foundation/Tests/TestNSData.swift

@@ -7,6 +7,16 @@
 // See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 
+import CoreFoundation
+
+#if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT
+    #if canImport(SwiftFoundation) && !DEPLOYMENT_RUNTIME_OBJC
+        @testable import SwiftFoundation
+    #else
+        @testable import Foundation
+    #endif
+#endif
+
 class TestNSData: LoopbackServerTest {
 
     class AllOnesImmutableData : NSData {
@@ -223,6 +233,8 @@ class TestNSData: LoopbackServerTest {
             ("test_limitDebugDescription", test_limitDebugDescription),
             ("test_edgeDebugDescription", test_edgeDebugDescription),
             ("test_writeToURLOptions", test_writeToURLOptions),
+            ("test_writeToURLPermissions", test_writeToURLPermissions),
+            ("test_writeToURLPermissionsWithAtomic", test_writeToURLPermissionsWithAtomic),
             ("test_edgeNoCopyDescription", test_edgeNoCopyDescription),
             ("test_initializeWithBase64EncodedDataGetsDecodedData", test_initializeWithBase64EncodedDataGetsDecodedData),
             ("test_initializeWithBase64EncodedDataWithNonBase64CharacterIsNil", test_initializeWithBase64EncodedDataWithNonBase64CharacterIsNil),
@@ -551,6 +563,61 @@ class TestNSData: LoopbackServerTest {
         }
     }
 
+#if !os(Windows)
+    // NOTE: `umask(3)` is process global. Therefore, the behavior is unknown if `withUmask(_:_:)` is used simultaniously.
+    private func withUmask(_ mode: mode_t, _ block: () -> Void) {
+        let original = umask(mode)
+        block()
+        umask(original)
+    }
+#endif
+
+    func test_writeToURLPermissions() {
+#if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT && !os(Windows)
+        withUmask(0) {
+            do {
+                let data = Data()
+                let url = URL(fileURLWithPath: NSTemporaryDirectory() + "meow")
+                try data.write(to: url)
+                let fileManager = FileManager.default
+                let permission = try fileManager._permissionsOfItem(atPath: url.path)
+#if canImport(Darwin)
+                let expected = Int(S_IRUSR) | Int(S_IWUSR) | Int(S_IRGRP) | Int(S_IWGRP) | Int(S_IROTH) | Int(S_IWOTH)
+#else
+                let expected = Int(Glibc.S_IRUSR) | Int(Glibc.S_IWUSR) | Int(Glibc.S_IRGRP) | Int(Glibc.S_IWGRP) | Int(Glibc.S_IROTH) | Int(Glibc.S_IWOTH)
+#endif
+                XCTAssertEqual(permission, expected)
+                try! fileManager.removeItem(atPath: url.path)
+            } catch {
+                XCTFail()
+            }
+        }
+#endif
+    }
+
+    func test_writeToURLPermissionsWithAtomic() {
+#if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT && !os(Windows)
+        withUmask(0) {
+            do {
+                let data = Data()
+                let url = URL(fileURLWithPath: NSTemporaryDirectory() + "meow")
+                try data.write(to: url, options: .atomic)
+                let fileManager = FileManager.default
+                let permission = try fileManager._permissionsOfItem(atPath: url.path)
+#if canImport(Darwin)
+                let expected = Int(S_IRUSR) | Int(S_IWUSR) | Int(S_IRGRP) | Int(S_IWGRP) | Int(S_IROTH) | Int(S_IWOTH)
+#else
+                let expected = Int(Glibc.S_IRUSR) | Int(Glibc.S_IWUSR) | Int(Glibc.S_IRGRP) | Int(Glibc.S_IWGRP) | Int(Glibc.S_IROTH) | Int(Glibc.S_IWOTH)
+#endif
+                XCTAssertEqual(permission, expected)
+                try! fileManager.removeItem(atPath: url.path)
+            } catch {
+                XCTFail()
+            }
+        }
+#endif
+    }
+
     func test_emptyDescription() {
         let expected = "<>"