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.

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

[test] Fix Wrapper script to properly handle arguments #4162

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

chantra
Copy link
Contributor

@chantra chantra commented Aug 11, 2022

In #4126 I solved one problem but essentially pushed it somewhere else.

Now the problem is that when multiple arguments are passed, they all end up
being within the same argument because they get wrapped within the double-quotes.

This is pretty much an escape-hell as we keep on passing the same arguments over
and over through functions and then within bash -c.

The reason for the bash -c bit is that it allows us to set some envirtonment variable used in the tests.
Those env var are set to the local environment values.

Instead of going an extra layer of indirection, we can tell sudo to preserve those
specific env var by using the --preserve-env argument this way, we do not have to
re-escape the arguments that are passed within the bash -c quoted arg.

Tests:

Confirm that this was not working before:

$ docker run -ti \
                    --privileged \
                    --network=host \
                    --pid=host \
                    -v $(pwd):/bcc \
                    -v /sys/kernel/debug:/sys/kernel/debug:rw \
                    -v /lib/modules:/lib/modules:ro \
                    -v /usr/src:/usr/src:ro \
                    -e CTEST_OUTPUT_ON_FAILURE=1 \
                    bcc-docker-focal \
                    /bin/bash -c \
                    '/bcc/build/tests/wrapper.sh \
                        c_test_all sudo /bcc/build/tests/cc/test_libbcc -s "test probing running Ruby*"'
test probing running Ruby*": -c: line 0: unexpected EOF while looking for matching `"'
test probing running Ruby*": -c: line 1: syntax error: unexpected end of file
Failed

and is fixed after the patch:

$ docker run -ti \
                    --privileged \
                    --network=host \
                    --pid=host \
                    -v $(pwd):/bcc \
                    -v /sys/kernel/debug:/sys/kernel/debug:rw \
                    -v /lib/modules:/lib/modules:ro \
                    -v /usr/src:/usr/src:ro \
                    -e CTEST_OUTPUT_ON_FAILURE=1 \
                    bcc-docker-focal \
                    /bin/bash -c \
                    '/bcc/build/tests/wrapper.sh \
                        c_test_all sudo /bcc/build/tests/cc/test_libbcc -s "test probing running Ruby*"'

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_libbcc is a Catch v1.4.0 host application.
Run with -? for options

-------------------------------------------------------------------------------
test probing running Ruby process in namespaces
  in separate mount namespace
-------------------------------------------------------------------------------
/bcc/tests/cc/test_usdt_probes.cc:351
...............................................................................

/bcc/tests/cc/test_usdt_probes.cc:367:
PASSED:
  REQUIRE( res.msg() == "" )
with expansion:
  "" == ""

/bcc/tests/cc/test_usdt_probes.cc:368:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:371:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:374:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

-------------------------------------------------------------------------------
test probing running Ruby process in namespaces
  in separate mount namespace and separate PID namespace
-------------------------------------------------------------------------------
/bcc/tests/cc/test_usdt_probes.cc:351
...............................................................................

/bcc/tests/cc/test_usdt_probes.cc:393:
PASSED:
  REQUIRE( res.msg() == "" )
with expansion:
  "" == ""

/bcc/tests/cc/test_usdt_probes.cc:394:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:397:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:400:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:405:
PASSED:
  REQUIRE( bcc_resolve_symname(module.c_str(), "rb_gc_mark", 0x0, ruby_pid, nullptr, &sym) == 0 )
with expansion:
  0 == 0

/bcc/tests/cc/test_usdt_probes.cc:406:
PASSED:
  REQUIRE( std::string(sym.module).find(pid_root, 1) == std::string::npos )
with expansion:
  18446744073709551615 (0xffffffffffffffff)
  ==
  18446744073709551615 (0xffffffffffffffff)

===============================================================================
All tests passed (10 assertions in 1 test case)

In iovisor#4126 I solved one problem but essentially pushed it somewhere else.

Now the problem is that when multiple arguments are passed, they all end up
being within the same argument because they get wrapped within the double-quotes.

This is pretty much an escape-hell as we keep on passing the same arguments over
and over through functions and then within `bash -c`.

The reason for the `bash -c` bit is that it allows us to set some envirtonment variable used in the tests.
Those env var are set to the local environment values.

Instead of going an extra layer of indirection, we can tell `sudo` to preserve those
specific env var by using the `--preserve-env` argument this way, we do not have to
re-escape the arguments that are passed within the `bash -c` quoted arg.

Tests:

