Fix crash while parsing malformed manifest files. #1211
Conversation
| @@ -1105,6 +1105,7 @@ namespace vcpkg | |||
| if (!maybe_manifest_scf) | |||
| { | |||
| print_error_message(maybe_manifest_scf.error()); | |||
| msg::println(); | |||
Alternative to microsoft#1211 Fixes microsoft/vcpkg#33973 I'm not entirely happy with this because it emits extra 'mismatched type' warnings like $.dependencies[0].features[0]: mismatched type: expected a feature in a dependency .
BillyONeal
added
the
requires:discussion
|
I did some experimentation in BillyONeal@68122ce
I'm going to try to ask around tomorrow... |
|
Seeing as |
It was there to prevent using "core" and "default" as feature names because depending of the internal representation that are valid feature names. So this check_exit gives you fail fast errors and enforces invariant so that you don't get non obvious errors.
I changed that to return an nullopt, but then the error message is: |
Yeah I understand what the intent was, I'm just saying that we have competing intents here, and the rest of the JSON deserializer infrastructure appears to prohibit the deserialized types having semantic constraints like this. |
| 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)); }); | ||
| } |
There was a problem hiding this comment.
This should work exactly like Dependency below:
| 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); | |
| }); | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This code would still result in a crash since the constructor of
DependencyRequestedFeaturecheck_exitthat the feature name is not "core" or "default" because there is a memberdefault-featuresthat 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
|
Please let me know what you think of #1219 |
|
Closed in favor of #1219 on which I marked you as a co-author for discovering the 'bones' of the fix. Thanks again! |
Fixes microsoft/vcpkg#33973