bugfix: SA_SIGINFO should not be set when it is using sa_handler#20278
bugfix: SA_SIGINFO should not be set when it is using sa_handler#20278Tommy-LXDAO wants to merge 4 commits intocurl:masterfrom
SA_SIGINFO should not be set when it is using sa_handler#20278Conversation
25921a9 to
a6b3ff1
Compare
Signed-off-by: tommy <tommyskypromax@gmail.com>
|
quote>> The storage occupied by sa_handler and sa_sigaction may overlap, and a conforming application shall not use both simultaneously. https://pubs.opengroup.org/onlinepubs/007904875/functions/sigaction.html Therefore, on some operating systems, |
|
Analysis of PR #20278 at 07fc7abc: Test ../../tests/http/test_03_goaway.py::TestGoAway::test_03_02_h3_goaway failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be exact) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test 1569 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Generated by Testclutch |
|
Wow that documentation is certainly not written with the intent for humans to understand it. Did you actually get a crash somewhere using the unpatched code? |
|
Yes, I am a developer in the OpenHarmony community, primarily responsible for libc. While troubleshooting the issue, I discovered that the stack pointer (PC) was pointing to an invalid address. After adding logs, I found that this code was using the sigaction function incorrectly. However, I regret that I cannot post the specific crash logs. Therefore, I carefully reviewed the code and POSIX guidelines, confirmed the problem, and then contributed the fix directly to the community. |
It means if we remove
It means if we add
It means we set |
This is the problem reproduction step:
#include <stdio.h>
#include <signal.h>
#include <dlfcn.h>
#include <unistd.h>
bool sca_sigaction(int signo, siginfo_t* info, void* context)
{
printf("sca_sigaction executed return false to continue execute other handlers info=%lx context=%lx\n", info, context);
return false;
}
/* The action of the sigchain. */
struct signal_chain_action {
bool (*sca_sigaction)(int, siginfo_t*, void*);
sigset_t sca_mask;
int sca_flags;
};
typedef void (*func_t)(int signo, struct signal_chain_action* sa); // add_special_signal_handler
void temp_sigaction(int signo, siginfo_t *info, void *context)
{
printf("temp_sigaction executed info=%lx context=%lx\n", info, context);
}
void pre_init()
{
func_t add_special_signal_handler = (func_t)dlsym(RTLD_DEFAULT, "add_special_signal_handler");
if (!add_special_signal_handler) {
printf("test failed dlsym add_special_signal_handler not found\n");
exit(0);
}
// print SA_SIGINFO=4 in arm32
printf("SA_SIGINFO=%lx\n", SA_SIGINFO);
struct signal_chain_action action = {0};
action.sca_sigaction = sca_sigaction;
sigemptyset(&action.sca_mask);
int signo = SIGPIPE;
add_special_signal_handler(signo, &action);
printf("Signal handler registered for signal %d\n", signo);
// Simulate the pre-set signal handler for SIGPIPE and use SA_SIGINFO
struct sigaction action2 = {0};
sigaction(SIGPIPE, NULL, &action2);
action2.sa_sigaction = temp_sigaction;
action2.sa_flags |= SA_SIGINFO;
int result = sigaction(SIGPIPE, &action2, NULL);
printf("sigaction registered for signal %d result %d\n", signo, result);
}
void set_sigaction()
{
struct sigaction action = {0};
sigaction(SIGPIPE, NULL, &action);
action.sa_handler = SIG_IGN;
sigaction(SIGPIPE, &action, NULL);
// We print sa_flags here, Now we can see that sa_flags is set with SA_SIGINFO
printf("set_sigaction sa_flags=%lx\n", action.sa_flags); // set_sigaction sa_flags=4
}
void trigger_SIGPIPE()
{
raise(SIGPIPE);
}
int main()
{
pre_init();
set_sigaction();
trigger_SIGPIPE();
return 0;
}
4. This is some additional DFX maintenance information.
```
Device info:OpenHarmony 3.2
Build info:OpenHarmony 6.1.0.28
DeviceDebuggable:Yes
Fingerprint:7828ec281cd85a83f4eddbe86dffc8e10f7a22be68981fa93f795b7114609050
Module name:test_sigaction
Timestamp:2021-04-17 22:44:49.000
Pid:25687
Uid:0
Process name:./test_sigaction
Process life time:1s
Process Memory(kB): 3076(Rss)
Device Memory(kB): Total 2001936, Free 404792, Available 1248536
Reason:Signal:SIGSEGV(SEGV_MAPERR)@0000000000 probably caused by NULL pointer dereference
Fault thread info:
Tid:25687, Name:test_sigaction
#00 pc 00000000 Not mapped
#1 pc 00109d1c /system/lib/ld-musl-arm.so.1(signal_chain_handler+1508)(363ef642612b37eaad558e20c442e4fd)
#2 pc 000727a0 /system/lib/ld-musl-arm.so.1(363ef642612b37eaad558e20c442e4fd)
Registers:
r0:0000000d r1:f78d5c90 r2:f78d5d10 r3:00000001
r4:f7e5ec20 r5:00000004 r6:0000000d r7:000000af
r8:f7d4fb9e r9:f78d5c90 r10:0d003f07
fp:f78d5c88 ip:0000007b sp:f78d5af8 lr:f7e3bd20 pc:00000000
cpsr:60800030
```
It also show that sa_sigaction == 0x1
5.2 Here is the crash stack information
Analyze:
The last code before crash is line 311.
`sig_chains[signo - 1].sig_action.sa_sigaction(signo, siginfo, ucontext_raw);`
If we judge SA_SIGINFO is not included in sa_flags. It should execute signal-catching function directly.
> Why doesn't this code check SIG_IGN first?
Because the POSIX standard specification does not require this step, but rather states that in this case, the signal-catching function should be executed directly.
I think it means executing the signal-catching function directly without checking if it is SIG_IGN. Therefore, sigchain.c conforms to the POSIX standard while curl does not.
I think we should follow the posix standard. So it is a bug. |
|
Thanks! |







SA_SIGINFOshould not be set when it is usingsa_handler. If we setsa_handlerasSIG_IGNandsa_flagswithSA_SIGINFO.The signal handling function will directly treat thissa_handleras a function address, rather than as a signal handling behavior. Then the processPCregister will trap into error address. It will make process crash.