-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: Add more path injection sinks #11742
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.
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
Swift: Add more path injection sinks #11742
Conversation
d6eea64
to
19f76c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. One small question.
NSString().write(toFile: remoteString, atomically: true, encoding: 0) // $ hasPathInjection=139 | ||
let _ = NSKeyedUnarchiver().unarchiveObject(withFile: remoteString) // $ hasPathInjection=139 | ||
let _ = ArchiveByteStream.fileStream(fd: remoteString as! FileDescriptor, automaticClose: true) // $ hasPathInjection=139 | ||
ArchiveByteStream.withFileStream(fd: remoteString as! FileDescriptor, automaticClose: true) { _ in } // $ hasPathInjection=139 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the implementation used by remoteString as! FileDescriptor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the functions being called? They're stubbed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was the conversion from String
(remoteString
) to FileDescriptor
(the argument type) that was puzzling me, I was expecting to see [stub] code for it somewhere. But I guess the test works...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a forced cast to keep it simple. That would probably not work if the code was actually executed, but it should be good enough for the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
19f76c3
to
6b52c15
Compare
Had to rebase to fix conflicts with the GRDB PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
6b52c15
to
c115a9f
Compare
No description provided.