Skip to content

Commit 81a71c6

Browse files
euanheuanh
committed
tar: Do not use String(format:) in octal6/11
String(format:) sometimes returns an empty string even if the number being formatted can be represented in an octal string of the requested length. This may be a thread-safety problem and has only been seen when running the tests. swiftlang/swift-corelibs-foundation#5152 This commit changes octal6/11 to use String(value, radix:) and handle padding directly. This has not failed during hundreds of test runs. Benchmarking the old and new implementations of octal6() shows that the new version is about twice as fast and makes no allocations, whereas the old version made 7 allocations.
1 parent 2de7109 commit 81a71c6

File tree

3 files changed

+112
-2
lines changed

3 files changed

+112
-2
lines changed

‎Package.swift

+16
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ let package = Package(
2727
.package(url: "https://github.com/apple/swift-crypto.git", "1.0.0"..<"4.0.0"),
2828
.package(url: "https://github.com/apple/swift-argument-parser", from: "1.3.0"),
2929
.package(url: "https://github.com/apple/swift-http-types.git", from: "1.2.0"),
30+
.package(url: "https://github.com/ordo-one/package-benchmark.git", from: "1.27.0"),
3031
],
3132
targets: [
3233
.target(
@@ -91,3 +92,18 @@ let package = Package(
9192
],
9293
swiftLanguageModes: [.v6]
9394
)
95+
96+
// Benchmark of Benchmarks
97+
package.targets += [
98+
.executableTarget(
99+
name: "Benchmarks",
100+
dependencies: [
101+
.product(name: "Benchmark", package: "package-benchmark"),
102+
.target(name: "Tar"),
103+
],
104+
path: "Benchmarks/Benchmarks",
105+
plugins: [
106+
.plugin(name: "BenchmarkPlugin", package: "package-benchmark")
107+
]
108+
),
109+
]

‎Sources/Tar/tar.swift

+23-2
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,33 @@ extension [UInt8] {
6666
}
6767
}
6868

69+
func newoctal6(_ value: Int) -> String {
70+
precondition(value >= 0)
71+
precondition(value < 0o777777)
72+
let str = String(value, radix: 8)
73+
return String(repeating: "0", count: 6 - str.count).appending(str)
74+
}
75+
76+
func newcustomoctal6(_ value: Int) -> String {
77+
precondition(value >= 0)
78+
precondition(value < 0o777777)
79+
var value = value
80+
var ret = ""
81+
for _ in (0..<6) {
82+
ret = String(value % 8) + ret
83+
value = value / 8
84+
}
85+
return ret
86+
}
87+
6988
/// Serializes an integer to a 6 character octal representation.
7089
/// - Parameter value: The integer to serialize.
7190
/// - Returns: The serialized form of `value`.
7291
func octal6(_ value: Int) -> String {
7392
precondition(value >= 0)
7493
precondition(value < 0o777777)
75-
return String(format: "%06o", value)
94+
let str = String(value, radix: 8)
95+
return String(repeating: "0", count: 6 - str.count).appending(str)
7696
}
7797

7898
/// Serializes an integer to a 11 character octal representation.
@@ -81,7 +101,8 @@ func octal6(_ value: Int) -> String {
81101
func octal11(_ value: Int) -> String {
82102
precondition(value >= 0)
83103
precondition(value < 0o777_7777_7777)
84-
return String(format: "%011o", value)
104+
let str = String(value, radix: 8)
105+
return String(repeating: "0", count: 11 - str.count).appending(str)
85106
}
86107

87108
// These ranges define the offsets of the standard fields in a Tar header.

‎Tests/TarTests/TarUnitTests.swift

+73
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,77 @@ let trailerLen = 2 * blocksize
7575
hdr == [97, 98, 99, 100, 101, 102, 0, 103, 104, 105, 32, 106, 107, 108, 0, 32, 109, 110, 111, 32, 0]
7676
)
7777
}
78+
79+
@Test func testTarHeader() async throws {
80+
// A zero-byte file is ok, but having no filename is probably not realistic
81+
let hdr = tarHeader(filesize: 0, filename: "")
82+
#expect(hdr.count == 512)
83+
#expect(
84+
hdr == [
85+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
86+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
87+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 48, 48, 48,
88+
53, 53, 53, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48,
89+
48, 48, 48, 48, 48, 32, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 32, 48, 48, 55, 49, 53, 54, 0, 32,
90+
48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
91+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
92+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 117,
93+
115, 116, 97, 114, 0, 48, 48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
94+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
95+
0, 0, 0, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
96+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
97+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
98+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
99+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
100+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
101+
]
102+
)
103+
104+
// No filename is not realistic
105+
let hdr2 = tarHeader(filesize: 1, filename: "")
106+
#expect(hdr2.count == 512)
107+
#expect(
108+
hdr2 == [
109+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
110+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
111+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 48, 48, 48,
112+
53, 53, 53, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48,
113+
48, 48, 48, 48, 49, 32, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 32, 48, 48, 55, 49, 53, 55, 0, 32,
114+
48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
115+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
116+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 117,
117+
115, 116, 97, 114, 0, 48, 48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
118+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
119+
0, 0, 0, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
120+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
121+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
122+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
123+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
124+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
125+
]
126+
)
127+
128+
let hdr3 = tarHeader(filesize: 1024, filename: "filename")
129+
#expect(hdr3.count == 512)
130+
#expect(
131+
hdr3 == [
132+
102, 105, 108, 101, 110, 97, 109, 101, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
133+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
134+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
135+
48, 48, 48, 53, 53, 53, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48,
136+
48, 48, 48, 48, 50, 48, 48, 48, 32, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 32, 48, 49, 48, 54, 54,
137+
49, 0, 32, 48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
138+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
139+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
140+
117, 115, 116, 97, 114, 0, 48, 48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
141+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
142+
0, 0, 0, 0, 0, 0, 48, 48, 48, 48, 48, 48, 32, 0, 48, 48, 48, 48, 48, 48, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0,
143+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
144+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
145+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
146+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
147+
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
148+
]
149+
)
150+
}
78151
}

0 commit comments

Comments
 (0)