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

Print error message on empty BPF #192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qua3k
Copy link

@qua3k qua3k commented Jun 17, 2022

Apologies in advance if this is unwanted/breaks API, but I just found it nice to know when no seccomp filters were installed. I am not a Ruby dev by profession so this may be wrong (but it works on my machine)

@david942j
Copy link
Owner

Thanks for the contribution!

For unknown reasons the test seems hang on CI, will see what happened when the job aborts.

@qua3k
Copy link
Author

qua3k commented Jun 18, 2022

It looks really broken :P

@david942j
Copy link
Owner

david942j commented Jun 19, 2022

Weird, it seems caused by this PR: https://github.com/david942j/seccomp-tools/runs/6949456039?check_suite_focus=true

Randomized with seed 27005
Error: ............................................[ERROR] No seccomp filters installed
Error: The operation was canceled.

The last message before hang was the log

@qua3k did you try running tests with your change locally? You need to do something like

sudo bundle install; sudo bundle exec rake spec
(sudo is required as dump_by_pid needs CAP_SYS_ADMIN)

@qua3k
Copy link
Author

qua3k commented Jun 19, 2022

I've found the issue; at dump_spec.rb#L37 there is a test with a limit of 2, but the program only has one filter installed. This worked fine previously, but the change breaks it and it fails. I'm not sure how you want to proceed with this -- do you want to modify the test?

++ additionally, we could separate ENOENT and EINVAL to print unique messages (no filter at this index/no filters installed for process)

here's the error:

Failures:

  1) SeccompTools::CLI::Dump by pid
  Failure/Error: expect { described_class.new(['-l', '2', '-p', pid.to_s]).handle }.to output(@bpf_disasm).to_stdout

    expected block to output " line  CODE  JT   JF      K\n=================================\n 0000: 0x20 0x00 0x00 0x00000000  A ...17\n 0016: 0x06 0x00 0x00 0x00000000  return KILL\n 0017: 0x06 0x00 0x00 0x7fff0000  return ALLOW\n" to stdout, but output " line  CODE  JT   JF      K\n=================================\n 0000: 0x20 0x00 0x00 0x00000000  A ...return KILL\n 0017: 0x06 0x00 0x00 0x7fff0000  return ALLOW\n[ERROR] No seccomp filters installed\n"

@david942j
Copy link
Owner

Thanks for the investigation!

no filters installed for process

Yes I prefer to differentiate the two cases. Like the test case shows if there is only one filter installed but set limit to 2, the output would look weird. So let's handle the case properly.

This is useful when we're not sure if a filter is installed or not
@qua3k
Copy link
Author

qua3k commented Jun 28, 2022

I fixed the test failures but am unsure how to stop it from printing the "no filter exists at this index" on test runs

Logger.error('No seccomp filters installed')
break
rescue Errno::ENOENT
Logger.error('No filter exists at this index')
Copy link
Owner

Choose a reason for hiding this comment

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

For the test case with limit = 2, it's fine to print this message.

But for the case of limit is negative (which means no limit), we shouldn't print that message as it's expected to just stop when there is no filters, it's not an error. So let's add an if limit.positive? condition at this line.

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @qua3k , do you want to make this change?

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.

2 participants