Replace yaml-cpp with libyaml #535

JohnMcPMS · 2020-08-11T20:40:12Z

Also fixes #312 , see below for details.

Change

Due to certain bugs in yaml-cpp, it has not had significant fuzzing applied. Even if those bugs were fixed, it is unknown how many more might exist behind it. In order to avoid chasing those bugs, we are switching to a YAML parser that does have significant fuzz testing coverage, libyaml.

In addition to the use of libyaml, a C++ wrapper is written to create a near identical surface area for use by the existing manifest validation. It also includes encoding detection to enable support for ANSI (assumed Windows-1252, which wasn't supported by yaml-cpp), UTF-8 (with and without BOM), UTF-16 LE/BE (both with and without BOM).

Finally, the validate command now returns a non-zero value when there are issues. If there is a failure during validation, the value returned is 0x8A150029. If there are only warnings, the value returned is 0x8A150028.

Validation

In addition to the regression testing, new tests are added for the validate command and its return values. New tests are also added for the various different text encoding types.

I also used scripts to run validate and show against every manifest in the winget-pkgs repo. They all validate successfully, and the only difference in show presentation that I found was a single file that contained an ANSI © character which was being translated to unknown in the older parser, but is working properly with the new code.

Microsoft Reviewers: Open in CodeFlow

… all winget-pkgs files validate

yao-msft · 2020-08-12T01:27:58Z

src/YamlCppLib/readme.md


 #### Steps used to create the VS project files.

-1. VS Create project from existing code wizard to pint to yaml-cpp directory. Use static library template.
+1. VS Create project from existing code wizard to point to libyaml directory. Use static library template.
 2. Removed code not needed by AppInstallerClient project. Basically keeping only files under src and include.


AppInstallerClient [](start = 30, length = 18)

nit: forgot to update to WinGet here and below #Resolved

yao-msft · 2020-08-12T01:40:03Z

                // Syntax error, yaml-cpp error is stored in FailureInfo

libyaml? #Resolved

Refers to: src/AppInstallerRepositoryCore/Manifest/ManifestValidation.h:89 in 7afa8a5. [](commit_id = 7afa8a5, deletion_comment = False)

yao-msft · 2020-08-12T02:07:20Z

src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj

  <ItemGroup>
    <Content Include="TestData\**">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
  </ItemGroup>

+  <ItemGroup>
+    <None Remove="TestData\Manifests\TestWarningManifest.yaml" />


[](start = 3, length = 62)

looks like not needed #Resolved

yao-msft · 2020-08-12T02:14:45Z

src/AppInstallerCLIE2ETests/TestData/Manifests/TestWarningManifest.yaml

@@ -0,0 +1,12 @@
+Id: TestInvalid.Manifest
+Name: TestInvalidManifest


Invalid [](start = 10, length = 7)

maybe Warning here #Resolved

yao-msft · 2020-08-12T02:47:26Z

src/AppInstallerCommonCore/Public/winget/Yaml.h

+        }
+
+        bool IsDefined() const { return m_type != Type::Invalid; }
+        bool IsNull() const { return m_type == Type::Invalid || m_type == Type::None || (m_type == Type::Scalar && m_scalar.empty()); }


m_type == Type::Invalid [](start = 37, length = 23)

should we allow Invalid as null? #ByDesign

This was to match the behavior of yaml-cpp with respect to what we generate from libyaml.

In reply to: 468973641 [](ancestors = 468973641)

yao-msft · 2020-08-12T02:49:13Z

src/AppInstallerCommonCore/Public/AppInstallerErrors.h

@@ -41,6 +41,17 @@
 #define APPINSTALLER_CLI_ERROR_EXPERIMENTAL_FEATURE_DISABLED    ((HRESULT)0x8A15001D)
 #define APPINSTALLER_CLI_ERROR_MSSTORE_INSTALL_FAILED           ((HRESULT)0x8A15001E)
 #define APPINSTALLER_CLI_ERROR_COMPLETE_INPUT_BAD               ((HRESULT)0x8A15001F)
+#define APPINSTALLER_CLI_ERROR_YAML_INIT_FAILED                 ((HRESULT)0x8A150020)
+#define APPINSTALLER_CLI_ERROR_INVALID_MAPPING_KEY              ((HRESULT)0x8A150021)
+#define APPINSTALLER_CLI_ERROR_DUPLICATE_MAPPING_KEY            ((HRESULT)0x8A150022)


maybe YAML for these 2 as well? #Resolved

yao-msft · 2020-08-12T02:52:27Z

src/AppInstallerRepositoryCore/Manifest/YamlParser.cpp

