Skip to content

lua: prevent LuaJIT from panicking.#6994

Merged
lizan merged 1 commit intoenvoyproxy:masterfrom
PiotrSikora:lua_panic
May 31, 2019
Merged

lua: prevent LuaJIT from panicking.#6994
lizan merged 1 commit intoenvoyproxy:masterfrom
PiotrSikora:lua_panic

Conversation

@PiotrSikora
Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora commented May 17, 2019

Migration from the build recipes to foreign_cc rules resulted
in dependencies being built with different compiler flags.

Among other things, those compiler flags were added:

-ffunction-sections -fdata-sections

use of which leads to LuaJIT panicking:

PANIC: unprotected error in call to Lua API

and Envoy subsequently crashing.

Broken in #6168.

Fixes istio/istio#13722.

Signed-off-by: Piotr Sikora piotrsikora@google.com

@PiotrSikora
lua: prevent LuaJIT from panicking.
be375d6 
Migration from the build recipes to foreign_cc rules resulted
in dependencies being built with different compiler flags.

Among other things, those compiler flags were added:

    -ffunction-sections -fdata-sections

use of which leads to LuaJIT panicking:

    PANIC: unprotected error in call to Lua API

and Envoy subsequently crashing.

Broken in envoyproxy#6168.

Fixes istio/istio#13722.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented May 17, 2019

cc @lizan @silentdai @jplevyak @duderino

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented May 17, 2019

/lgtm
I cherry-picked this commit and verified on istio proxy which is built on top of envoy

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented May 17, 2019

@PiotrSikora why doesn't this crash Envoy's integration tests?

@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented May 17, 2019

@mattklein123 it happens once JIT kicks in (it always panicked on the 8th request in my tests), and tries to read something that linker removed thinking it was a dead code (educated guess).

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented May 18, 2019

Interesting. Is there any way to make one of our integration tests run a bunch of requests and crash? That would be pretty nice.

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented May 18, 2019

Adding more details: Not sure if my observation is helping: A clean envoy build without istio proxy build didn't crash at my environment.

@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented May 18, 2019

@silentdai are you building optimized binary (i.e. using bazel build -c opt)? Clean Envoy build definitely crashes for me, since this is how I was replicating this issue.

@mattklein123 it looks that integration tests already have LuaIntegrationTest.SurviveMultipleCalls, but it doesn't trigger this issue. Weird.

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented May 22, 2019

Sorry for the late reply: I am pretty sure I build envoy use ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.release.server_only' and it didn't crash

@lizan
Copy link
Copy Markdown
Member

lizan commented May 24, 2019

@PiotrSikora sorry for being late, how do you want to proceed with this? It would be nice to have a regression test but if it is not trivial then can you file an issue to track that?

@PiotrSikora PiotrSikora mentioned this pull request May 30, 2019
Closed
@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented May 30, 2019

@lizan sorry for the delay.

I couldn't figure out how to create a regression test for this, since the same Lua script that panics when loaded in the binary, works fine in the integration tests.

I created an issue to add it, with the reproduction steps: #7116.

However, the weird thing is that, without this fix, existing Lua tests are failing for me already, when built with -c opt:

$ export PATH=/usr/lib/llvm-8/bin:$PATH
$ export CC=clang
$ export CXX=clang++
$ export ASAN_SYMBOLIZER_PATH=/usr/lib/llvm-8/bin/llvm-symbolizer

$ bazel test -c opt //test/extensions/filters/common/lua/... //test/extensions/filters/http/lua/...

...
INFO: Build completed, 4 tests FAILED, 26 total actions
//test/extensions/filters/http/lua:config_test                           PASSED in 1.6s
//test/extensions/filters/http/lua:lua_integration_test                  PASSED in 2.8s
//test/extensions/filters/common/lua:lua_test                            FAILED in 0.9s
  /ramdisk/piotrsikora/cache/bazel/_bazel_piotrsikora/052b53ce070b0dcea90180b82120ed3b/execroot/envoy/bazel-out/k8-opt/testlogs/test/extensions/filters/common/lua/lua_test/test.log
