Skip to content

Commit 42a2020

Browse files
David KilzerJonWBedard
David Kilzer
authored and
JonWBedard
committed
[WTF] Add LIFETIME_BOUND to CString.h methods
<https://bugs.webkit.org/show_bug.cgi?id=280809> <rdar://137179585> Reviewed by Darin Adler. * Source/JavaScriptCore/tools/VMInspector.cpp: (JSC::VMInspector::dumpRegisters): - Fix use-after-free by storing CString on the stack instead of the const char* pointer. * Source/WTF/wtf/text/CString.h: - Add LIFETIME_BOUND attributes to methods that return pointers or references to inner data. * Source/WTF/wtf/text/StringConcatenate.h: (WTF::StringTypeAdapter<CString, void>): Add. * Source/WTF/wtf/text/TextStream.cpp: (WTF::TextStream::operator<<): Add. * Source/WTF/wtf/text/TextStream.h: (WTF::TextStream::operator<<): Add. - Add support for CString objects. * Source/WebCore/platform/graphics/BitmapImage.h: (WebCore::BitmapImage::sourceUTF8 const): * Source/WebCore/platform/graphics/BitmapImageSource.cpp: (WebCore::BitmapImageSource::destroyDecodedData): (WebCore::BitmapImageSource::stopDecodingWorkQueue): (WebCore::BitmapImageSource::decode): (WebCore::BitmapImageSource::imageFrameDecodeAtIndexHasFinished): (WebCore::BitmapImageSource::requestNativeImageAtIndex): (WebCore::BitmapImageSource::requestNativeImageAtIndexIfNeeded): (WebCore::BitmapImageSource::nativeImageAtIndexCacheIfNeeded): (WebCore::BitmapImageSource::sourceUTF8 const): * Source/WebCore/platform/graphics/BitmapImageSource.h: (WebCore::BitmapImageSource::sourceUTF8 const): * Source/WebCore/platform/graphics/ImageFrameAnimator.cpp: (WebCore::ImageFrameAnimator::startAnimation): (WebCore::ImageFrameAnimator::advanceAnimation): (WebCore::ImageFrameAnimator::sourceUTF8 const): * Source/WebCore/platform/graphics/ImageFrameAnimator.h: (WebCore::ImageFrameAnimator::sourceUTF8 const): * Source/WebCore/platform/graphics/ImageFrameWorkQueue.cpp: (WebCore::ImageFrameWorkQueue::start): (WebCore::ImageFrameWorkQueue::stop): - Change return type of sourceUTF8() methods to CString to keep the string alive until it's needed for logging. This fixes the use-after-free in the original BitmapImage::sourceUTF8() method. - Add call to CString::data() when logging. Originally-landed-as: 283286.185@safari-7620-branch (fb73615). rdar://141323047 Canonical link: https://commits.webkit.org/288678@main
1 parent 810335a commit 42a2020

11 files changed

Lines changed: 51 additions & 36 deletions

File tree

‎Source/JavaScriptCore/tools/VMInspector.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -508,8 +508,8 @@ SUPPRESS_ASAN void VMInspector::dumpRegisters(CallFrame* callFrame)
508508
dataLogF("% 4d CodeBlock : %10p 0x%llx ", registerNumber++, it++, (long long)codeBlock);
509509
dataLogLn(codeBlock);
510510
long long calleeBits = (long long)callFrame->callee().rawPtr();
511-
auto calleeString = valueAsString(it->jsValue()).data();
512-
dataLogF("% 4d Callee : %10p 0x%llx %s\n", registerNumber++, it++, calleeBits, calleeString);
511+
auto calleeString = valueAsString(it->jsValue());
512+
dataLogF("% 4d Callee : %10p 0x%llx %s\n", registerNumber++, it++, calleeBits, calleeString.data());
513513

514514
StackVisitor::visit(callFrame, vm, [&] (StackVisitor& visitor) {
515515
if (visitor->callFrame() == callFrame) {

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ class CStringBuffer final : public RefCounted<CStringBuffer> {
4545
size_t length() const { return m_length; }
4646

4747
WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN
48-
std::span<const LChar> span() const { return unsafeMakeSpan(reinterpret_cast_ptr<const LChar*>(this + 1), m_length); }
49-
std::span<const char> spanIncludingNullTerminator() const { return unsafeMakeSpan(reinterpret_cast_ptr<const char*>(this + 1), m_length + 1); }
48+
std::span<const LChar> span() const LIFETIME_BOUND { return unsafeMakeSpan(reinterpret_cast_ptr<const LChar*>(this + 1), m_length); }
49+
std::span<const char> spanIncludingNullTerminator() const LIFETIME_BOUND { return unsafeMakeSpan(reinterpret_cast_ptr<const char*>(this + 1), m_length + 1); }
5050
WTF_ALLOW_UNSAFE_BUFFER_USAGE_END
5151

5252
private:
@@ -56,8 +56,8 @@ WTF_ALLOW_UNSAFE_BUFFER_USAGE_END
5656

5757
CStringBuffer(size_t length) : m_length(length) { }
5858
WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN
59-
std::span<char> mutableSpan() { return unsafeMakeSpan(reinterpret_cast_ptr<char*>(this + 1), m_length); }
60-
std::span<char> mutableSpanIncludingNullTerminator() { return unsafeMakeSpan(reinterpret_cast_ptr<char*>(this + 1), m_length + 1); }
59+
std::span<char> mutableSpan() LIFETIME_BOUND { return unsafeMakeSpan(reinterpret_cast_ptr<char*>(this + 1), m_length); }
60+
std::span<char> mutableSpanIncludingNullTerminator() LIFETIME_BOUND { return unsafeMakeSpan(reinterpret_cast_ptr<char*>(this + 1), m_length + 1); }
6161
WTF_ALLOW_UNSAFE_BUFFER_USAGE_END
6262

6363
const size_t m_length;
@@ -77,21 +77,21 @@ class CString final {
7777
WTF_EXPORT_PRIVATE static CString newUninitialized(size_t length, std::span<char>& characterBuffer);
7878
CString(HashTableDeletedValueType) : m_buffer(HashTableDeletedValue) { }
7979

80-
const char* data() const;
80+
const char* data() const LIFETIME_BOUND;
8181

8282
std::string toStdString() const { return m_buffer ? std::string(m_buffer->spanIncludingNullTerminator().data()) : std::string(); }
8383

84-
std::span<const LChar> span() const;
85-
std::span<const char> spanIncludingNullTerminator() const;
84+
std::span<const LChar> span() const LIFETIME_BOUND;
85+
std::span<const char> spanIncludingNullTerminator() const LIFETIME_BOUND;
8686

87-
WTF_EXPORT_PRIVATE std::span<char> mutableSpan();
88-
WTF_EXPORT_PRIVATE std::span<char> mutableSpanIncludingNullTerminator();
87+
WTF_EXPORT_PRIVATE std::span<char> mutableSpan() LIFETIME_BOUND;
88+
WTF_EXPORT_PRIVATE std::span<char> mutableSpanIncludingNullTerminator() LIFETIME_BOUND;
8989
size_t length() const;
9090

9191
bool isNull() const { return !m_buffer; }
9292
bool isSafeToSendToAnotherThread() const;
9393

94-
CStringBuffer* buffer() const { return m_buffer.get(); }
94+
CStringBuffer* buffer() const LIFETIME_BOUND { return m_buffer.get(); }
9595

9696
bool isHashTableDeletedValue() const { return m_buffer.isHashTableDeletedValue(); }
9797

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2010-2023 Apple Inc. All rights reserved.
2+
* Copyright (C) 2010-2024 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -175,6 +175,14 @@ template<typename CharacterType, size_t Extent> class StringTypeAdapter<std::spa
175175
std::span<const CharacterType> m_characters;
176176
};
177177

178+
template<> class StringTypeAdapter<CString, void> : public StringTypeAdapter<std::span<const char>, void> {
179+
public:
180+
StringTypeAdapter(const CString& string)
181+
: StringTypeAdapter<std::span<const char>, void> { spanReinterpretCast<const char>(string.span()) }
182+
{
183+
}
184+
};
185+
178186
template<> class StringTypeAdapter<ASCIILiteral, void> : public StringTypeAdapter<std::span<const LChar>, void> {
179187
public:
180188
StringTypeAdapter(ASCIILiteral characters)

‎Source/WTF/wtf/text/TextStream.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,12 @@ TextStream& TextStream::operator<<(const AtomString& string)
125125
return *this;
126126
}
127127

128+
TextStream& TextStream::operator<<(const CString& string)
129+
{
130+
m_text.append(string);
131+
return *this;
132+
}
133+
128134
TextStream& TextStream::operator<<(const String& string)
129135
{
130136
m_text.append(string);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ class TextStream {
7777
WTF_EXPORT_PRIVATE TextStream& operator<<(const char*);
7878
WTF_EXPORT_PRIVATE TextStream& operator<<(const void*);
7979
WTF_EXPORT_PRIVATE TextStream& operator<<(const AtomString&);
80+
WTF_EXPORT_PRIVATE TextStream& operator<<(const CString&);
8081
WTF_EXPORT_PRIVATE TextStream& operator<<(const String&);
8182
WTF_EXPORT_PRIVATE TextStream& operator<<(ASCIILiteral);
8283
WTF_EXPORT_PRIVATE TextStream& operator<<(StringView);

‎Source/WebCore/platform/graphics/BitmapImage.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class BitmapImage final : public Image {
8585
RefPtr<NativeImage> nativeImageAtIndex(unsigned index) final { return m_source->nativeImageAtIndex(index); }
8686

8787
// Testing support.
88-
const char* sourceUTF8() const { return sourceURL().string().utf8().data(); }
88+
CString sourceUTF8() const { return sourceURL().string().utf8(); }
8989
void setAsyncDecodingEnabledForTesting(bool enabled) { m_source->setAsyncDecodingEnabledForTesting(enabled); }
9090
bool isAsyncDecodingEnabledForTesting() const { return m_source->isAsyncDecodingEnabledForTesting(); }
9191
void setMinimumDecodingDurationForTesting(Seconds duration) { m_source->setMinimumDecodingDurationForTesting(duration); }

‎Source/WebCore/platform/graphics/BitmapImageSource.cpp‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ EncodedDataStatus BitmapImageSource::dataChanged(FragmentedSharedBuffer* data, b
123123

124124
void BitmapImageSource::destroyDecodedData(bool destroyAll)
125125
{
126-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoded data with destroyAll = %d will be destroyed.", __FUNCTION__, this, sourceUTF8(), destroyAll);
126+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoded data with destroyAll = %d will be destroyed.", __FUNCTION__, this, sourceUTF8().data(), destroyAll);
127127

128128
bool canDestroyDecodedData = destroyAll && this->canDestroyDecodedData();
129129

@@ -331,7 +331,7 @@ bool BitmapImageSource::isDecodingWorkQueueIdle() const
331331

332332
void BitmapImageSource::stopDecodingWorkQueue()
333333
{
334-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoding work queue will be stopped.", __FUNCTION__, this, sourceUTF8());
334+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoding work queue will be stopped.", __FUNCTION__, this, sourceUTF8().data());
335335

336336
if (!m_workQueue || !m_workQueue->isIdle())
337337
return;
@@ -358,7 +358,7 @@ void BitmapImageSource::decode(Function<void(DecodingStatus)>&& decodeCallback)
358358
unsigned index = currentFrameIndex();
359359

360360
if (isPendingDecodingAtIndex(index, SubsamplingLevel::Default, DecodingMode::Asynchronous)) {
361-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d is being decoded.", __FUNCTION__, this, sourceUTF8(), index);
361+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d is being decoded.", __FUNCTION__, this, sourceUTF8().data(), index);
362362
return;
363363
}
364364

@@ -369,18 +369,18 @@ void BitmapImageSource::decode(Function<void(DecodingStatus)>&& decodeCallback)
369369
// startAnimation() always decodes the nextFrame which is currentFrameIndex + 1.
370370
// If primaryFrameIndex = 0, then the sequence of decoding is { 1, 2, .., n, 0, 1, ...}.
371371
if (startAnimation(SubsamplingLevel::Default, DecodingMode::Asynchronous)) {
372-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Animator has requested decoding next frame at index = %d.", __FUNCTION__, this, sourceUTF8(), index);
372+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Animator has requested decoding next frame at index = %d.", __FUNCTION__, this, sourceUTF8().data(), index);
373373
return;
374374
}
375375
}
376376

377377
if (!isCompatibleNativeImage) {
378-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoding for frame at index = %d will be requested.", __FUNCTION__, this, sourceUTF8(), index);
378+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoding for frame at index = %d will be requested.", __FUNCTION__, this, sourceUTF8().data(), index);
379379
requestNativeImageAtIndex(index, SubsamplingLevel::Default, ImageAnimatingState::No, { DecodingMode::Asynchronous });
380380
return;
381381
}
382382

383-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d was decoded for natural size.", __FUNCTION__, this, sourceUTF8(), index);
383+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d was decoded for natural size.", __FUNCTION__, this, sourceUTF8().data(), index);
384384
callDecodeCallbacks(DecodingStatus::Complete);
385385
}
386386

@@ -422,12 +422,12 @@ void BitmapImageSource::imageFrameDecodeAtIndexHasFinished(unsigned index, Subsa
422422
ASSERT(index < m_frames.size());
423423

424424
if (!nativeImage || !m_decoder) {
425-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d has failed.", __FUNCTION__, this, sourceUTF8(), index);
425+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d has failed.", __FUNCTION__, this, sourceUTF8().data(), index);
426426

427427
destroyNativeImageAtIndex(index);
428428
imageFrameDecodeAtIndexHasFinished(index, animatingState, DecodingStatus::Invalid);
429429
} else {
430-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d has been decoded.", __FUNCTION__, this, sourceUTF8(), index);
430+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d has been decoded.", __FUNCTION__, this, sourceUTF8().data(), index);
431431

432432
cacheNativeImageAtIndex(index, subsamplingLevel, options, nativeImage.releaseNonNull());
433433

@@ -517,7 +517,7 @@ DecodingStatus BitmapImageSource::requestNativeImageAtIndex(unsigned index, Subs
517517
if (index >= m_frames.size())
518518
return DecodingStatus::Invalid;
519519

520-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoding for frame at index = %d will be requested.", __FUNCTION__, this, sourceUTF8(), index);
520+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Decoding for frame at index = %d will be requested.", __FUNCTION__, this, sourceUTF8().data(), index);
521521

522522
workQueue().dispatch({ index, subsamplingLevel, animatingState, options });
523523

@@ -534,7 +534,7 @@ DecodingStatus BitmapImageSource::requestNativeImageAtIndexIfNeeded(unsigned ind
534534

535535
// Never decode the same frame from two different threads.
536536
if (isPendingDecodingAtIndex(index, subsamplingLevel, options)) {
537-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d is being decoded.", __FUNCTION__, this, sourceUTF8(), index);
537+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d is being decoded.", __FUNCTION__, this, sourceUTF8().data(), index);
538538
++m_blankDrawCountForTesting;
539539
return DecodingStatus::Decoding;
540540
}
@@ -557,7 +557,7 @@ Expected<Ref<NativeImage>, DecodingStatus> BitmapImageSource::nativeImageAtIndex
557557
// FIXME: Remove this for CG; ImageIO should be thread safe when decoding the same frame from multiple threads.
558558
// Never decode the same frame from two different threads.
559559
if (isPendingDecodingAtIndex(index, subsamplingLevel, options)) {
560-
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d is being decoded.", __FUNCTION__, this, sourceUTF8(), index);
560+
LOG(Images, "BitmapImageSource::%s - %p - url: %s. Frame at index = %d is being decoded.", __FUNCTION__, this, sourceUTF8().data(), index);
561561
++m_blankDrawCountForTesting;
562562
return makeUnexpected(DecodingStatus::Decoding);
563563
}
@@ -692,7 +692,7 @@ long long BitmapImageSource::expectedContentLength() const
692692
return m_bitmapImage ? m_bitmapImage->expectedContentLength() : 0;
693693
}
694694

695-
const char* BitmapImageSource::sourceUTF8() const
695+
CString BitmapImageSource::sourceUTF8() const
696696
{
697697
constexpr const char* emptyString = "";
698698
return m_bitmapImage ? m_bitmapImage->sourceUTF8() : emptyString;

‎Source/WebCore/platform/graphics/BitmapImageSource.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ class BitmapImageSource : public ImageSource {
8484
DecodingStatus frameDecodingStatusAtIndex(unsigned index) const final;
8585

8686
// Testing support
87-
const char* sourceUTF8() const;
87+
CString sourceUTF8() const;
8888

8989
private:
9090
BitmapImageSource(BitmapImage&, AlphaOption, GammaAndColorProfileOption);

‎Source/WebCore/platform/graphics/ImageFrameAnimator.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,10 @@ bool ImageFrameAnimator::startAnimation(SubsamplingLevel subsamplingLevel, const
133133
m_nextFrameSubsamplingLevel = subsamplingLevel;
134134
m_nextFrameOptions = options;
135135

136-
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Animation at index = %d will be started.", __FUNCTION__, this, sourceUTF8(), m_currentFrameIndex);
136+
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Animation at index = %d will be started.", __FUNCTION__, this, sourceUTF8().data(), m_currentFrameIndex);
137137

138138
if (options.decodingMode() == DecodingMode::Asynchronous) {
139-
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Decoding for frame at index = %d will be requested.", __FUNCTION__, this, sourceUTF8(), nextFrameIndex());
139+
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Decoding for frame at index = %d will be requested.", __FUNCTION__, this, sourceUTF8().data(), nextFrameIndex());
140140
source->requestNativeImageAtIndexIfNeeded(nextFrameIndex(), subsamplingLevel, ImageAnimatingState::Yes, options);
141141
}
142142

@@ -157,11 +157,11 @@ bool ImageFrameAnimator::startAnimation(SubsamplingLevel subsamplingLevel, const
157157

158158
void ImageFrameAnimator::advanceAnimation()
159159
{
160-
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Animation at index = %d will be advanced.", __FUNCTION__, this, sourceUTF8(), m_currentFrameIndex);
160+
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Animation at index = %d will be advanced.", __FUNCTION__, this, sourceUTF8().data(), m_currentFrameIndex);
161161

162162
m_currentFrameIndex = nextFrameIndex();
163163
if (m_currentFrameIndex == m_frameCount - 1) {
164-
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Animation loop %d has ended.", __FUNCTION__, this, sourceUTF8(), m_repetitionsComplete);
164+
LOG(Images, "ImageFrameAnimator::%s - %p - url: %s. Animation loop %d has ended.", __FUNCTION__, this, sourceUTF8().data(), m_repetitionsComplete);
165165
++m_repetitionsComplete;
166166
}
167167

@@ -190,7 +190,7 @@ bool ImageFrameAnimator::isAnimationAllowed() const
190190
return m_repetitionCount == RepetitionCountInfinite || m_repetitionsComplete < m_repetitionCount;
191191
}
192192

193-
const char* ImageFrameAnimator::sourceUTF8() const
193+
CString ImageFrameAnimator::sourceUTF8() const
194194
{
195195
return m_source.get()->sourceUTF8();
196196
}

‎Source/WebCore/platform/graphics/ImageFrameAnimator.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class ImageFrameAnimator {
6969

7070
unsigned nextFrameIndex() const { return (m_currentFrameIndex + 1) % m_frameCount; }
7171

72-
const char* sourceUTF8() const;
72+
CString sourceUTF8() const;
7373

7474
ThreadSafeWeakPtr<BitmapImageSource> m_source; // Cannot be null.
7575
unsigned m_frameCount { 0 };

0 commit comments

Comments
 (0)