Skip to content

Commit 29a96fa

Browse files
cdumezcdumez
committed
Make CString's literal, buffer and mutable accessors reachable only through CStringWithEncoding
https://bugs.webkit.org/show_bug.cgi?id=324791 Reviewed by Darin Adler. CString(ASCIILiteral), CString(CStringBuffer*), mutableSpan(), mutableSpanIncludingNullTerminator() and grow() each either put bytes into a CString or hand out a buffer to put them into, and the encoding-erased base has no way to say what encoding those bytes are in. They become protected, joining newUninitialized(). CString(const std::string&) is deleted: it had no callers, and dropping it removes the implicit std::string conversion. ASCIILiteral converts to const char*, but CString(ASCIILiteral) is the better match, so overload resolution still selects it and the access check then rejects the call: a literal is refused outright rather than quietly losing its length, and any embedded null, to a strlen. A std::string does still convert to a span, so there only the implicit conversion goes away. A FIXME marks the two remaining raw-byte constructors. JIT::compileTimeStats() keys its map with ASCII stat names, so it and its jsc.cpp consumer are typed ASCIICString, whose data() is the const char* printf wants. FormData::create() takes a UTF8CString, which is what its callers already hold; PingLoader builds its body from a byte span instead of allocating a CString. extractAXLogArg()'s fallback is constrained on derived_from rather than same_as, so an already-typed UTF8CString reaches the const char* overload instead of being passed to WTFLogAlways as a class type. * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp: (JSC::FTL::DFG::LowerDFGToB3::lower): * Source/JavaScriptCore/jit/JIT.cpp: (JSC::JIT::compileTimeStats): * Source/JavaScriptCore/jit/JIT.h: * Source/JavaScriptCore/jsc.cpp: (runJSC): * Source/WTF/wtf/text/CString.h: (WTF::CString::CString): * Source/WebCore/accessibility/AXLogger.h: (WebCore::convertAXLogArg): (WebCore::extractAXLogArg): * Source/WebCore/loader/PingLoader.cpp: (WebCore::PingLoader::sendPing): * Source/WebCore/platform/network/FormData.cpp: (WebCore::FormData::create): * Source/WebCore/platform/network/FormData.h: * Source/WebCore/testing/MockCDMFactory.cpp: (WebCore::MockCDMInstanceSession::requestLicense): (WebCore::MockCDMInstanceSession::loadSession): (WebCore::MockCDMInstanceSession::removeSessionData): * Tools/TestWebKitAPI/Tests/WTF/CString.cpp: (TEST(WTF, CStringCopyOnWrite)): (TEST(WTF, CStringStdStringInterop)): (TEST(WTF, CStringViewASCIICaseConversions)): (requires): * Tools/TestWebKitAPI/Tests/WTF/SynchronizedFixedQueue.cpp: (TestWebKitAPI::textItem): (TestWebKitAPI::toUpper): (TestWebKitAPI::ToUpperConverter::ToUpperConverter): (TestWebKitAPI::ToUpperConverter::startProducing): (TestWebKitAPI::ToUpperConverter::startConsuming): (TestWebKitAPI::ToUpperConverter::enqueueLower): (TestWebKitAPI::TEST(WTF_SynchronizedFixedQueue, ProduceOnly)): (TestWebKitAPI::TEST(WTF_SynchronizedFixedQueue, Limits)): Canonical link: https://commits.webkit.org/321673@main
1 parent 36a3ac7 commit 29a96fa

18 files changed

Lines changed: 111 additions & 79 deletions

File tree

