Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash while parsing malformed manifest files. #1211

Closed
wants to merge 1 commit into from
Closed

Fix crash while parsing malformed manifest files. #1211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/vcpkg-test/manifests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,25 @@ TEST_CASE ("SourceParagraph manifest default feature missing name", "[manifests]
REQUIRE(!m_pgh.has_value());
}

TEST_CASE ("SourceParagraph manifest default feature reserved name", "[manifests]")
{
auto m_pgh = test_parse_port_manifest(R"json({
"name": "a",
"version-string": "1.0",
"default-features": ["core"]
})json",
PrintErrors::No);
REQUIRE(!m_pgh.has_value());

m_pgh = test_parse_port_manifest(R"json({
"name": "a",
"version-string": "1.0",
"default-features": [{"name": "core", "platform": "!windows"}]
})json",
PrintErrors::No);
REQUIRE(!m_pgh.has_value());
}

TEST_CASE ("SourceParagraph manifest description paragraph", "[manifests]")
{
auto m_pgh = test_parse_port_manifest(R"json({
Expand Down
1 change: 1 addition & 0 deletions src/vcpkg/commands.install.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,7 @@ namespace vcpkg
if (!maybe_manifest_scf)
{
print_error_message(maybe_manifest_scf.error());
msg::println();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I repeated this in #1219 but #1203 resolves it by making print_error_message itself add the extra newline since I found other occurrences.

msg::println(msgExtendedDocumentationAtUrl, msg::url = docs::manifests_url);
Checks::exit_fail(VCPKG_LINE_INFO);
}
Expand Down
9 changes: 7 additions & 2 deletions src/vcpkg/sourceparagraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,10 +510,15 @@ namespace vcpkg
return t;
}

static std::string map_illegal_names(std::string&& name)
{
return Json::IdentifierDeserializer::is_ident(name) ? std::move(name) : "<error>";
}

Optional<DependencyRequestedFeature> visit_string(Json::Reader& r, StringView sv) const override
{
return Json::IdentifierDeserializer::instance.visit_string(r, sv).map(
[](std::string&& name) { return std::move(name); });
[](std::string&& name) { return map_illegal_names(std::move(name)); });
}
Comment on lines +513 to 522
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work exactly like Dependency below:

Suggested change
static std::string map_illegal_names(std::string&& name)
{
return Json::IdentifierDeserializer::is_ident(name) ? std::move(name) : "<error>";
}
Optional<DependencyRequestedFeature> visit_string(Json::Reader& r, StringView sv) const override
{
return Json::IdentifierDeserializer::instance.visit_string(r, sv).map(
[](std::string&& name) { return std::move(name); });
[](std::string&& name) { return map_illegal_names(std::move(name)); });
}
Optional<DependencyRequestedFeature> visit_string(Json::Reader& r, StringView sv) const override
{
return Json::IdentifierDeserializer::instance.visit_string(r, sv).map(
[&r](std::string&& name) {
if (!Json::IdentifierDeserializer::is_ident(name))
r.add_generic_error(type_name(), msg::format(msgInvalidDependency));
return std::move(name);
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would still result in a crash since the constructor of DependencyRequestedFeature check_exit that the feature name is not "core" or "default" because there is a member default-features that must be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would still result in a crash since the constructor of DependencyRequestedFeature check_exit that the feature name is not "core" or "default" because there is a member default-features that must be used.

The point is that things that are deserialized can't have check_exits like that.

Notably, the assert doesn't even succeed in maintaining the invariant that it won't be empty, since:

DependencyRequestedFeature f{"nonempty"};
auto moved = std::move(f);
assert(f.name.empty()); // passes on most standard library implementations


Optional<DependencyRequestedFeature> visit_object(Json::Reader& r, const Json::Object& obj) const override
Expand All @@ -523,7 +528,7 @@ namespace vcpkg
r.required_object_field(type_name(), obj, NAME, name, Json::IdentifierDeserializer::instance);
r.optional_object_field(obj, PLATFORM, platform, PlatformExprDeserializer::instance);
if (name.empty()) return nullopt;
return DependencyRequestedFeature{std::move(name), std::move(platform)};
return DependencyRequestedFeature{map_illegal_names(std::move(name)), std::move(platform)};
}

const static DependencyFeatureDeserializer instance;
Expand Down