Skip to content

ext/pcntl: adding SIGTRAP handling for freebsd. #14266

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

Closed
wants to merge 2 commits into from

Conversation

devnexen
Copy link
Member

if a restricted file descriptor based syscall by the system had been attempted, a SIGTRAP is raised with the syscall id.

if a restricted file descriptor based syscall by the system had been attempted,
a SIGTRAP is raised with the syscall id.
@nielsdos
Copy link
Member

This could be just me, as I'm not a freebsd user so I don't know the details, but looking at the source code:

https://github.com/freebsd/freebsd-src/blob/c7581d76a1e46a925bba59d3e18394908f65c907/sys/sys/signal.h#L222

It seems that si_sycall and si_trapno will always have the same value because they're overlapping parts of the union?

@nielsdos
Copy link
Member

CI failures are legit

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

One question, other than that it looks good, thanks.

#ifdef SIGTRAP
case SIGTRAP:
# if defined(si_syscall) && defined(__FreeBSD__)
if (siginfo->si_code == TRAP_CAP) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't have FreeBSD, and I have no idea what our lowest supported version is, but is this TRAP_CAP constant old enough that it won't cause build failures on older FreeBSD's that we support?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK TRAP_CAP appeared before the si_syscall shortcut.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Alright, seems fine to me then!

@devnexen devnexen closed this in fe7f699 May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants