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

openrc: avoid unnecessary malloc inside sig-handler #590

Merged
merged 1 commit into from
Jan 28, 2023

Conversation

N-R-K
Copy link
Contributor

@N-R-K N-R-K commented Jan 27, 2023

malloc (called by xasprintf) is not async-signal-safe. beside, the string here is constant, so there's no need to malloc it all.

eerrorx isn't async-signal-safe either (due to calling fprintf and exit) but consequence of them are typically not as grave as calling malloc while it's internal state is inconsistent.

Bug: #589

@N-R-K
Copy link
Contributor Author

N-R-K commented Jan 27, 2023

Since there's interest in fixing the sig-handler issue - I plan on fixing and submitting the fixes one at a time. But if one large PR is preferred instead - let me know and I can do that too.

src/openrc/rc.c Outdated Show resolved Hide resolved
@vapier
Copy link
Member

vapier commented Jan 28, 2023

agreed that this is def not fixed, and we can't use errorx for a bunch of reasons (fprintf/syslog), but this prob is marginally better by not directly diving into the memory allocator

@thesamesam
Copy link
Contributor

thesamesam commented Jan 28, 2023

Bit frustrated there's no annotations for this class of problem other than IIRC Valgrind being able to detect it at runtime (I don't recall if any of the sanitizers can).

Someone laid out the general recipe at https://news.ycombinator.com/item?id=16754394, not that it went anywhere.

@N-R-K
Copy link
Contributor Author

N-R-K commented Jan 28, 2023

Bit frustrated there's no annotations for this class of problem other than IIRC Valgrind being able to detect it at runtime (I don't recall if any of the sanitizers can).

clang-tidy has a check for this IIRC (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/signal-handler.html) - but for whatever reason it "doesn't seem to be working" on my end (I'm most likely doing something wrong here).

@N-R-K N-R-K force-pushed the rm_asprintf branch 2 times, most recently from 6e0e3f1 to 83bba03 Compare January 28, 2023 15:12
malloc (called by xasprintf) is not async-signal-safe. beside, the
string here is constant, so there's no need to malloc it all.

eerrorx isn't async-signal-safe either (due to calling fprintf and exit)
but consequence of them are _typically_ not as grave as calling malloc
while it's internal state is inconsistent.

Bug: OpenRC#589
@vapier vapier merged commit 459783b into OpenRC:master Jan 28, 2023
@vapier
Copy link
Member

vapier commented Jan 28, 2023

thx bud

@N-R-K N-R-K deleted the rm_asprintf branch January 28, 2023 18:10
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.

3 participants