SR-4993: Read zero length files on Linux #1166
Conversation
TestFoundation/TestNSData.swift
Outdated
|
|
||
| func test_contentsOfZeroFile() { | ||
| #if os(Linux) | ||
| let contents = NSData(contentsOfFile: "/proc/self/cmdline") |
There was a problem hiding this comment.
It's possible that the /proc/ filesystem isn't mounted in some situations, so I would guard this with a check to see if /proc/self/cmdline exists and silently abort if it doesn't - because it's possible someone will run this with a minimal container where procfs isn't present.
There was a problem hiding this comment.
Good point, I will wrap something around it
|
@swift-ci please test |
|
Rebased, as I was printing the contents of |
TestFoundation/TestNSData.swift
Outdated
| let maps = try String(contentsOfFile: "/proc/self/maps", encoding: .utf8) | ||
| XCTAssertTrue(maps.characters.count > 0) | ||
| } catch { | ||
| XCTFail() |
There was a problem hiding this comment.
Might be worth having a failure reason here inside the XCTFail?
| var amt = 0 | ||
|
|
||
| repeat { | ||
| data = realloc(data, bytesRead + blockSize) |
There was a problem hiding this comment.
This may return NULL, in which case the unwrap subsequently will fail. Is there any reason why we can't use a single byte array for reading and then have an NSMutableData to append the bytes into? That would solve the realloc calls.
There was a problem hiding this comment.
I was duplicating the usage of malloc() in the readBytesFromFileWithExtendedAttributes function above. Generally the pattern in Foundation seems to be not checking the return value of malloc()/realloc() and letting it eventually abort on a NULL deref. The same is true if bytesRead exceeds Int.max
|
@swift-ci please test |
|
@alblue could you give this another test please, I think ci missed the last one |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
It seems a shame to wrap all this in |
|
Its not a problem to remove them, although I will leave them in the test case for now, I dont think macOS has any files that can be used for tests. |
- Some files (eg in /proc) report a filesize of zero and need to be read into a buffer instead of using mmap(). - Check that '/proc/self' exists before using it for tests.
|
I removed the |
|
What do you think @alblue? |
|
@parkera Any thoughts on this PR? |
|
We've been having a discussion about this on the team, trying to decide if it's non-portable behavior or not. In the end we decided that if you're accessing |
|
@parkera Is this ok to merge now? |
|
@swift-ci please test |
|
Yup, we can merge when ready. |
|
@parkera Could you test and merge please, my swift-ci access isnt currently working |
|
@swift-ci please test and merge |
be read into a buffer instead of using mmap().