llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-llvm-support Author: Henrich Lauko (xlauko) <details> <summary>Changes</summary> The YAML remark serializer writes an argument value with more than one newline as a literal block scalar. Block scalars have no escapes, so if the value also contains a control character other than tab or newline, the raw byte ends up in the file. For `"abc\ndef\n\x01ghi"`, PyYAML rejects the document with `unacceptable character #x0001`, while `llvm-remarkutil` accepts it without complaint. Values like that now go through the double-quoted path, which escapes them. Ordinary multi-line values keep the block form. Two more cases also fall back to the quoted form: - A carriage return, since a reader turns a CR inside a block scalar into a line break. - A first line that starts with a space. Readers take the block's indentation from that line, so `" abc\ndef\nghi"` used to produce a document that neither LLVM's parser nor PyYAML could read. The parser had the matching problem on the way back in. `parseStr` took the raw scalar text and stripped only single quotes, so a double-quoted value came back with its quotes and escapes, and `'it''s'` came back as `it''s`. It now decodes scalars with `ScalarNode::getValue`. Unescaped values are copied into an allocator owned by the parser. Plain values still point into the input buffer. A bad escape such as `"a\qb"` now fails the parse instead of silently becoming an empty string. So does a value that decodes to a null character. The bitstream string table stores NUL-terminated strings, so a NUL would shift every later string to the wrong index. `Output::blockScalarString` always wrote `|`, which keeps exactly one trailing newline. Block values without one came back with one, and extra trailing newlines were dropped. It now writes `|-` or `|+` when needed. MIR output does not change, since printed modules and bodies end in exactly one newline. Stacked on #<!-- -->228072, which fixes the use-after-free of block scalar values. Assisted-by: Claude Code (Claude Fable 5.1). --- Full diff: https://github.com/llvm/llvm-project/pull/227654.diff 8 Files Affected: - (modified) llvm/lib/Remarks/YAMLRemarkParser.cpp (+18-18) - (modified) llvm/lib/Remarks/YAMLRemarkParser.h (+2-2) - (modified) llvm/lib/Remarks/YAMLRemarkSerializer.cpp (+15-1) - (modified) llvm/lib/Support/YAMLTraits.cpp (+9) - (added) llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test (+12) - (modified) llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp (+51) - (modified) llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp (+63) - (modified) llvm/unittests/Support/YAMLIOTest.cpp (+7-7) ``````````diff diff --git a/llvm/lib/Remarks/YAMLRemarkParser.cpp b/llvm/lib/Remarks/YAMLRemarkParser.cpp index 405575b3123de..31d255b56d55b 100644 --- a/llvm/lib/Remarks/YAMLRemarkParser.cpp +++ b/llvm/lib/Remarks/YAMLRemarkParser.cpp @@ -265,24 +265,24 @@ Expected<StringRef> YAMLRemarkParser::parseKey(yaml::KeyValueNode &Node) { } Expected<StringRef> YAMLRemarkParser::parseStr(yaml::KeyValueNode &Node) { - auto *Value = dyn_cast_if_present<yaml::ScalarNode>(Node.getValue()); - yaml::BlockScalarNode *ValueBlock; - StringRef Result; - if (!Value) { - // Try to parse the value as a block node. - ValueBlock = dyn_cast_if_present<yaml::BlockScalarNode>(Node.getValue()); - if (!ValueBlock) - return error("expected a value of scalar type.", Node); - // The block value lives in the YAML document, which next() frees before - // returning the remark. - Result = ValueBlock->getValue().copy(Alloc); - } else - Result = Value->getRawValue(); - - Result.consume_front("\'"); - Result.consume_back("\'"); - - return Result; + yaml::Node *Value = Node.getValue(); + // A block value lives in the YAML document, which next() frees before + // returning the remark. + if (auto *Block = dyn_cast_if_present<yaml::BlockScalarNode>(Value)) + return Block->getValue().copy(Alloc); + + auto *Scalar = dyn_cast_if_present<yaml::ScalarNode>(Value); + if (!Scalar) + return error("expected a value of scalar type.", Node); + SmallString<32> Storage; + StringRef Result = Scalar->getValue(Storage); + if (Error E = error()) + return std::move(E); + // The bitstream format cannot hold a null character in a string. + if (Result.contains('\0')) + return error("remark strings cannot contain null characters.", *Scalar); + // getValue only fills Storage when it had to rewrite the value. + return Storage.empty() ? Result : Result.copy(Alloc); } Expected<unsigned> YAMLRemarkParser::parseUnsigned(yaml::KeyValueNode &Node) { diff --git a/llvm/lib/Remarks/YAMLRemarkParser.h b/llvm/lib/Remarks/YAMLRemarkParser.h index e53adf1bd16e8..7de905b2a5bf8 100644 --- a/llvm/lib/Remarks/YAMLRemarkParser.h +++ b/llvm/lib/Remarks/YAMLRemarkParser.h @@ -59,8 +59,8 @@ struct YAMLRemarkParser : public RemarkParser { /// If we parse remark metadata in separate mode, we need to open a new file /// and parse that. std::unique_ptr<MemoryBuffer> SeparateBuf; - /// Storage for block scalar values, which live in the YAML document that - /// next() frees. + /// Storage for values that do not point into the input buffer: unescaped + /// scalars and block scalars. Remarks point into it. BumpPtrAllocator Alloc; YAMLRemarkParser(StringRef Buf); diff --git a/llvm/lib/Remarks/YAMLRemarkSerializer.cpp b/llvm/lib/Remarks/YAMLRemarkSerializer.cpp index 22e297040575c..39107f665e73f 100644 --- a/llvm/lib/Remarks/YAMLRemarkSerializer.cpp +++ b/llvm/lib/Remarks/YAMLRemarkSerializer.cpp @@ -12,6 +12,8 @@ //===----------------------------------------------------------------------===// #include "llvm/Remarks/YAMLRemarkSerializer.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/Remarks/Remark.h" #include "llvm/Support/FileSystem.h" #include <optional> @@ -109,6 +111,18 @@ template <typename T> struct SequenceTraits<ArrayRef<T>> { } }; +/// A literal block scalar has no escapes, so it cannot hold control characters +/// other than tab and line feed. UTF-8 is written as-is. Readers take the +/// indentation from the first non-empty line, so that line cannot start with a +/// space. +static bool canUseBlockScalar(StringRef S) { + if (S.ltrim('\n').starts_with(' ')) + return false; + return all_of(S, [](unsigned char C) { + return isPrint(C) || C == '\t' || C == '\n' || C >= 0x80; + }); +} + /// Implement this as a mapping for now to get proper quotation for the value. template <> struct MappingTraits<Argument> { static void mapping(IO &io, Argument &A) { @@ -116,7 +130,7 @@ template <> struct MappingTraits<Argument> { // NB: A.Key.data() is not necessarily null-terminated, as the StringRef may // be a span into the middle of a string. - if (StringRef(A.Val).count('\n') > 1) { + if (StringRef(A.Val).count('\n') > 1 && canUseBlockScalar(A.Val)) { StringBlockVal S(A.Val); io.mapRequired(A.Key, S); } else { diff --git a/llvm/lib/Support/YAMLTraits.cpp b/llvm/lib/Support/YAMLTraits.cpp index 622806423635f..7c5ab1ef67712 100644 --- a/llvm/lib/Support/YAMLTraits.cpp +++ b/llvm/lib/Support/YAMLTraits.cpp @@ -756,6 +756,15 @@ void Output::blockScalarString(StringRef &S) { if (!StateStack.empty()) newLineCheck(); output(" |"); + // The default chomping keeps exactly one trailing line break, so strip or + // keep them explicitly to preserve the value. + if (!S.empty()) { + size_t LineBreaks = S.size() - S.rtrim('\n').size(); + if (LineBreaks == 0) + output("-"); + else if (LineBreaks > 1) + output("+"); + } unsigned Indent = StateStack.empty() ? 1 : StateStack.size(); diff --git a/llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test b/llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test new file mode 100644 index 0000000000000..6cd09e374f412 --- /dev/null +++ b/llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test @@ -0,0 +1,12 @@ +# The bitstream string table cannot hold a null character, so reject one in +# YAML input instead of writing a corrupt string table. +# RUN: not llvm-remarkutil yaml2bitstream %s -o /dev/null 2>&1 | FileCheck %s +# CHECK: error: {{.*}}remark strings cannot contain null characters. + +--- !Missed +Pass: pass +Name: name +Function: func +Args: + - String: "abc\0def" +... diff --git a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp index 95ca63fc979f9..f74c9958e0ff3 100644 --- a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp @@ -384,6 +384,27 @@ TEST(YAMLRemarks, ParsingWrongArgs) { " - DebugLoc: { File: a, Line: 1, Column: 2 }\n" "", "argument key is missing.")); + // Bad escape in a double-quoted value. + EXPECT_TRUE(parseExpectError("\n" + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "Args:\n" + " - Str: \"a\\qb\"\n" + "", + "Unrecognized escape code")); + // Null character in a value. + EXPECT_TRUE( + parseExpectError("\n" + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "Args:\n" + " - Str: \"a\\0b\"\n" + "", + "remark strings cannot contain null characters.")); } static inline StringRef checkStr(StringRef Str, unsigned ExpectedLen) { @@ -512,6 +533,36 @@ TEST(YAMLRemarks, ContentsBlockScalar) { EXPECT_EQ(checkStr(Remark.Args[0].Val, 8), "abc\ndef\n"); } +TEST(YAMLRemarks, ContentsQuoted) { + StringRef Buf = "--- !Missed\n" + "Pass: pass\n" + "Name: name\n" + "Function: func\n" + "Args:\n" + " - Single: 'it''s'\n" + " - Double: \"abc\\ndef\\n\\x01ghi\"\n" + " - Block: |\n" + " 'abc'\n" + " def\n" + "\n"; + + Expected<std::unique_ptr<remarks::RemarkParser>> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); + + remarks::RemarkParser &Parser = **MaybeParser; + Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = Parser.next(); + EXPECT_FALSE(errorToBool(MaybeRemark.takeError())); + EXPECT_TRUE(*MaybeRemark != nullptr); + + const remarks::Remark &Remark = **MaybeRemark; + ASSERT_EQ(Remark.Args.size(), 3U); + EXPECT_EQ(checkStr(Remark.Args[0].Val, 4), "it's"); + EXPECT_EQ(checkStr(Remark.Args[1].Val, 12), "abc\ndef\n\x01ghi"); + EXPECT_EQ(checkStr(Remark.Args[2].Val, 10), "'abc'\ndef\n"); +} + static inline StringRef checkStr(LLVMRemarkStringRef Str, unsigned ExpectedLen) { const char *StrData = LLVMRemarkStringGetData(Str); diff --git a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp index 974356d9cf30a..1950c01dfb034 100644 --- a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp @@ -193,3 +193,66 @@ TEST(YAMLRemarks, SerializerRemarkStringRefOOBRead) { " DebugLoc: { File: argpath, Line: 6, Column: 7 }\n" "...\n"); } + +TEST(YAMLRemarks, SerializerRemarkMultiLineArg) { + remarks::Remark R; + R.RemarkType = remarks::Type::Missed; + R.PassName = "pass"; + R.RemarkName = "name"; + R.FunctionName = "func"; + R.Args.emplace_back(); + R.Args.back().Key = "block"; + R.Args.back().Val = "abc\ndef\nghi"; + // A literal block scalar cannot hold control characters, so this has to be + // escaped in a double-quoted scalar instead. + R.Args.emplace_back(); + R.Args.back().Key = "control"; + R.Args.back().Val = "abc\ndef\n\x01ghi"; + checkStandalone(remarks::Format::YAML, R, + "--- !Missed\n" + "Pass: pass\n" + "Name: name\n" + "Function: func\n" + "Args:\n" + " - block: |-\n" + " abc\n" + " def\n" + " ghi\n" + " - control: \"abc\\ndef\\n\\x01ghi\"\n" + "...\n"); +} + +TEST(YAMLRemarks, SerializerRemarkRoundTrip) { + // Values must survive a round trip, whether they are written as a block or + // as a quoted scalar. + StringRef Vals[] = {"abc\ndef\n\x01ghi", "abc\r\ndef\r\nghi", + " abc\ndef\nghi", "abc\ndef\nghi", + "abc\ndef\nghi\n", "abc\ndef\nghi\n\n", + "\nabc\ndef\nghi"}; + remarks::Remark R; + R.RemarkType = remarks::Type::Missed; + R.PassName = "pass"; + R.RemarkName = "name"; + R.FunctionName = "func"; + for (StringRef Val : Vals) { + R.Args.emplace_back(); + R.Args.back().Key = "key"; + R.Args.back().Val = Val; + } + + std::string Buf; + raw_string_ostream OS(Buf); + Expected<std::unique_ptr<remarks::RemarkSerializer>> MaybeS = + createRemarkSerializer(remarks::Format::YAML, OS); + ASSERT_FALSE(errorToBool(MaybeS.takeError())); + (*MaybeS)->emit(R); + (*MaybeS)->finalize(); + + Expected<std::unique_ptr<remarks::RemarkParser>> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf); + ASSERT_FALSE(errorToBool(MaybeParser.takeError())); + Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = + (*MaybeParser)->next(); + ASSERT_FALSE(errorToBool(MaybeRemark.takeError())); + EXPECT_EQ(**MaybeRemark, R); +} diff --git a/llvm/unittests/Support/YAMLIOTest.cpp b/llvm/unittests/Support/YAMLIOTest.cpp index 78e19c5ff4f2c..216ee353555fc 100644 --- a/llvm/unittests/Support/YAMLIOTest.cpp +++ b/llvm/unittests/Support/YAMLIOTest.cpp @@ -1211,12 +1211,12 @@ TEST(YAMLIO, TestReadWriteMultilineStringType) { yin >> map2; EXPECT_FALSE(yin.error()); - EXPECT_EQ(map2.name.str, "An Item\n"); - EXPECT_EQ(map2.description.str, "Hello\nWorld\n"); + EXPECT_EQ(map2.name.str, "An Item"); + EXPECT_EQ(map2.description.str, "Hello\nWorld"); EXPECT_EQ(map2.ingredients.str, "SubItem 1\nSub Item 2\n\nSub Item 3\n"); - EXPECT_EQ(map2.recipes.str, "\n\nTest 1\n"); + EXPECT_EQ(map2.recipes.str, "\n\nTest 1\n\n\n"); EXPECT_TRUE(map2.warningLabels.str.empty()); - EXPECT_TRUE(map2.documentation.str.empty()); + EXPECT_EQ(map2.documentation.str, "\n\n"); EXPECT_EQ(map2.price, 350); } } @@ -1248,7 +1248,7 @@ TEST(YAMLIO, TestReadWriteBlockScalarDocuments) { EXPECT_FALSE(yin.error()); EXPECT_EQ(documents2.size(), size_t(1)); - EXPECT_EQ(documents2[0].str, "Hello\nWorld\n"); + EXPECT_EQ(documents2[0].str, "Hello\nWorld"); } } @@ -1268,7 +1268,7 @@ TEST(YAMLIO, TestReadWriteBlockScalarValue) { yin >> doc; EXPECT_FALSE(yin.error()); - EXPECT_EQ(doc.str, "Just a block\nscalar doc\n"); + EXPECT_EQ(doc.str, "Just a block\nscalar doc"); } } @@ -1294,7 +1294,7 @@ TEST(YAMLIO, TestScalarAfterBlockScalar) { Output yout(ostr); yout << v; EXPECT_EQ(output, R"(--- -- block_scalac: | +- block_scalac: |- AA BB scalar: a `````````` </details> https://github.com/llvm/llvm-project/pull/227654 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
