Fix for NSKeyedArchiver.archiveRootObject api failure on Linux #1464

mamabusi · 2018-03-09T11:11:03Z

Test case:

        let filePath = NSTemporaryDirectory() + "testdir\(NSUUID().uuidString)"
        let result = NSKeyedArchiver.archiveRootObject("Hello", toFile: filePath)
        print(result)

Expected Result: true
Actual result on Linux: false
Actual result on Darwin: true

mamabusi · 2018-03-09T11:16:29Z

Foundation/NSKeyedArchiver.swift

    ///   - path:       The path of the file in which to write the archive.
    /// - Returns:      `true` if the operation was successful, otherwise `false`.
    open class func archiveRootObject(_ rootObject: Any, toFile path: String) -> Bool {
+        var filePath = "/" + path


Path input like "Users/sdsfd/test.txt" is accepted on Darwin and a "/" is prepended to the path. Hence, prepending the path since a double "/" will not impact.

Is this the only normalisation that Darwin does? It seems unlikely to me... I would guess that there is more to it than this. Also, why not only prepend if required - allowing a double // seems a bit hacky.

I don't think prepending with / is valid here. If I try to write to a relative path (say, "./my-archive"), we'd be making it absolute ("/./my-archive"), which is not correct.

Why is the current behavior a failure on Linux? What paths are failing? Note that in Darwin Foundation, we don't modify this path at all; the underlying code for creating the temporary file takes care of things.

mamabusi · 2018-03-09T11:20:12Z

Foundation/NSPathUtilities.swift

 }

 internal func _NSCreateTemporaryFile(_ filePath: String) throws -> (Int32, String) {
-    let template = "." + filePath + ".tmp.XXXXXX"


With the "." in place, the fd returned by system library call mkstemp() is always -1 and the temporary file creation always fails. Also, since the file path sent to _NSCreateTemporaryFile() is absolute, making it relative here does not seems right.

The file path to _NSCreateTemporaryFile should not need to be absolute; we're indeed using it as a template to create a new file, so we should be able to make whatever modifications necessary internally to make it work.

ianpartridge · 2018-03-09T12:06:11Z

@swift-ci please test

spevans · 2018-03-09T19:59:23Z

@swift-ci please test

pushkarnk · 2018-03-12T06:19:24Z

The failure doesn't appear to be related to the change.
07:26:54 /home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux@2/Ninja-ReleaseAssert/swiftpm-linux-x86_64/x86_64-unknown-linux/release/swift-test: error while loading shared libraries: libswiftSwiftOnoneSupport.so: cannot open shared object file: No such file or directory

I'm firing up another test, just in case we have some intermittent issue at hand.

@swift-ci please test

pushkarnk · 2018-03-12T06:19:46Z

@swift-ci please test

parkera · 2018-03-12T22:12:06Z

cc @itaiferber

mamabusi · 2018-03-21T17:23:59Z

@itaiferber Could you please review this PR.

itaiferber · 2018-03-21T18:25:03Z

@mamabusi I must've missed this; looking now.

itaiferber

Thanks for taking the time to put this PR together, but I think this is fixing a problem at the wrong level. Call sites of _NSCreateTemporaryFile should not need to modify the path they pass in to get it to work; instead, we should figure out why the result of

let filePath = NSTemporaryDirectory() + "testdir\(NSUUID().uuidString)"

is not valid to write into when passing that in to _NSCreateTemporaryFile. Presumably there are other usages of _NSCreateTemporaryFile that would be hitting the same problem; the solution would be to fix how _NSCreateTemporaryFile creates a new file, not all of the call sites.

I don't have a Linux box to test on at the moment. What is the result of NSTemporaryDirectory() + "testdir\(NSUUID().uuidString)" on your machine? And what would the resulting path from _NSCreateTemporaryFile be? If it indeed is not a writable path, we need to fix that.

itaiferber · 2018-03-21T18:37:21Z

Foundation/NSKeyedArchiver.swift

    ///   - path:       The path of the file in which to write the archive.
    /// - Returns:      `true` if the operation was successful, otherwise `false`.
    open class func archiveRootObject(_ rootObject: Any, toFile path: String) -> Bool {
+        var filePath = "/" + path


I don't think prepending with / is valid here. If I try to write to a relative path (say, "./my-archive"), we'd be making it absolute ("/./my-archive"), which is not correct.

Why is the current behavior a failure on Linux? What paths are failing? Note that in Darwin Foundation, we don't modify this path at all; the underlying code for creating the temporary file takes care of things.

itaiferber · 2018-03-21T18:41:19Z

Foundation/NSPathUtilities.swift

 }

 internal func _NSCreateTemporaryFile(_ filePath: String) throws -> (Int32, String) {
-    let template = "." + filePath + ".tmp.XXXXXX"


The file path to _NSCreateTemporaryFile should not need to be absolute; we're indeed using it as a template to create a new file, so we should be able to make whatever modifications necessary internally to make it work.

mamabusi · 2018-03-26T08:36:47Z

@itaiferber Thanks for taking time and looking into this PR.
Below are more details in response to the review:

internal func _NSCreateTemporaryFile(_ filePath: String) throws -> (Int32, String)

The above internal function is called only from the function NSKeyedArchiver.archiveRootObject in the whole of SCLF. There are no other existing call sites to _NSCreateTemporaryFile

Here’s the result of NSTemporaryDirectory() + "testdir\(NSUUID().uuidString)" on Linux box:

/tmp/testdir60CABFCC-3F02-4EC7-90C0-42F4E1C8F55F

The above result looks fine and there are no issues there.

When I pass the resulting filePath /tmp/testdir60CABFCC-3F02-4EC7-90C0-42F4E1C8F55F to
NSKeyedArchiver.archiveRootObject("Hello", toFile: filePath) , it fails to write to the file.

What is happening internally is:
When _NSCreateTemporaryFile(“/tmp/testdir60CABFCC-3F02-4EC7-90C0-42F4E1C8F55F”) is called, it prepends "." to the path.

Now the path looks like this:
./tmp/testdir60CABFCC-3F02-4EC7-90C0-42F4E1C8F55F

and then when the system call “mkstemp(&buf)” is made, the fd returned is always “-1” causing a failure in temporary file creation.

Is there a reason why the function _NSCreateTemporaryFile() has to convert the given path to a relative path from the current directory?

itaiferber · 2018-03-26T18:16:36Z

@mamabusi I agree in that I don't think that it should make the path relative; it doesn't do that on the Darwin side AFAICT. I think it's worth trying to figure out from the git history why (if for any reason) it was there in the first place, especially if NSKeyedArchiver is currently the only remaining place using it. It seems that the solution here could be as simple as just removing the leading "." + from _NSCreateTemporaryFile, without modifying NSKeyedArchiver itself.

mamabusi · 2018-03-27T07:51:23Z

@itaiferber
Here’s what I got from the history:
The internal function _NSCreateTemporaryFile() was introduced as part of the commit b4d207e#diff-c4535e3015bf3d52374e84bcaf592d95 that maps to the SR: https://bugs.swift.org/browse/SR-432 which tells us that this internal api was created to help the implementation of the NSKeyedArchiver apis. The implementation has not changed since then (not sure about the reason for "." prefixed to the filepath). Also, there is no test-case in the Foundation test-suite that covers NSKeyedArchiver.archiveRootObject() api which uses the internal api _NSCreateTemporaryFile() and hence this bug is not caught.

On the other aspect, the reason I decided to prefix "/" to the filePath in NSKeyedArchiver was because the below test-cases pass on Darwin platform:

let result = NSKeyedArchiver.archiveRootObject("Hello", toFile: "tmp/test.txt")
let result = NSKeyedArchiver.archiveRootObject("Hello", toFile: "Users/mamatha/Desktop/../Documents/test.txt")

Without adding the prefix of "/" , the above test-cases fail on Linux.

This test-case:
let result = NSKeyedArchiver.archiveRootObject("Hello", toFile: "./../Documents/dd")

fails on both Darwin and Linux. The behaviour is the same even if Linux prefixes "/", i.e. we are not deviating from any of the behaviour of Darwin for all the above test-cases.

If there is something more I need to consider, please let me know.

itaiferber · 2018-03-27T16:06:33Z

@mamabusi Thanks for taking the time to uncover the history of this! It sounds like removing the preceding "." + from _NSCreateTemporaryFile makes sense, so I'm definitely on board with that.

Regarding prepending /: I'm not sure how those tests pass on Darwin unless the current working directory is already /. When you create a path that is not rooted in / to begin with, the path you have is relative. Archiving to that path, then, depends on the current working directory. Consider the following:

import Foundation

let path = "Users/itai/Desktop/../Documents/test.txt"
let url = URL(fileURLWithPath: path)
print(url.path)

if NSKeyedArchiver.archiveRootObject("Hello, world!", toFile: path) {
    print("Write succeeded")
} else {
    print("Write failed")
}

When I run the above from my home directory (cd ~; swift Test.swift), the output is

/Users/itai/Users/itai/Documents/test.txt
Write failed

When I run it from / (cd /; swift ~/Test.swift), the output is

/Users/itai/Documents/test.txt
Write succeeded

In fact, that can only succeed when run when the current working directory is /; the path given to NSKeyedArchiver is turned into a URL for writing exactly as above (NSURL *fileURL = [NSURL fileURLWithPath:tempFilePath isDirectory:NO];). That path is then used to create a stream in the same way, and the stream resolves the URL as all relative URLs are resolved. Those tests should pass only if the current working directory is /.

Given that, I don't think that making the path absolute by forcibly rooting it against / is correct; I think the current behavior is correct. If the write fails, the path in the test should probably be updated.

mamabusi · 2018-03-28T06:21:04Z

@itaiferber Thank you! I understand and agree with you on "not making the path absolute". Have made the necessary changes on the PR.

alblue · 2018-03-28T12:50:50Z

@swift-ci please test

itaiferber · 2018-03-28T16:42:28Z

@mamabusi Thanks for taking the time to fix this up! LGTM

pushkarnk · 2018-03-29T06:06:03Z

@swift-ci please test and merge

pushkarnk · 2018-03-29T06:08:57Z

@swift-ci please test and merge

mamabusi commented Mar 9, 2018

View reviewed changes

itaiferber suggested changes Mar 21, 2018

View reviewed changes

Fix for NSKeyedArchiver.archiveRootObject api failure on Linux

77e436d

mamabusi force-pushed the nskeyedarchiver-branch branch from 0dccd4e to 77e436d Compare March 28, 2018 05:51

itaiferber approved these changes Mar 28, 2018

View reviewed changes

swift-ci merged commit d1272be into swiftlang:master Mar 29, 2018

Fix for NSKeyedArchiver.archiveRootObject api failure on Linux #1464

Fix for NSKeyedArchiver.archiveRootObject api failure on Linux #1464

Uh oh!

Conversation

mamabusi commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mamabusi Mar 9, 2018

Choose a reason for hiding this comment

Uh oh!

ianpartridge Mar 12, 2018

Choose a reason for hiding this comment

Uh oh!

itaiferber Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mamabusi Mar 9, 2018

Choose a reason for hiding this comment

Uh oh!

itaiferber Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge commented Mar 9, 2018

Uh oh!

spevans commented Mar 9, 2018

Uh oh!

pushkarnk commented Mar 12, 2018

Uh oh!

pushkarnk commented Mar 12, 2018

Uh oh!

parkera commented Mar 12, 2018

Uh oh!

mamabusi commented Mar 21, 2018

Uh oh!

itaiferber commented Mar 21, 2018

Uh oh!

itaiferber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mamabusi commented Mar 26, 2018

Uh oh!

itaiferber commented Mar 26, 2018

Uh oh!

mamabusi commented Mar 27, 2018

Uh oh!

itaiferber commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mamabusi commented Mar 28, 2018

Uh oh!

alblue commented Mar 28, 2018

Uh oh!

itaiferber commented Mar 28, 2018

Uh oh!

pushkarnk commented Mar 29, 2018

Uh oh!

pushkarnk commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mamabusi commented Mar 9, 2018 •

edited

Loading

itaiferber Mar 21, 2018 •

edited

Loading

itaiferber Mar 21, 2018 •

edited

Loading

itaiferber left a comment •

edited

Loading

itaiferber Mar 21, 2018 •

edited

Loading

itaiferber Mar 21, 2018 •

edited

Loading

itaiferber commented Mar 27, 2018 •

edited

Loading