Skip to content

json: Fix issue converting null yaml into json#2706

Merged
htuch merged 2 commits intoenvoyproxy:masterfrom
snowp:empty-yaml
Mar 6, 2018
Merged

json: Fix issue converting null yaml into json#2706
htuch merged 2 commits intoenvoyproxy:masterfrom
snowp:empty-yaml

Conversation

@snowp
Copy link
Copy Markdown
Contributor

@snowp snowp commented Mar 3, 2018

When envoy is launched with an empty yaml file through -c it blows up with:

2018-03-02_15:39:44.80673 [2018-03-02 15:39:44.806][1][critical][assert] external/envoy/source/common/json/json_loader.cc:311] panic: not reached
2018-03-02_15:39:44.80675 [2018-03-02 15:39:44.806][1][critical][backtrace] bazel-out/k8-opt/bin/external/envoy/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:11
4] Caught Aborted, suspect faulting address 0x4e3800000001
2018-03-02_15:39:44.80698 [2018-03-02 15:39:44.806][1][critical][backtrace] bazel-out/k8-opt/bin/external/envoy/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:90
] Backtrace obj</lib/x86_64-linux-gnu/libc.so.6> thr<0> (use tools/stack_decode.py):

This happens because the yaml loader correctly reads the file as a null value but converting this into a rapidjson object fails because the Null type isn't handled. This PR simply adds a switch case that handles this case by returning a null json object.

Risk Level: Low, it makes the json generator more permissive

Testing:
Added unit test that verifies that an empty string can be parsed as yaml and converted to a json string. Also verified that envoy doesn't crash when given an empty yaml file:

➜  envoy git:(empty-yaml) ✗ bazel-bin/source/exe/envoy-static -c empty.yaml
[2018-03-02 23:11:52.558][2664388][info][main] source/server/server.cc:178] initializing epoch 0 (hot restart version=disabled)
[2018-03-02 23:11:52.572][2664388][critical][main] source/server/server.cc:71] error initializing configuration 'empty.yaml': JSON at lines 0-0 does not conform to schema.
 Invalid schema: #
 Schema violation: type
 Offending document key: #
[2018-03-02 23:11:52.572][2664388][info][main] source/server/server.cc:392] exiting

When envoy reads an empty yaml file it parses it as a null Field which
blows up due to the conversion code not handling this case.

This change fixes this by treating a null Field as a null json value.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks. Small nit if you feel like it.

Comment thread test/common/json/json_loader_test.cc Outdated
}

TEST(JsonLoaderTest, YamlAsJsonString) {
{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit if you feel like it: I would remove brace at 449/452. They are not needed as this is the only thing that happens in this test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 5a49ba6 into envoyproxy:master Mar 6, 2018
jpsim added a commit that referenced this pull request Nov 28, 2022
Co-authored-by: jpsim <jpsim@users.noreply.github.com>
Signed-off-by: GitHub Action <noreply@github.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit that referenced this pull request Nov 29, 2022
Co-authored-by: jpsim <jpsim@users.noreply.github.com>
Signed-off-by: GitHub Action <noreply@github.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants