Skip to content

Fix some UB in signal handling #18

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.

Sign up for GitHub

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

Jump to bottom
Merged
jonrebm merged 3 commits into from
Jan 15, 2026
Merged

Fix some UB in signal handling #18

jonrebm merged 3 commits into from
Jan 15, 2026

Conversation

@a3f
Copy link
Member

@a3f a3f commented Aug 25, 2019

While looking over #15, I noticed some issues with the way microcom handles signals. This fixes what was straight forward to fix. What remains is the access to ios, ios->exit and whatever is called there. With #15 merged, this could be fixed by peppering some volatile around, but it's very unlikely to happen so postponing this for now.

ukleinek
ukleinek previously approved these changes Aug 25, 2019
View reviewed changes
@ukleinek ukleinek self-requested a review August 25, 2019 21:19
@ukleinek
Copy link
Contributor

ukleinek commented Aug 25, 2019

Sorry, github UI problems. I only intended to approve the first commit.

ukleinek
ukleinek reviewed Aug 25, 2019
View reviewed changes
Copy link
Contributor

@ukleinek ukleinek left a comment

Choose a reason for hiding this comment

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

Hmm, I don't particularly like this. I'd like to allow backends calling async-unsafe functions. Freeing ios in global code seems to work, but is a layer violation because the structure was allocated in the backends. I'll think about that, if you want to improve this while I think that would be welcome of course :-)

@ukleinek ukleinek dismissed their stale review August 25, 2019 21:27

added by accident, only intended to approve first commit

@a3f
Copy link
Member Author

a3f commented Aug 25, 2019 •
edited
Loading

Hmm, I don't particularly like this. I'd like to allow backends calling async-unsafe functions.

We could longjmp back into main and exit regularly there. Thinking about it, I like this approach more as well. What do you think?
(EDIT: Thinking some more about this, this is non-nonsensical. If you call an async-signal-unsafe function after a longjmp, you run into UB as well.)

Freeing ios in global code seems to work, but is a layer violation because the structure was allocated in the backends.

Is it? malloc, calloc and strdup are all free'd by the same free. Same goes for open, socket and creat, which are all destructed by close.

@jonrebm
Copy link
Contributor

jonrebm commented Jun 4, 2025

Could you elaborate on the type of issue this resolves? Do we have reproducible crashes, hypothetical undefined behavior or is this just about following best-practices?
Is the issue still present on current master?

@jonrebm jonrebm added the needs-info Needs more information before triage label Jun 4, 2025
jonrebm
jonrebm requested changes Jan 14, 2026
View reviewed changes
Copy link
Contributor

@jonrebm jonrebm left a comment

Choose a reason for hiding this comment

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

Hi @a3f
I just now noticed the overlap this has with #55 and #56

I would like to apply your pr here first, then rebase mine.

@jonrebm jonrebm force-pushed the async-signal-safer branch from 6b025f4 to 7efc70c Compare January 15, 2026 14:34
a3f added 3 commits January 15, 2026 15:36
@a3f @jonrebm
microcom: initialize the struct sigaction before use
37ffd8f 
sigaction(2) reads the sa_mask and sa_flags struct members as well, but
we didn't initialize them so far. We probably didn't run into problems,
because we allocate it when the stack is fresh and full of zeroes.

Fix this by explicitly zeroing the struct and emptying the signal set.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
@a3f @jonrebm
Don't call async-signal-unsafe free in signal handler
70580b8 
We use malloc elsewhere in the code with signals unmasked, but use free
in a signal handler. This could result in a dead lock when terminating
microcom by signal. Simply drop the free as exit is imminent.

The duplication of exit(0) call is ok, as the call site in the signal
handler will be replaced in a follow-up commit.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
@a3f @jonrebm
microcom: make microcom_exit async-signal-safer
90efa26 
Both printf and exit aren't async signal safe, replace them with write
and _Exit respectively.

The access to ios and ios->exit and callees is still illegal, but at
least we don't run risk reentering stdio and causing a deadlock anymore.

As exit flushes stdio buffers as well, we now risk losing unflushed
stdio output. This is acceptable as we aren't buffering the serial
port output and the program is being terminated abnormally anyway.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
@jonrebm jonrebm force-pushed the async-signal-safer branch from 7efc70c to 90efa26 Compare January 15, 2026 14:36
@jonrebm jonrebm changed the base branch from master to main January 15, 2026 14:46
@jonrebm jonrebm merged commit 9a559ba into pengutronix:main Jan 15, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonrebm jonrebm jonrebm requested changes

+1 more reviewer

@ukleinek ukleinek ukleinek left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-info Needs more information before triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@a3f @ukleinek @jonrebm