//test/extensions/filters/common/lua:wrappers_test                       FAILED in 1.4s
  /ramdisk/piotrsikora/cache/bazel/_bazel_piotrsikora/052b53ce070b0dcea90180b82120ed3b/execroot/envoy/bazel-out/k8-opt/testlogs/test/extensions/filters/common/lua/wrappers_test/test.log
//test/extensions/filters/http/lua:lua_filter_test                       FAILED in 3.3s
  /ramdisk/piotrsikora/cache/bazel/_bazel_piotrsikora/052b53ce070b0dcea90180b82120ed3b/execroot/envoy/bazel-out/k8-opt/testlogs/test/extensions/filters/http/lua/lua_filter_test/test.log
//test/extensions/filters/http/lua:wrappers_test                         FAILED in 1.4s
  /ramdisk/piotrsikora/cache/bazel/_bazel_piotrsikora/052b53ce070b0dcea90180b82120ed3b/execroot/envoy/bazel-out/k8-opt/testlogs/test/extensions/filters/http/lua/wrappers_test/test.log

Executed 6 out of 6 tests: 2 tests pass and 4 fail locally.

but for some reason they are all passing in the bazel.release target on CircleCI, so perhaps there is some issue there as well.

With this fix applied, all Lua tests are passing locally:

$ bazel test -c opt //test/extensions/filters/common/lua/... //test/extensions/filters/http/lua/...

...
INFO: Build completed successfully, 20 total actions
//test/extensions/filters/common/lua:lua_test                            PASSED in 0.9s
//test/extensions/filters/common/lua:wrappers_test                       PASSED in 1.4s
//test/extensions/filters/http/lua:config_test                           PASSED in 1.9s
//test/extensions/filters/http/lua:lua_filter_test                       PASSED in 3.3s
//test/extensions/filters/http/lua:lua_integration_test                  PASSED in 2.7s
//test/extensions/filters/http/lua:wrappers_test                         PASSED in 1.4s

Executed 6 out of 6 tests: 6 tests pass.

@lizan lizan merged commit 0e12efc into envoyproxy:master May 31, 2019
PiotrSikora added a commit to istio/envoy that referenced this pull request Jun 3, 2019
@PiotrSikora
lua: prevent LuaJIT from panicking. (envoyproxy#6994)
ad33805 
Migration from the build recipes to foreign_cc rules resulted
in dependencies being built with different compiler flags.

Among other things, those compiler flags were added:

    -ffunction-sections -fdata-sections

use of which leads to LuaJIT panicking:

    PANIC: unprotected error in call to Lua API

and Envoy subsequently crashing.

Broken in envoyproxy#6168.

Fixes istio/istio#13722.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
duderino pushed a commit to istio/proxy that referenced this pull request Jun 4, 2019
Build 1.1.8 part 2 (#2259)
102e286 
* Update istio/envoy sha to pick up envoyproxy/envoy#6994

* Pick up istio/envoy#76
duderino pushed a commit to istio/istio that referenced this pull request Jun 4, 2019
Update istio/proxy for 1.1.8 (#14538)
45b7365 
* Update istio/proxy for 1.1.8

* Import two additional istio/envoy commits.  envoyproxy/envoy#6994 and istio/envoy#76
duderino pushed a commit to istio/envoy that referenced this pull request Jun 12, 2019
Merge pull request #83 from duderino/jblatt_1_2_lua_crash
082a138 
cherry-pick into 1.2:  lua: prevent LuaJIT from panicking. (envoyproxy#6994)
@Elv13 Elv13 mentioned this pull request Jan 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lizan lizan lizan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lizan lizan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Istio-Proxy segfaults on simple Lua scripts

4 participants

@PiotrSikora @lambdai @mattklein123 @lizan