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

Feature:Prevent segmentation fault & update new example:syscall_hijack #366

Closed
wants to merge 1 commit into from

Conversation

Sy0307
Copy link
Contributor

@Sy0307 Sy0307 commented Dec 16, 2024

Description

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Test A
  • Test B

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 53.03%. Comparing base (50a1e15) to head (37c5b01).

Files with missing lines Patch % Lines
runtime/src/bpf_helper.cpp 70.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   52.80%   53.03%   +0.23%     
==========================================
  Files          99      100       +1     
  Lines        9426     9486      +60     
  Branches      956      966      +10     
==========================================
+ Hits         4977     5031      +54     
- Misses       4449     4455       +6     
Flag Coverage Δ
attach tests (uprobe & syscall trace) 97.96% <ø> (ø)
coverage-example-fedora-false-bashreadline-false 13.22% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-bashreadline-true 13.22% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-funclatency-false 10.80% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-false-funclatency-true 10.80% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-false-malloc-true 12.09% <0.00%> (-0.06%) ⬇️
coverage-example-fedora-false-minimal-false 9.77% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-false-minimal-true 9.77% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-false-mountsnoop-true 13.91% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-opensnoop-libbpf-tools-true 14.92% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-opensnoop-true 13.91% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-sigsnoop-true 22.76% <0.00%> (-0.11%) ⬇️
coverage-example-fedora-false-sslsniff-false 14.69% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-sslsniff-true 14.69% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-statsnoop-true 14.92% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-false-syscount-false 9.50% <0.00%> (+1.38%) ⬆️
coverage-example-fedora-false-syscount-true 11.09% <0.00%> (-0.06%) ⬇️
coverage-example-fedora-false-tailcall_minimal-true 9.86% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-false-usdt_minimal-false 10.76% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-false-usdt_minimal-true 10.76% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-bashreadline-false 12.84% <0.00%> (-0.06%) ⬇️
coverage-example-fedora-true-bashreadline-true 12.84% <0.00%> (-0.06%) ⬇️
coverage-example-fedora-true-funclatency-false 10.49% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-funclatency-true 10.49% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-malloc-true 11.75% <0.00%> (-0.06%) ⬇️
coverage-example-fedora-true-minimal-false 9.49% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-minimal-true 9.49% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-mountsnoop-true 13.51% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-true-opensnoop-libbpf-tools-true 14.49% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-true-opensnoop-true 13.51% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-true-sigsnoop-true 22.11% <0.00%> (-0.10%) ⬇️
coverage-example-fedora-true-sslsniff-false 14.27% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-true-sslsniff-true 14.27% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-true-statsnoop-true 14.49% <0.00%> (-0.07%) ⬇️
coverage-example-fedora-true-syscount-false 7.85% <0.00%> (-0.04%) ⬇️
coverage-example-fedora-true-syscount-true 10.78% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-tailcall_minimal-true 9.58% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-usdt_minimal-false 10.45% <0.00%> (-0.05%) ⬇️
coverage-example-fedora-true-usdt_minimal-true 10.45% <0.00%> (-0.05%) ⬇️
coverage-example-ubuntu-false-bashreadline-false 28.61% <ø> (ø)
coverage-example-ubuntu-false-bashreadline-true 28.61% <ø> (ø)
coverage-example-ubuntu-false-funclatency-false 23.42% <ø> (ø)
coverage-example-ubuntu-false-funclatency-true 23.42% <ø> (ø)
coverage-example-ubuntu-false-malloc-true 26.24% <ø> (ø)
coverage-example-ubuntu-false-minimal-false 21.19% <ø> (ø)
coverage-example-ubuntu-false-minimal-true 21.19% <ø> (ø)
coverage-example-ubuntu-false-mountsnoop-true 30.17% <ø> (ø)
coverage-example-ubuntu-false-opensnoop-libbpf-tools-true 32.31% <ø> (ø)
coverage-example-ubuntu-false-opensnoop-true 30.17% <ø> (ø)
coverage-example-ubuntu-false-sigsnoop-true 39.74% <0.00%> (-0.31%) ⬇️
coverage-example-ubuntu-false-sslsniff-false 31.81% <ø> (ø)
coverage-example-ubuntu-false-sslsniff-true 31.81% <ø> (ø)
coverage-example-ubuntu-false-statsnoop-true 32.31% <ø> (ø)
coverage-example-ubuntu-false-syscount-false 17.22% <0.00%> (-0.14%) ⬇️
coverage-example-ubuntu-false-syscount-true 24.06% <ø> (ø)
coverage-example-ubuntu-false-tailcall_minimal-true 21.39% <ø> (ø)
coverage-example-ubuntu-false-usdt_minimal-false 23.33% <ø> (ø)
coverage-example-ubuntu-false-usdt_minimal-true 23.33% <ø> (ø)
coverage-example-ubuntu-true-bashreadline-false 28.61% <ø> (ø)
coverage-example-ubuntu-true-bashreadline-true 28.61% <ø> (ø)
coverage-example-ubuntu-true-funclatency-false 23.42% <ø> (ø)
coverage-example-ubuntu-true-funclatency-true 23.42% <ø> (ø)
coverage-example-ubuntu-true-malloc-true 26.24% <ø> (ø)
coverage-example-ubuntu-true-minimal-false 21.19% <ø> (ø)
coverage-example-ubuntu-true-minimal-true 21.19% <ø> (ø)
coverage-example-ubuntu-true-mountsnoop-true 30.17% <ø> (ø)
coverage-example-ubuntu-true-opensnoop-libbpf-tools-true 32.31% <ø> (ø)
coverage-example-ubuntu-true-opensnoop-true 30.17% <ø> (ø)
coverage-example-ubuntu-true-sigsnoop-true 39.74% <0.00%> (-0.31%) ⬇️
coverage-example-ubuntu-true-sslsniff-false 31.81% <ø> (ø)
coverage-example-ubuntu-true-sslsniff-true 31.81% <ø> (ø)
coverage-example-ubuntu-true-statsnoop-true 32.31% <ø> (ø)
coverage-example-ubuntu-true-syscount-false 17.55% <ø> (ø)
coverage-example-ubuntu-true-syscount-true 24.06% <ø> (ø)
coverage-example-ubuntu-true-tailcall_minimal-true 21.39% <ø> (ø)
coverage-example-ubuntu-true-usdt_minimal-false 23.33% <ø> (ø)
coverage-example-ubuntu-true-usdt_minimal-true 23.33% <ø> (ø)
runtime tests 47.56% <83.87%> (+0.39%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@yunwei37 yunwei37 left a comment

Choose a reason for hiding this comment

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

  1. Please split the example and fix into 2 different pr.
  2. All comments should be in English
  3. Avoid comment code without any explanation.
  4. Please add more tests if possible.
  5. The example should be added into ci for testing.

SEC("tracepoint/syscalls/sys_enter_open")
int tracepoint__syscalls__sys_enter_open(struct trace_event_raw_sys_enter *ctx)
{
// spdlog::info("tracepoint__syscalls__sys_enter_open");
Copy link
Member

Choose a reason for hiding this comment

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

remove these comments

printf("pending until input\n");
int c = getchar();

// 设置要执行的victim程序路径
Copy link
Member

Choose a reason for hiding this comment

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

All comments should be in English

(size_t)(uint32_t)(size));
return 0;
spdlog::debug("probe_read: dst={}, src={}, len={}", dst, ptr, size);
Copy link
Member

Choose a reason for hiding this comment

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

Use trace log here

uint64_t ret = 0;

sa.sa_handler = segv_handler; // set signal handler
sigemptyset(&sa.sa_mask); // clear signal set
Copy link
Member

Choose a reason for hiding this comment

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

Can we further optimize the code here?

uint64_t bpftime_probe_read(uint64_t dst, uint64_t size, uint64_t ptr, uint64_t,
uint64_t)
{
memcpy((void *)(uintptr_t)dst, (void *)(uintptr_t)ptr,
if(size<=0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please format your code with vendored clang format file

return 0;
spdlog::debug("probe_read: dst={}, src={}, len={}", dst, ptr, size);
} else {
spdlog::error("probe_read: failed to read from src={}", ptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use macros to log, so we can control log level at compile time. For example, SPDLOG_ERROR(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yunwei37 suggested we use SPDLOG_TRACE here, I modify it by SPDLOG_TRACE. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

No, for error here, you should use SPDLOG_ERROR.

For debug purposes, change to SPDLOG_DEBUG or SPDLOG_TRACE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it, thanks.

sa.sa_flags = 0;

if (sigaction(SIGSEGV, &sa, &old_sa) == -1) {
perror("sigaction");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should print errors in logs, not to console

@Sy0307
Copy link
Contributor Author

Sy0307 commented Dec 17, 2024

As comment below, I will split it into two individual PRs.

@Sy0307 Sy0307 closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants