Skip to content

Commit 2b4621e

Browse files
jroelofsmahesh-attarde
jroelofs
authored and
mahesh-attarde
committed
[Remarks] YAMLRemarkSerializer: StringRef-ize apis to avoid out-of-bounds read footguns (llvm#160397)
In llvm#159759, Tobias identified that because YAML IO `mapRequired` expected a null-terminated `const char * Key`, we couldn't legally pass a `StringRef` to it, as that might be length-terminated and not null-terminated. In this patch, we move all of the YAML IO functions that accept a `const char *` over to `StringRef`, avoiding that footgun altogether.
1 parent 01b9472 commit 2b4621e

File tree

3 files changed

+37
-40
lines changed

3 files changed

+37
-40
lines changed

‎llvm/include/llvm/Support/YAMLTraits.h‎

Lines changed: 26 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -705,20 +705,20 @@ class LLVM_ABI IO {
705705
virtual bool mapTag(StringRef Tag, bool Default = false) = 0;
706706
virtual void beginMapping() = 0;
707707
virtual void endMapping() = 0;
708-
virtual bool preflightKey(const char *, bool, bool, bool &, void *&) = 0;
708+
virtual bool preflightKey(StringRef, bool, bool, bool &, void *&) = 0;
709709
virtual void postflightKey(void *) = 0;
710710
virtual std::vector<StringRef> keys() = 0;
711711

712712
virtual void beginFlowMapping() = 0;
713713
virtual void endFlowMapping() = 0;
714714

715715
virtual void beginEnumScalar() = 0;
716-
virtual bool matchEnumScalar(const char *, bool) = 0;
716+
virtual bool matchEnumScalar(StringRef, bool) = 0;
717717
virtual bool matchEnumFallback() = 0;
718718
virtual void endEnumScalar() = 0;
719719

720720
virtual bool beginBitSetScalar(bool &) = 0;
721-
virtual bool bitSetMatch(const char *, bool) = 0;
721+
virtual bool bitSetMatch(StringRef, bool) = 0;
722722
virtual void endBitSetScalar() = 0;
723723

724724
virtual void scalarString(StringRef &, QuotingType) = 0;
@@ -731,16 +731,15 @@ class LLVM_ABI IO {
731731
virtual std::error_code error() = 0;
732732
virtual void setAllowUnknownKeys(bool Allow);
733733

734-
template <typename T>
735-
void enumCase(T &Val, const char *Str, const T ConstVal) {
734+
template <typename T> void enumCase(T &Val, StringRef Str, const T ConstVal) {
736735
if (matchEnumScalar(Str, outputting() && Val == ConstVal)) {
737736
Val = ConstVal;
738737
}
739738
}
740739

741740
// allow anonymous enum values to be used with LLVM_YAML_STRONG_TYPEDEF
742741
template <typename T>
743-
void enumCase(T &Val, const char *Str, const uint32_t ConstVal) {
742+
void enumCase(T &Val, StringRef Str, const uint32_t ConstVal) {
744743
if (matchEnumScalar(Str, outputting() && Val == static_cast<T>(ConstVal))) {
745744
Val = ConstVal;
746745
}
@@ -757,28 +756,28 @@ class LLVM_ABI IO {
757756
}
758757

759758
template <typename T>
760-
void bitSetCase(T &Val, const char *Str, const T ConstVal) {
759+
void bitSetCase(T &Val, StringRef Str, const T ConstVal) {
761760
if (bitSetMatch(Str, outputting() && (Val & ConstVal) == ConstVal)) {
762761
Val = static_cast<T>(Val | ConstVal);
763762
}
764763
}
765764

766765
// allow anonymous enum values to be used with LLVM_YAML_STRONG_TYPEDEF
767766
template <typename T>
768-
void bitSetCase(T &Val, const char *Str, const uint32_t ConstVal) {
767+
void bitSetCase(T &Val, StringRef Str, const uint32_t ConstVal) {
769768
if (bitSetMatch(Str, outputting() && (Val & ConstVal) == ConstVal)) {
770769
Val = static_cast<T>(Val | ConstVal);
771770
}
772771
}
773772

774773
template <typename T>
775-
void maskedBitSetCase(T &Val, const char *Str, T ConstVal, T Mask) {
774+
void maskedBitSetCase(T &Val, StringRef Str, T ConstVal, T Mask) {
776775
if (bitSetMatch(Str, outputting() && (Val & Mask) == ConstVal))
777776
Val = Val | ConstVal;
778777
}
779778

780779
template <typename T>
781-
void maskedBitSetCase(T &Val, const char *Str, uint32_t ConstVal,
780+
void maskedBitSetCase(T &Val, StringRef Str, uint32_t ConstVal,
782781
uint32_t Mask) {
783782
if (bitSetMatch(Str, outputting() && (Val & Mask) == ConstVal))
784783
Val = Val | ConstVal;
@@ -787,29 +786,29 @@ class LLVM_ABI IO {
787786
void *getContext() const;
788787
void setContext(void *);
789788

790-
template <typename T> void mapRequired(const char *Key, T &Val) {
789+
template <typename T> void mapRequired(StringRef Key, T &Val) {
791790
EmptyContext Ctx;
792791
this->processKey(Key, Val, true, Ctx);
793792
}
794793

795794
template <typename T, typename Context>
796-
void mapRequired(const char *Key, T &Val, Context &Ctx) {
795+
void mapRequired(StringRef Key, T &Val, Context &Ctx) {
797796
this->processKey(Key, Val, true, Ctx);
798797
}
799798

800-
template <typename T> void mapOptional(const char *Key, T &Val) {
799+
template <typename T> void mapOptional(StringRef Key, T &Val) {
801800
EmptyContext Ctx;
802801
mapOptionalWithContext(Key, Val, Ctx);
803802
}
804803

805804
template <typename T, typename DefaultT>
806-
void mapOptional(const char *Key, T &Val, const DefaultT &Default) {
805+
void mapOptional(StringRef Key, T &Val, const DefaultT &Default) {
807806
EmptyContext Ctx;
808807
mapOptionalWithContext(Key, Val, Default, Ctx);
809808
}
810809

811810
template <typename T, typename Context>
812-
void mapOptionalWithContext(const char *Key, T &Val, Context &Ctx) {
811+
void mapOptionalWithContext(StringRef Key, T &Val, Context &Ctx) {
813812
if constexpr (has_SequenceTraits<T>::value) {
814813
// omit key/value instead of outputting empty sequence
815814
if (this->canElideEmptySequence() && Val.begin() == Val.end())
@@ -819,14 +818,14 @@ class LLVM_ABI IO {
819818
}
820819

821820
template <typename T, typename Context>
822-
void mapOptionalWithContext(const char *Key, std::optional<T> &Val,
821+
void mapOptionalWithContext(StringRef Key, std::optional<T> &Val,
823822
Context &Ctx) {
824823
this->processKeyWithDefault(Key, Val, std::optional<T>(),
825824
/*Required=*/false, Ctx);
826825
}
827826

828827
template <typename T, typename Context, typename DefaultT>
829-
void mapOptionalWithContext(const char *Key, T &Val, const DefaultT &Default,
828+
void mapOptionalWithContext(StringRef Key, T &Val, const DefaultT &Default,
830829
Context &Ctx) {
831830
static_assert(std::is_convertible<DefaultT, T>::value,
832831
"Default type must be implicitly convertible to value type!");
@@ -836,12 +835,12 @@ class LLVM_ABI IO {
836835

837836
private:
838837
template <typename T, typename Context>
839-
void processKeyWithDefault(const char *Key, std::optional<T> &Val,
838+
void processKeyWithDefault(StringRef Key, std::optional<T> &Val,
840839
const std::optional<T> &DefaultValue,
841840
bool Required, Context &Ctx);
842841

843842
template <typename T, typename Context>
844-
void processKeyWithDefault(const char *Key, T &Val, const T &DefaultValue,
843+
void processKeyWithDefault(StringRef Key, T &Val, const T &DefaultValue,
845844
bool Required, Context &Ctx) {
846845
void *SaveInfo;
847846
bool UseDefault;
@@ -857,7 +856,7 @@ class LLVM_ABI IO {
857856
}
858857

859858
template <typename T, typename Context>
860-
void processKey(const char *Key, T &Val, bool Required, Context &Ctx) {
859+
void processKey(StringRef Key, T &Val, bool Required, Context &Ctx) {
861860
void *SaveInfo;
862861
bool UseDefault;
863862
if (this->preflightKey(Key, Required, false, UseDefault, SaveInfo)) {
@@ -1332,7 +1331,7 @@ class LLVM_ABI Input : public IO {
13321331
bool mapTag(StringRef, bool) override;
13331332
void beginMapping() override;
13341333
void endMapping() override;
1335-
bool preflightKey(const char *, bool, bool, bool &, void *&) override;
1334+
bool preflightKey(StringRef Key, bool, bool, bool &, void *&) override;
13361335
void postflightKey(void *) override;
13371336
std::vector<StringRef> keys() override;
13381337
void beginFlowMapping() override;
@@ -1346,11 +1345,11 @@ class LLVM_ABI Input : public IO {
13461345
void postflightFlowElement(void *) override;
13471346
void endFlowSequence() override;
13481347
void beginEnumScalar() override;
1349-
bool matchEnumScalar(const char *, bool) override;
1348+
bool matchEnumScalar(StringRef, bool) override;
13501349
bool matchEnumFallback() override;
13511350
void endEnumScalar() override;
13521351
bool beginBitSetScalar(bool &) override;
1353-
bool bitSetMatch(const char *, bool) override;
1352+
bool bitSetMatch(StringRef, bool) override;
13541353
void endBitSetScalar() override;
13551354
void scalarString(StringRef &, QuotingType) override;
13561355
void blockScalarString(StringRef &) override;
@@ -1483,7 +1482,7 @@ class LLVM_ABI Output : public IO {
14831482
bool mapTag(StringRef, bool) override;
14841483
void beginMapping() override;
14851484
void endMapping() override;
1486-
bool preflightKey(const char *key, bool, bool, bool &, void *&) override;
1485+
bool preflightKey(StringRef Key, bool, bool, bool &, void *&) override;
14871486
void postflightKey(void *) override;
14881487
std::vector<StringRef> keys() override;
14891488
void beginFlowMapping() override;
@@ -1497,11 +1496,11 @@ class LLVM_ABI Output : public IO {
14971496
void postflightFlowElement(void *) override;
14981497
void endFlowSequence() override;
14991498
void beginEnumScalar() override;
1500-
bool matchEnumScalar(const char *, bool) override;
1499+
bool matchEnumScalar(StringRef, bool) override;
15011500
bool matchEnumFallback() override;
15021501
void endEnumScalar() override;
15031502
bool beginBitSetScalar(bool &) override;
1504-
bool bitSetMatch(const char *, bool) override;
1503+
bool bitSetMatch(StringRef, bool) override;
15051504
void endBitSetScalar() override;
15061505
void scalarString(StringRef &, QuotingType) override;
15071506
void blockScalarString(StringRef &) override;
@@ -1558,7 +1557,7 @@ class LLVM_ABI Output : public IO {
15581557
};
15591558

15601559
template <typename T, typename Context>
1561-
void IO::processKeyWithDefault(const char *Key, std::optional<T> &Val,
1560+
void IO::processKeyWithDefault(StringRef Key, std::optional<T> &Val,
15621561
const std::optional<T> &DefaultValue,
15631562
bool Required, Context &Ctx) {
15641563
assert(!DefaultValue && "std::optional<T> shouldn't have a value!");

‎llvm/lib/Remarks/YAMLRemarkSerializer.cpp‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,15 +114,13 @@ template <> struct MappingTraits<Argument> {
114114
static void mapping(IO &io, Argument &A) {
115115
assert(io.outputting() && "input not yet implemented");
116116

117-
// A.Key.data() is not necessarily null-terminated, so we must make a copy,
118-
// otherwise we potentially read out of bounds.
119-
// FIXME: Add support for StringRef Keys in YAML IO.
120-
std::string Key(A.Key);
117+
// NB: A.Key.data() is not necessarily null-terminated, as the StringRef may
118+
// be a span into the middle of a string.
121119
if (StringRef(A.Val).count('\n') > 1) {
122120
StringBlockVal S(A.Val);
123-
io.mapRequired(Key.c_str(), S);
121+
io.mapRequired(A.Key, S);
124122
} else {
125-
io.mapRequired(Key.c_str(), A.Val);
123+
io.mapRequired(A.Key, A.Val);
126124
}
127125
io.mapOptional("DebugLoc", A.Loc);
128126
}

‎llvm/lib/Support/YAMLTraits.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ std::vector<StringRef> Input::keys() {
144144
return Ret;
145145
}
146146

147-
bool Input::preflightKey(const char *Key, bool Required, bool, bool &UseDefault,
147+
bool Input::preflightKey(StringRef Key, bool Required, bool, bool &UseDefault,
148148
void *&SaveInfo) {
149149
UseDefault = false;
150150
if (EC)
@@ -168,7 +168,7 @@ bool Input::preflightKey(const char *Key, bool Required, bool, bool &UseDefault,
168168
UseDefault = true;
169169
return false;
170170
}
171-
MN->ValidKeys.push_back(Key);
171+
MN->ValidKeys.push_back(Key.str());
172172
HNode *Value = MN->Mapping[Key].first;
173173
if (!Value) {
174174
if (Required)
@@ -266,7 +266,7 @@ void Input::beginEnumScalar() {
266266
ScalarMatchFound = false;
267267
}
268268

269-
bool Input::matchEnumScalar(const char *Str, bool) {
269+
bool Input::matchEnumScalar(StringRef Str, bool) {
270270
if (ScalarMatchFound)
271271
return false;
272272
if (ScalarHNode *SN = dyn_cast<ScalarHNode>(CurrentNode)) {
@@ -302,7 +302,7 @@ bool Input::beginBitSetScalar(bool &DoClear) {
302302
return true;
303303
}
304304

305-
bool Input::bitSetMatch(const char *Str, bool) {
305+
bool Input::bitSetMatch(StringRef Str, bool) {
306306
if (EC)
307307
return false;
308308
if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
@@ -541,7 +541,7 @@ std::vector<StringRef> Output::keys() {
541541
report_fatal_error("invalid call");
542542
}
543543

544-
bool Output::preflightKey(const char *Key, bool Required, bool SameAsDefault,
544+
bool Output::preflightKey(StringRef Key, bool Required, bool SameAsDefault,
545545
bool &UseDefault, void *&SaveInfo) {
546546
UseDefault = false;
547547
SaveInfo = nullptr;
@@ -666,7 +666,7 @@ void Output::beginEnumScalar() {
666666
EnumerationMatchFound = false;
667667
}
668668

669-
bool Output::matchEnumScalar(const char *Str, bool Match) {
669+
bool Output::matchEnumScalar(StringRef Str, bool Match) {
670670
if (Match && !EnumerationMatchFound) {
671671
newLineCheck();
672672
outputUpToEndOfLine(Str);
@@ -695,7 +695,7 @@ bool Output::beginBitSetScalar(bool &DoClear) {
695695
return true;
696696
}
697697

698-
bool Output::bitSetMatch(const char *Str, bool Matches) {
698+
bool Output::bitSetMatch(StringRef Str, bool Matches) {
699699
if (Matches) {
700700
if (NeedBitValueComma)
701701
output(", ");

0 commit comments

Comments
 (0)