‎Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ class LowerDFGToB3 {
216216
{
217217
State* state = &m_ftlState;
218218

219-
CString name;
219+
UTF8CString name;
220220
if (verboseCompilationEnabled()) {
221221
name = toUTF8CString(
222222
"jsBody_", ++compileCounter, "_", codeBlock()->inferredName(),

‎Source/JavaScriptCore/jit/JIT.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,9 +1079,9 @@ int JIT::stackPointerOffsetFor(CodeBlock* codeBlock)
10791079
return stackPointerOffsetFor(codeBlock->unlinkedCodeBlock());
10801080
}
10811081

1082-
UncheckedKeyHashMap<CString, Seconds> JIT::compileTimeStats()
1082+
UncheckedKeyHashMap<ASCIICString, Seconds> JIT::compileTimeStats()
10831083
{
1084-
UncheckedKeyHashMap<CString, Seconds> result;
1084+
UncheckedKeyHashMap<ASCIICString, Seconds> result;
10851085
if (Options::reportTotalCompileTimes()) {
10861086
result.add("Total Compile Time"_s, totalCompileTime());
10871087
result.add("Baseline Compile Time"_s, totalBaselineCompileTime);

‎Source/JavaScriptCore/jit/JIT.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ namespace JSC {
189189
static int NODELETE stackPointerOffsetFor(UnlinkedCodeBlock*);
190190
static int stackPointerOffsetFor(CodeBlock*);
191191

192-
JS_EXPORT_PRIVATE static UncheckedKeyHashMap<CString, Seconds> compileTimeStats();
192+
JS_EXPORT_PRIVATE static UncheckedKeyHashMap<ASCIICString, Seconds> compileTimeStats();
193193
JS_EXPORT_PRIVATE static Seconds NODELETE totalCompileTime();
194194

195195
private:

‎Source/JavaScriptCore/jsc.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4667,11 +4667,11 @@ int runJSC(const CommandLine& options, bool isWorker, const Func& func)
46674667
}
46684668

46694669
auto compileTimeStats = JIT::compileTimeStats();
4670-
Vector<CString> compileTimeKeys;
4670+
Vector<ASCIICString> compileTimeKeys;
46714671
for (auto& entry : compileTimeStats)
46724672
compileTimeKeys.append(entry.key);
46734673
std::sort(compileTimeKeys.begin(), compileTimeKeys.end());
4674-
for (const CString& key : compileTimeKeys) {
4674+
for (const ASCIICString& key : compileTimeKeys) {
46754675
if (key.data())
46764676
printf("%40s: %.3lf ms\n", key.data(), compileTimeStats.get(key).milliseconds());
46774677
}

‎Source/WTF/wtf/text/CString.h‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,10 @@ class CString {
8181
WTF_DEPRECATED_MAKE_FAST_ALLOCATED(CString);
8282
public:
8383
CString() { }
84-
WTF_EXPORT_PRIVATE CString(ASCIILiteral);
84+
// FIXME: These two should join the constructors below, which would leave CStringWithEncoding
85+
// as the only way to get bytes into a CString.
8586
WTF_EXPORT_PRIVATE CString(const char*); // Any encoding
8687
WTF_EXPORT_PRIVATE CString(std::span<const char>); // Any encoding
87-
CString(CStringBuffer* buffer) : m_buffer(buffer) { }
88-
CString(const std::string&); // Any encoding.
8988
CString(HashTableDeletedValueType) : m_buffer(HashTableDeletedValue) { }
9089

9190
const char* data() const LIFETIME_BOUND; // Any encoding
@@ -101,11 +100,6 @@ class CString {
101100
std::span<const char> span() const LIFETIME_BOUND; // Any encoding
102101
std::span<const char> spanIncludingNullTerminator() const LIFETIME_BOUND; // Any encoding
103102

104-
// Copy-on-write
105-
WTF_EXPORT_PRIVATE std::span<char> mutableSpan() LIFETIME_BOUND;
106-
WTF_EXPORT_PRIVATE std::span<char> mutableSpanIncludingNullTerminator() LIFETIME_BOUND;
107-
WTF_EXPORT_PRIVATE void grow(size_t newLength);
108-
109103
size_t length() const;
110104

111105
bool isNull() const { return !m_buffer; }
@@ -119,10 +113,20 @@ class CString {
119113
WTF_EXPORT_PRIVATE unsigned NODELETE hash() const;
120114

121115
protected:
122-
// Only reachable through CStringWithEncoding below: this hands out a mutable buffer, so there is no
123-
// point at which the encoding of the bytes written into it could be established.
116+
// Only reachable through CStringWithEncoding below. Each of these either puts bytes into a
117+
// CString or hands out a buffer to put them into, and the encoding-erased base has no way to
118+
// say what encoding those bytes are in. An ASCII literal is valid in every supported encoding,
119+
// but a CString built from one still has to name the encoding it is going to be read as.
120+
WTF_EXPORT_PRIVATE CString(ASCIILiteral);
121+
CString(CStringBuffer* buffer) : m_buffer(buffer) { }
122+
124123
WTF_EXPORT_PRIVATE static CString newUninitialized(size_t length, std::span<char>& characterBuffer);
125124

125+
// Copy-on-write
126+
WTF_EXPORT_PRIVATE std::span<char> mutableSpan() LIFETIME_BOUND;
127+
WTF_EXPORT_PRIVATE std::span<char> mutableSpanIncludingNullTerminator() LIFETIME_BOUND;
128+
WTF_EXPORT_PRIVATE void grow(size_t newLength);
129+
126130
private:
127131
void copyBufferIfNeeded();
128132
void init(std::span<const char>);
@@ -148,11 +152,6 @@ template<> struct DefaultHash<CString> : CStringHash { };
148152
template<typename> struct HashTraits;
149153
template<> struct HashTraits<CString> : SimpleClassHashTraits<CString> { };
150154

151-
inline CString::CString(const std::string& value)
152-
: CString(unsafeMakeSpan(value.data(), value.size()))
153-
{
154-
}
155-
156155
inline const char* CString::data() const LIFETIME_BOUND
157156
{
158157
return m_buffer ? m_buffer->spanIncludingNullTerminator().data() : nullptr;
@@ -249,6 +248,7 @@ template<typename CharacterType> class CStringWithEncoding final : public CStrin
249248
std::span<const CharacterType> spanIncludingNullTerminator() const LIFETIME_BOUND { return byteCast<CharacterType>(CString::spanIncludingNullTerminator()); }
250249
std::span<CharacterType> mutableSpan() LIFETIME_BOUND { return byteCast<CharacterType>(CString::mutableSpan()); }
251250
std::span<CharacterType> mutableSpanIncludingNullTerminator() LIFETIME_BOUND { return byteCast<CharacterType>(CString::mutableSpanIncludingNullTerminator()); }
251+
using CString::grow;
252252
CStringWithEncoding isolatedCopy() const { return CStringWithEncoding { span() }; }
253253

254254
// This is the escape hatch for external C functions and printf-style formatting. It is named for the
@@ -291,6 +291,13 @@ template<typename CharacterType> class CStringWithEncoding final : public CStrin
291291
}
292292
};
293293

294+
// Latin-1 is refused for the same reason legacyCStringPointer() withholds a pointer from it: a printf
295+
// destination reads the bytes back as UTF-8 or ASCII, so a non-ASCII Latin-1 byte would be garbled. Use
296+
// String::utf8(), or String::ascii() where losing a non-ASCII character to '?' is as acceptable as it is
297+
// for the other unprintables. This is an exact match, so it wins over the CString overload above, which
298+
// would require a derived-to-base conversion.
299+
const char* safePrintfType(const Latin1CString&) = delete;
300+
294301
// These are exact matches, so they win over the CString overloads above, which would require a derived-to-base conversion.
295302
template<typename CharacterType>
296303
bool NODELETE operator==(const CStringWithEncoding<CharacterType>& a, const CStringWithEncoding<CharacterType>& b)

‎Source/WebCore/Modules/reporting/DeprecationReportBody.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ Ref<FormData> DeprecationReportBody::createReportFormDataForViolation() const
7676
reportObject->setString("url"_s, ""_s);
7777
reportObject->setObject("body"_s, WTF::move(reportBody));
7878

79-
return FormData::create(reportObject->toJSONString().utf8());
79+
return FormData::create(reportObject->toJSONString().utf8().span());
8080
}
8181

8282
} // namespace WebCore

‎Source/WebCore/Modules/reporting/Report.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ Ref<FormData> Report::createReportFormDataForViolation(const String& type, const
7878
auto reportList = JSON::Array::create();
7979
reportList->pushObject(reportObject);
8080

81-
return FormData::create(reportList->toJSONString().utf8());
81+
return FormData::create(reportList->toJSONString().utf8().span());
8282
}
8383

8484
} // namespace WebCore

‎Source/WebCore/accessibility/AXLogger.h‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include <wtf/MonotonicTime.h>
3232
#include <wtf/RefPtr.h>
3333
#include <wtf/Vector.h>
34+
#include <wtf/text/CString.h>
3435

3536
namespace WebCore {
3637

@@ -106,20 +107,23 @@ template<typename T>
106107
decltype(auto) convertAXLogArg(T&& arg)
107108
{
108109
if constexpr (std::is_pointer_v<std::remove_cvref_t<T>> && std::derived_from<std::remove_pointer_t<std::remove_cvref_t<T>>, AXCoreObject>)
109-
return arg ? arg->debugDescription().utf8() : CString("(null)"_s);
110+
return arg ? arg->debugDescription().utf8() : UTF8CString { "(null)"_s };
110111
else if constexpr (std::derived_from<std::remove_cvref_t<T>, AXCoreObject>)
111112
return arg.debugDescription().utf8();
112113
else if constexpr (requires { { *arg } -> std::convertible_to<const AXCoreObject&>; })
113-
return arg ? arg->debugDescription().utf8() : CString("(null)"_s);
114+
return arg ? arg->debugDescription().utf8() : UTF8CString { "(null)"_s };
114115
else if constexpr (std::same_as<std::remove_cvref_t<T>, String>)
115116
return arg.utf8();
116117
else
117118
return std::forward<T>(arg);
118119
}
119120

120-
inline const char* extractAXLogArg(const CString& string) { return string.data(); }
121+
// WTFLogAlways is raw varargs, so a converted string has to arrive as a pointer. Only UTF-8 is offered:
122+
// the conversions above all produce one, and the generic overload below excludes every CString so that a
123+
// Latin-1 or encoding-erased one is rejected rather than logged as bytes.
124+
inline const char* extractAXLogArg(const UTF8CString& string) { return string.legacyCStringPointer(); }
121125

122-
template<typename T> requires (!std::same_as<std::remove_cvref_t<T>, CString>)
126+
template<typename T> requires (!std::derived_from<std::remove_cvref_t<T>, CString>)
123127
decltype(auto) extractAXLogArg(T&& arg) { return std::forward<T>(arg); }
124128

125129
// Used like WTFLogAlways, but auto-converts String and AXCoreObject arguments for use with the %s format specifier.

‎Source/WebCore/loader/PingLoader.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ void PingLoader::sendPing(LocalFrame& frame, URL&& sendPingURL, const URL& desti
146146

147147
request.setHTTPMethod("POST"_s);
148148
request.setHTTPContentType("text/ping"_s);
149-
request.setHTTPBody(FormData::create("PING"_s));
149+
request.setHTTPBody(FormData::create("PING"_span8));
150150
request.setHTTPHeaderField(HTTPHeaderName::CacheControl, HTTPHeaderValues::maxAge0());
151151

152152
Ref sourceOrigin = document->securityOrigin();

‎Source/WebCore/loader/SubresourceIntegrity.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ static Ref<FormData> createReportFormData(const String& type, const URL& url, co
186186
auto reportList = JSON::Array::create();
187187
reportList->pushObject(reportObject);
188188

189-
return FormData::create(reportList->toJSONString().utf8());
189+
return FormData::create(reportList->toJSONString().utf8().span());
190190
}
191191

192192
static String addHashPrefix(ResourceCryptographicDigest::Algorithm algorithm, StringView hash)

0 commit comments

Comments
 (0)