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

feat:Prevent segmentation fault in probe_read & probe_write #367

Closed
wants to merge 0 commits into from

Conversation

Sy0307
Copy link
Contributor

@Sy0307 Sy0307 commented Dec 17, 2024

Description

We avoid core dump when accessing an illegal address (such as NULL). Now we will get different error code for such situation.

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

sigemptyset(&sa.sa_mask); // clear signal set
sa.sa_flags = 0;

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

Choose a reason for hiding this comment

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

Do we have any method to optimize these syscalls? For now, each call to bpftime_probe_read may lead to a series syscalls, which are relatively slow.

Maybe we can setup signal handlers only once, and in signal handler, walk stack trace to dertermine whether a call to bpftime_probe_read triggers this signal, and determine to let the call fail or crash.
Will this be too hard?

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 will make a try for this idea later.

Copy link
Member

Choose a reason for hiding this comment

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

}

// restore the origin signal handler
if (sigaction(SIGSEGV, &old_sa, NULL) == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use nullptr in C++, don't use NULL

uint64_t, uint64_t);

// prepare for future use
long bpftime_strncmp(const char *s1, uint64_t s1_sz, const char *s2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you don't need to copy these unused declarations now, I'll sync my fork with your once yours is merged, because my PR still has a long way to go

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