+        // Detects empty files with a better error.
+        if (!rootNode.IsMap())
+        {
+            THROW_EXCEPTION_MSG(ManifestException(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST), "The manifest does not contain a valid root.");


APPINSTALLER_CLI_ERROR_INVALID_MANIFEST [](start = 50, length = 39)

There's an existing error code for manifest failure APPINSTALLER_CLI_ERROR_MANIFEST_FAILED, should we merge the 2? #ByDesign

That is the more generic parsing error. It is rarely bad to have a more specific error.

In reply to: 468974896 [](ancestors = 468974896)

yao-msft · 2020-08-12T02:57:34Z

src/AppInstallerCommonCore/pch.h

@@ -12,7 +12,8 @@

 #include "TraceLogging.h"

-#include <yaml-cpp/yaml.h>
+#define YAML_DECLARE_STATIC


YAML_DECLARE_STATIC [](start = 8, length = 19)

Is this intended here? #ByDesign

Yes. I could put it in the project, but I don't see the value. Here it is tied to the header it actually is related to.

In reply to: 468976197 [](ancestors = 468976197)

yao-msft · 2020-08-12T03:43:30Z

src/AppInstallerCommonCore/YamlWrapper.cpp

+    {
+        if (init)
+        {
+            if (!yaml_document_initialize(&m_document, NULL, NULL, NULL, 1, 1))


NULL, NULL, NULL, 1, 1 [](start = 55, length = 22)

maybe a comment on what these parameters mean? #Resolved

yao-msft · 2020-08-12T04:09:29Z

src/AppInstallerCommonCore/Yaml.cpp

+        std::ostringstream out;
+        OutputExceptionHeader(out, type);
+
+        out << (problem ? problem : "Unexplained error");


"Unexplained error" [](start = 36, length = 19)

nit; I feel "Unknown error" is less confusing for me #Closed

After a second thought, "unexplained" seems better fit

In reply to: 468993477 [](ancestors = 468993477)

yao-msft · 2020-08-12T04:18:24Z

src/AppInstallerCommonCore/Yaml.cpp

+        THROW_HR_IF(APPINSTALLER_CLI_ERROR_YAML_INVALID_OPERATION, m_type != type);
+    }
+
+    void Node::Require(Type type1, Type type2) const


Require [](start = 15, length = 7)

I think this is never used or not even possible? #Resolved

It was used previously, but I had to change the code. It is possible, it is simply !(type1 || type2).

In reply to: 468995615 [](ancestors = 468995615)

yao-msft · 2020-08-12T04:24:16Z

src/AppInstallerCommonCore/Yaml.cpp

+
+    bool Node::as_dispatch(bool*) const
+    {
+        if (m_scalar == "true")


m_scalar [](start = 12, length = 8)

I think yaml spec recommends case insensitive #Resolved

When I looked at it I didn't see it called out. I can do case insensitive though.

In reply to: 468996962 [](ancestors = 468996962)

yao-msft · 2020-08-12T04:27:16Z

src/AppInstallerCommonCore/Yaml.cpp

+    Emitter::Emitter(Emitter&&) = default;
+    Emitter& Emitter::operator=(Emitter&&) = default;
+
+    Emitter::~Emitter() = default;


curious why not put these in the header?
#ByDesign

I'm using pImpl idioms here to remove the need for the libyaml header to be included in all of the consumers of our Yaml.h. In order to do that, the std::unique_ptr's destructor (and anything that requires the complete type) must only be referenced within the cpp. So these functions are actually "implemented" here, rather than allowing them to be written out in every other TU and be folded by the compiler (or however they actually implement inline header functions).

In reply to: 468997684 [](ancestors = 468997684)

yao-msft

JohnMcPMS added 8 commits August 10, 2020 23:12

Update project to build

47ff837

Builds

faef0c8

Validated existing manifest

2ad5118

All unit tests pass

e94eb6e

Added encoding detection to enable broader support, all test pass and…

5a93f79

… all winget-pkgs files validate

Swap the CG manifest to libyaml and remove yaml-cpp files

26005d7

Add tests for validate command and encoding, fix bug with non-BOM UTF-16

53d1ce4

Handle empty and flat out invalid files better

7afa8a5

JohnMcPMS requested a review from a team as a code owner August 11, 2020 20:40

ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Aug 11, 2020

yao-msft reviewed Aug 12, 2020

View reviewed changes

PR feedback

74c65f0

yao-msft approved these changes Aug 12, 2020

View reviewed changes

JohnMcPMS merged commit dc7ed4d into microsoft:master Aug 12, 2020

JohnMcPMS deleted the replaceyaml2 branch August 12, 2020 18:42

chausner mentioned this pull request Aug 12, 2020

Create SandboxTest script microsoft/winget-pkgs#827

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace yaml-cpp with libyaml #535

Replace yaml-cpp with libyaml #535

JohnMcPMS commented Aug 11, 2020 •

edited by ghost

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft commented Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

JohnMcPMS Aug 12, 2020

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

JohnMcPMS Aug 12, 2020

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

JohnMcPMS Aug 12, 2020

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited

Loading

yao-msft Aug 12, 2020

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

JohnMcPMS Aug 12, 2020

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

JohnMcPMS Aug 12, 2020

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

JohnMcPMS Aug 12, 2020

yao-msft left a comment

		@@ -0,0 +1,12 @@
		Id: TestInvalid.Manifest
		Name: TestInvalidManifest

Replace yaml-cpp with libyaml #535

Replace yaml-cpp with libyaml #535

Conversation

JohnMcPMS commented Aug 11, 2020 • edited by ghost Loading

Change

Validation

Microsoft Reviewers: Open in CodeFlow

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

yao-msft commented Aug 12, 2020 • edited by JohnMcPMS Loading

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

JohnMcPMS Aug 12, 2020

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

JohnMcPMS Aug 12, 2020

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

JohnMcPMS Aug 12, 2020

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

yao-msft Aug 12, 2020

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

JohnMcPMS Aug 12, 2020

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

JohnMcPMS Aug 12, 2020

Choose a reason for hiding this comment

yao-msft Aug 12, 2020 • edited by JohnMcPMS Loading

Choose a reason for hiding this comment

JohnMcPMS Aug 12, 2020

Choose a reason for hiding this comment

yao-msft left a comment

Choose a reason for hiding this comment

JohnMcPMS commented Aug 11, 2020 •

edited by ghost

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft commented Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading

yao-msft Aug 12, 2020 •

edited by JohnMcPMS

Loading