Skip to content

TSCBasic: protect against an invalid API usage #325

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

TSCBasic: protect against an invalid API usage #325

wants to merge 1 commit into from

Conversation

compnerd
Copy link
Member

filePathRepresentation may not be invoked on an empty string. This
protects against that case which does occur during the SPM test suite
execution.

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test macOS platform

@compnerd compnerd force-pushed the order-will-be-maintained branch from f726748 to 5d31e72 Compare June 4, 2022 00:48
@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test

@compnerd compnerd force-pushed the order-will-be-maintained branch from 5d31e72 to 01849f8 Compare June 4, 2022 00:57
@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test macOS platform

@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test

1 similar comment
@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test

@compnerd
TSCBasic: protect against an invalid API usage
561bd3d 
`filePathRepresentation` may not be invoked on an empty string.  This
protects against that case which does occur during the SPM test suite
execution.
@compnerd compnerd force-pushed the order-will-be-maintained branch from b8025fc to 561bd3d Compare June 4, 2022 22:49
@compnerd
Copy link
Member Author

compnerd commented Jun 4, 2022

@swift-ci please test

@tomerd
Copy link
Contributor

tomerd commented Jun 8, 2022

this does a bit more than just the empty string case, can you elaborate on what is going on here?

cc @abertelrud

1 similar comment
@tomerd
Copy link
Contributor

tomerd commented Jun 8, 2022

this does a bit more than just the empty string case, can you elaborate on what is going on here?

cc @abertelrud

tomerd
tomerd reviewed Jun 8, 2022
View reviewed changes
Sources/TSCBasic/Path.swift
@@ -254,7 +259,18 @@ public struct RelativePath: Hashable {
/// normalization or canonicalization. This will construct a path without
/// an anchor and thus may be invalid.
fileprivate init(unsafeUncheckedPath string: String) {
self.init(PathImpl(string: string))
if string.isEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

does the case need to be a guard and then return a ~const empty representation?

tomerd
tomerd reviewed Jun 8, 2022
View reviewed changes
Sources/TSCBasic/Path.swift
@@ -254,7 +259,18 @@ public struct RelativePath: Hashable {
/// normalization or canonicalization. This will construct a path without
/// an anchor and thus may be invalid.
fileprivate init(unsafeUncheckedPath string: String) {
self.init(PathImpl(string: string))
if string.isEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

does the case need to be a guard and then return a ~const empty representation?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can change that of course, but that doesn't help the macOS case. It would be purely a syntactic change and should be equivalent to this.

@compnerd
Copy link
Member Author

compnerd commented Jun 8, 2022

@swift-ci please test macOS platform

@tomerd
Copy link
Contributor

tomerd commented Jun 8, 2022

@compnerd this does a bit more than just the empty string case, can you elaborate on what is going on here / why all the thing manipulation is needed?

cc @abertelrud @milseman

1 similar comment
@tomerd
Copy link
Contributor

tomerd commented Jun 8, 2022

@compnerd this does a bit more than just the empty string case, can you elaborate on what is going on here / why all the thing manipulation is needed?

cc @abertelrud @milseman

@compnerd
Copy link
Member Author

compnerd commented Jun 8, 2022

@tomerd I don't follow, its doing nothing at all ...

NSString.fileSystemRepresentation cannot be invoked on an empty string, so the check (or guard if you really want it just spelt that way for your tastes) is just protecting against that.

If it is not empty, we normalize the string and return that because the path operations should be performed on the normalized form (otherwise we need to go in at each point where an API is called and re-normalize the path before each call, which is extremely expensive as strings must be re-encoded to UTF16, have the conversion, re-encoded to UTF8, manipulated and then re-encoded to UTF16 to perform the operation).

@compnerd
Copy link
Member Author

compnerd commented Jun 8, 2022

Hmm, wait, do you mean the self.init(PathImpl(string: string)) in the case that the string is empty? I don't see how that is any different than writing self.init(PathImpl(string: "")). We already know that string is empty, so its just passing in string as opposed to creating a new String value from the "" literal. Is that you prefer spelling that as self.init(Self.constant)?

@tomerd
Copy link
Contributor

tomerd commented Jun 14, 2022

iiuc the non-empty code path is now:

#if _runtime(_ObjC)
             self.init(PathImpl(string: String(cString: NSURL(fileURLWithPath: string).fileSystemRepresentation)))
 #else
             let normalized: UnsafePointer<Int8> = string.fileSystemRepresentation
             defer { normalized.deallocate() }

             self.init(PathImpl(string: String(cString: normalized)))
 #endif

that is quite different from that the code used to be, and the motivation for this change (why do we need to use fileSystemRepresentation? what problem is it solving) is not explained as the motivation for this PR

@compnerd
Copy link
Member Author

Ah, I see.

The difference in the ObjC runtime is because fileSystemRepresentation behaviour is different. The reason for that is to give similar behaviour across the two paths.

As to why the call to .fileSystemRepresentation is to canonicalise the path (that will rewrite the filepath to use the natural path separator rather than POSIX. This is important because subsequent use of the resulting path makes assumptions that the path is in the canonicalised form rather than POSIX. If we do not perform this at this point, future attempts to get pieces of the path fail.

@tomerd
Copy link
Contributor

tomerd commented Jun 14, 2022

As to why the call to .fileSystemRepresentation is to canonicalise the path (that will rewrite the filepath to use the natural path separator rather than POSIX. This is important because subsequent use of the resulting path makes assumptions that the path is in the canonicalised form rather than POSIX. If we do not perform this at this point, future attempts to get pieces of the path fail.

can you give some concrete examples of what this does exactly / what concrete problem it solves. I am a bit worried about replying on fileSystemRepresentation.

@milseman @abertelrud can you chime in here too?

@compnerd
Copy link
Member Author

I suppose I can try to work through and find the test cases it fixes in SPM's test suite. It fixed a few of the last failures.

It literally converts /some/path\here to \some\path\here. There are a number of operations on FilePath that are dependent on the file path separator being canonical as they don't do the UTF8 -> UTF16 -> UTF8 conversion around each call (not to mention would make it pretty expensive).

@milseman
Copy link
Member

@milseman @abertelrud can you chime in here too?

Sorry, I'm not familiar with what this is. This doesn't look to be using FilePath, should it be?

@compnerd
Copy link
Member Author

Right, this is still using the path utilities from tools-support-core, not the swift-system FilePath.

@compnerd
Copy link
Member Author

@swift-ci please test Windows platform

@tomerd
Copy link
Contributor

tomerd commented Apr 11, 2023

@compnerd iirc #369 and swiftlang/swift-package-manager#5910 are designed to avoid some of these issues, we should prioritize getting them over the finish lines so we need less workarounds like this

@compnerd
Copy link
Member Author

@tomerd are you okay with merging this for now? I'm going to likely take a pause on some of the SPM work and see if we can get the two mentioned changes merged as that is going to just be generally easier to make progress with.

@tomerd
Copy link
Contributor

tomerd commented Apr 13, 2023

@compnerd are you asking if we can merge #369 and swiftlang/swift-package-manager#5910? I am happy to merge them if you can confirm they are not breaking anything for windows and helping with some of the windows path issues

@compnerd
Copy link
Member Author

@tomerd I meant merging this change so that I can have a little bit of breathing room to work through those two changes :)

@tomerd
Copy link
Contributor

tomerd commented Apr 13, 2023

I am not comfortable with this change for all platforms. if you can do something more minimal for windows we can consider that

@compnerd
Copy link
Member Author

@tomerd okay, can you please at least get the patch pair that you have building on Windows? I should be able to put it through its paces on Windows once you have the patches complete. Can we get that done early today so that I can test it? Alternatively, can we merge this temporarily so that we can work through the issues in the much larger refactoring?

@tomerd
Copy link
Contributor

tomerd commented Apr 14, 2023

@compnerd just kicked off the windows build - this is done via cross repo testing in swiftlang/swift-package-manager#5910. may need your help if there are any windows specific errors

@compnerd
Copy link
Member Author

I believe that this can be closed off now that we have the other change merged.

@compnerd compnerd closed this Apr 21, 2023
@compnerd compnerd deleted the order-will-be-maintained branch April 21, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomerd tomerd tomerd left review comments

@neonichu neonichu neonichu approved these changes

@aciidgh aciidgh Awaiting requested review from aciidgh

@abertelrud abertelrud Awaiting requested review from abertelrud

@friedbunny friedbunny Awaiting requested review from friedbunny

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@compnerd @tomerd @milseman @neonichu