Do not crash when converting YAML to JSON fails by dio · Pull Request #4110 · envoyproxy/envoy

dio · 2018-08-10T18:04:07Z

Description: Do not crash when converting YAML to JSON fails.
Risk Level: Low
Testing: Added
Docs Changes: N/A
Release Notes: N/A
Fixes #3990

Signed-off-by: Dhi Aurrahman dio@tetrate.io

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123

Thanks for fixing.

mattklein123 · 2018-08-10T18:21:56Z

test/common/protobuf/utility_test.cc


+TEST(UtilityTest, YamlLoadFromStringFail) {
+  envoy::config::bootstrap::v2::Bootstrap bootstrap;
+  EXPECT_THROW_WITH_MESSAGE(MessageUtil::loadFromYaml("/home/configs/config.yaml", bootstrap),


I don't think this path is available in CI? Is it possible to do a more direct test case of the actual issue potentially not even loading from a file?

This simulates what is described in the issue #3990, that gave input to --config-yaml a file path string (while this config option expects a valid yaml string instead). #3990 (comment)

This doesn't try to load a file. The --config-yaml perceives this as a string (in this case the string value is a file path).

Ah I see. Got it.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123

Awesome, thank you!

dio · 2018-08-10T23:00:01Z

I'm fixing the approach...

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2018-08-11T00:32:05Z

It seems fine now. I deferred the checking until at the protobuf util.

mattklein123

Thanks, so what was wrong with the previous approach for reference?

mattklein123 · 2018-08-11T16:34:46Z

source/common/protobuf/utility.cc

 void MessageUtil::loadFromYaml(const std::string& yaml, Protobuf::Message& message) {
-  const std::string json = Json::Factory::loadFromYamlString(yaml)->asJsonString();
-  loadFromJson(json, message);
+  const auto& loaded_object = Json::Factory::loadFromYamlString(yaml);


nit: technically this should not be a reference so I would remove &.

mattklein123 · 2018-08-11T16:35:11Z

source/common/protobuf/utility.cc

-  const std::string json = Json::Factory::loadFromYamlString(yaml)->asJsonString();
-  loadFromJson(json, message);
+  const auto& loaded_object = Json::Factory::loadFromYamlString(yaml);
+  // Load to the message if the loaded object has type Object or Array.


s/to the/the

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2018-08-11T22:17:45Z

The previous attempt failed to pass the following unit tests,

[  FAILED  ] JsonLoaderTest.YamlScalar
[  FAILED  ] JsonLoaderTest.YamlObject
[  FAILED  ] JsonLoaderTest.YamlAsJsonString
[  FAILED  ] JsonLoaderTest.BadYamlException

envoy/test/common/json/json_loader_test.cc

Lines 421 to 467 in 5a49ba6

    
           TEST(JsonLoaderTest, YamlScalar) { 
        
             EXPECT_EQ(true, Factory::loadFromYamlString("true")->asBoolean()); 
        
             EXPECT_EQ("true", Factory::loadFromYamlString("\"true\"")->asString()); 
        
             EXPECT_EQ(1, Factory::loadFromYamlString("1")->asInteger()); 
        
             EXPECT_EQ("1", Factory::loadFromYamlString("\"1\"")->asString()); 
        
             EXPECT_DOUBLE_EQ(1.0, Factory::loadFromYamlString("1.0")->asDouble()); 
        
             EXPECT_EQ("1.0", Factory::loadFromYamlString("\"1.0\"")->asString()); 
        
           } 
        
           TEST(JsonLoaderTest, YamlObject) { 
        
             { 
        
               const Json::ObjectSharedPtr json = Json::Factory::loadFromYamlString("[foo, bar]"); 
        
               std::vector<Json::ObjectSharedPtr> output = json->asObjectArray(); 
        
               EXPECT_EQ(2, output.size()); 
        
               EXPECT_EQ("foo", output[0]->asString()); 
        
               EXPECT_EQ("bar", output[1]->asString()); 
        
             } 
        
             { 
        
               const Json::ObjectSharedPtr json = Json::Factory::loadFromYamlString("foo: bar"); 
        
               EXPECT_EQ("bar", json->getString("foo")); 
        
             } 
        
             { 
        
               const Json::ObjectSharedPtr json = Json::Factory::loadFromYamlString("Null"); 
        
               EXPECT_TRUE(json->isNull()); 
        
             } 
        
           } 
        
           TEST(JsonLoaderTest, YamlAsJsonString) { 
        
             const Json::ObjectSharedPtr json = Json::Factory::loadFromYamlString(""); 
        
             EXPECT_EQ(json->asJsonString(), "null"); 
        
           } 
        
           TEST(JsonLoaderTest, BadYamlException) { 
        
             std::string bad_yaml = R"EOF( 
        
           admin: 
        
             access_log_path: /dev/null 
        
             address: 
        
               socket_address: 
        
                 address: {{ ntop_ip_loopback_address }} 
        
                 port_value: 0 
        
           )EOF"; 
        
             EXPECT_THROW_WITH_REGEX(Json::Factory::loadFromYamlString(bad_yaml), EnvoyException, 
        
                                     "bad conversion"); 
        
             EXPECT_THROW_WITHOUT_REGEX(Json::Factory::loadFromYamlString(bad_yaml), EnvoyException, 
        
                                        "Unexpected YAML exception"); 
        
           }

mattklein123

Thanks, LGTM. It would be good if @htuch could take a quick look since he is more familiar w/ the YAML parsing code.

htuch

LGTM, thanks.

htuch · 2018-08-12T12:01:22Z

source/common/protobuf/utility.cc

-  loadFromJson(json, message);
+  const auto loaded_object = Json::Factory::loadFromYamlString(yaml);
+  // Load the message if the loaded object has type Object or Array.
+  if (loaded_object->isObject() || loaded_object->isArray()) {


I guess this works because 100% of the time we use loadFromYaml it's for non-trivial compound types.

* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

Do not crash when converting yaml to json fails

2b42ec8

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio mentioned this pull request Aug 10, 2018

Envoy crashing with NOT_REACHED_GCOVR_EXCL_LINE when loading YAML config using --config-yaml #3990

Closed

mattklein123 reviewed Aug 10, 2018

View reviewed changes

dio added 2 commits August 11, 2018 01:47

Clarify test

7611631

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Add valid loading

2162723

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123 previously approved these changes Aug 10, 2018

View reviewed changes

Defer the checking at protobuf util

2f20370

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio dismissed mattklein123’s stale review via 2f20370 August 10, 2018 23:52

dio added 2 commits August 11, 2018 06:53

Remove superfluous semicolons

ac89352

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Remove TODO

16b8691

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123 reviewed Aug 11, 2018

View reviewed changes

mattklein123 self-assigned this Aug 11, 2018

review comment: remove "&" and fix comment

c2aff43

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123 approved these changes Aug 12, 2018

View reviewed changes

htuch approved these changes Aug 12, 2018

View reviewed changes

htuch merged commit 1381673 into envoyproxy:master Aug 12, 2018

Conversation

dio commented Aug 10, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

dio Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

dio commented Aug 10, 2018

Uh oh!

dio commented Aug 11, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Aug 11, 2018

Choose a reason for hiding this comment

Uh oh!

mattklein123 Aug 11, 2018

Choose a reason for hiding this comment

Uh oh!

dio commented Aug 11, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Aug 12, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dio Aug 10, 2018 •

edited

Loading