Confirm that this was not working before:
```
$ docker run -ti \
                    --privileged \
                    --network=host \
                    --pid=host \
                    -v $(pwd):/bcc \
                    -v /sys/kernel/debug:/sys/kernel/debug:rw \
                    -v /lib/modules:/lib/modules:ro \
                    -v /usr/src:/usr/src:ro \
                    -e CTEST_OUTPUT_ON_FAILURE=1 \
                    bcc-docker-focal \
                    /bin/bash -c \
                    '/bcc/build/tests/wrapper.sh \
                        c_test_all sudo /bcc/build/tests/cc/test_libbcc -s "test probing running Ruby*"'
test probing running Ruby*": -c: line 0: unexpected EOF while looking for matching `"'
test probing running Ruby*": -c: line 1: syntax error: unexpected end of file
Failed

```

and is fixed after the patch:

```
$ docker run -ti \
                    --privileged \
                    --network=host \
                    --pid=host \
                    -v $(pwd):/bcc \
                    -v /sys/kernel/debug:/sys/kernel/debug:rw \
                    -v /lib/modules:/lib/modules:ro \
                    -v /usr/src:/usr/src:ro \
                    -e CTEST_OUTPUT_ON_FAILURE=1 \
                    bcc-docker-focal \
                    /bin/bash -c \
                    '/bcc/build/tests/wrapper.sh \
                        c_test_all sudo /bcc/build/tests/cc/test_libbcc -s "test probing running Ruby*"'

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_libbcc is a Catch v1.4.0 host application.
Run with -? for options

-------------------------------------------------------------------------------
test probing running Ruby process in namespaces
  in separate mount namespace
-------------------------------------------------------------------------------
/bcc/tests/cc/test_usdt_probes.cc:351
...............................................................................

/bcc/tests/cc/test_usdt_probes.cc:367:
PASSED:
  REQUIRE( res.msg() == "" )
with expansion:
  "" == ""

/bcc/tests/cc/test_usdt_probes.cc:368:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:371:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:374:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

-------------------------------------------------------------------------------
test probing running Ruby process in namespaces
  in separate mount namespace and separate PID namespace
-------------------------------------------------------------------------------
/bcc/tests/cc/test_usdt_probes.cc:351
...............................................................................

/bcc/tests/cc/test_usdt_probes.cc:393:
PASSED:
  REQUIRE( res.msg() == "" )
with expansion:
  "" == ""

/bcc/tests/cc/test_usdt_probes.cc:394:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:397:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:400:
PASSED:
  REQUIRE( res.ok() )
with expansion:
  true

/bcc/tests/cc/test_usdt_probes.cc:405:
PASSED:
  REQUIRE( bcc_resolve_symname(module.c_str(), "rb_gc_mark", 0x0, ruby_pid, nullptr, &sym) == 0 )
with expansion:
  0 == 0

/bcc/tests/cc/test_usdt_probes.cc:406:
PASSED:
  REQUIRE( std::string(sym.module).find(pid_root, 1) == std::string::npos )
with expansion:
  18446744073709551615 (0xffffffffffffffff)
  ==
  18446744073709551615 (0xffffffffffffffff)

===============================================================================
All tests passed (10 assertions in 1 test case)
```
@davemarchevsky davemarchevsky merged commit f9aa1e7 into iovisor:master Aug 12, 2022
chantra added a commit to chantra/bcc that referenced this pull request Aug 12, 2022
Python tests are failing to import bcc module:
```
41: Test command: /bcc/build/tests/wrapper.sh "py_test_map_in_map" "sudo" "/bcc/tests/python/test_map_in_map.py"
41: Test timeout computed to be: 10000000
41: Traceback (most recent call last):
41:   File "/bcc/tests/python/test_map_in_map.py", line 9, in <module>
41:     from bcc import BPF
41: ModuleNotFoundError: No module named 'bcc'
41: Failed
41/41 Test iovisor#41: py_test_map_in_map ...............***Failed    0.03 sec
Traceback (most recent call last):
  File "/bcc/tests/python/test_map_in_map.py", line 9, in <module>
    from bcc import BPF
ModuleNotFoundError: No module named 'bcc'
Failed
```

This was caused by iovisor#4162 . `sudo` has a list of env variables that get reset
regardless of what is in `preserve-env` list. See https://github.com/sudo-project/sudo/blob/158facf6d5852ebc420adca5ff06135b18ee57b8/plugins/sudoers/env.c#L138

This patch uses the `env` command to set those env variables within the sudoed context.
@chantra chantra deleted the fix_test_wrapper_escaping branch September 1, 2022 16:07
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.

None yet

2 participants