Skip to content

Commit d10812f

Browse files
mbaludambaluda
committed
Implement SIG30-C
Extract library Signal.qll
1 parent 61c41f3 commit d10812f

File tree

5 files changed

+264
-28
lines changed

5 files changed

+264
-28
lines changed

‎c/cert/src/rules/ERR32-C/DoNotRelyOnIndeterminateValuesOfErrno.ql

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,9 @@
1313
import cpp
1414
import codingstandards.c.cert
1515
import codingstandards.c.Errno
16+
import codingstandards.c.Signal
1617
import semmle.code.cpp.controlflow.Guards
1718

18-
/**
19-
* A call to function `signal`
20-
*/
21-
class SignalCall extends FunctionCall {
22-
SignalCall() { this.getTarget().hasGlobalName("signal") }
23-
}
24-
25-
/**
26-
* A call to `abort` or `_Exit`
27-
*/
28-
class AbortCall extends FunctionCall {
29-
AbortCall() { this.getTarget().hasGlobalName(["abort", "_Exit"]) }
30-
}
3119

3220
/**
3321
* A check on `signal` call return value
@@ -47,22 +35,16 @@ class SignalCheckOperation extends EqualityOperation, GuardCondition {
4735
)
4836
}
4937

50-
BasicBlock getCheckedSuccessor() {
51-
result != errorSuccessor and result = this.getASuccessor()
52-
}
38+
BasicBlock getCheckedSuccessor() { result != errorSuccessor and result = this.getASuccessor() }
5339

5440
BasicBlock getErrorSuccessor() { result = errorSuccessor }
5541
}
5642

5743
/**
5844
* Models signal handlers that call signal() and return
5945
*/
60-
class SignalCallingHandler extends Function {
61-
SignalCall registration;
62-
46+
class SignalCallingHandler extends SignalHandler {
6347
SignalCallingHandler() {
64-
// is a signal handler
65-
this = registration.getArgument(1).(FunctionAccess).getTarget() and
6648
// calls signal() on the handled signal
6749
exists(SignalCall sCall |
6850
sCall.getEnclosingFunction() = this and
@@ -75,8 +57,6 @@ class SignalCallingHandler extends Function {
7557
)
7658
)
7759
}
78-
79-
SignalCall getCall() { result = registration }
8060
}
8161

8262
/**
@@ -100,7 +80,7 @@ where
10080
not isExcluded(errno, Contracts5Package::doNotRelyOnIndeterminateValuesOfErrnoQuery()) and
10181
exists(SignalCallingHandler handler |
10282
// errno read after the handler returns
103-
handler.getCall() = signal
83+
handler.getRegistration() = signal
10484
or
10585
// errno read inside the handler
10686
signal.getEnclosingFunction() = handler

‎c/cert/src/rules/SIG30-C/CallOnlyAsyncSafeFunctionsWithinSignalHandlers.ql

Lines changed: 104 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,109 @@
1111

1212
import cpp
1313
import codingstandards.c.cert
14+
import codingstandards.c.Signal
15+
import semmle.code.cpp.dataflow.DataFlow
1416

15-
from
17+
/**
18+
* Does not an access an external variable except
19+
* to assign a value to a volatile static variable of sig_atomic_t type
20+
*/
21+
class AsyncSafeVariableAccess extends VariableAccess {
22+
AsyncSafeVariableAccess() {
23+
this.getTarget() instanceof StackVariable
24+
or
25+
this.getTarget().getType().hasName("volatile sig_atomic_t") and // TODO search without "volatile"
26+
this.isModified() and
27+
this.getTarget().isVolatile()
28+
}
29+
}
30+
31+
abstract class AsyncSafeFunction extends Function { }
32+
33+
/**
34+
* C standard library ayncronous-safe functions
35+
*/
36+
class CAsyncSafeFunction extends AsyncSafeFunction {
37+
//tion, or the signal function with the first argument equal to the signal number corresponding to the signal that caused the invocation of the handler
38+
CAsyncSafeFunction() { this.hasGlobalName(["abort", "_Exit", "quick_exit", "signal"]) }
39+
}
40+
41+
/**
42+
* POSIX defined ayncronous-safe functions
43+
*/
44+
class PosixAsyncSafeFunction extends AsyncSafeFunction {
45+
PosixAsyncSafeFunction() {
46+
this.hasGlobalName([
47+
"_Exit", "_exit", "abort", "accept", "access", "aio_error", "aio_return", "aio_suspend",
48+
"alarm", "bind", "cfgetispeed", "cfgetospeed", "cfsetispeed", "cfsetospeed", "chdir",
49+
"chmod", "chown", "clock_gettime", "close", "connect", "creat", "dup", "dup2", "execl",
50+
"execle", "execv", "execve", "faccessat", "fchdir", "fchmod", "fchmodat", "fchown",
51+
"fchownat", "fcntl", "fdatasync", "fexecve", "fork", "fstat", "fstatat", "fsync",
52+
"ftruncate", "futimens", "getegid", "geteuid", "getgid", "getgroups", "getpeername",
53+
"getpgrp", "getpid", "getppid", "getsockname", "getsockopt", "getuid", "kill", "link",
54+
"linkat", "listen", "lseek", "lstat", "mkdir", "mkdirat", "mkfifo", "mkfifoat", "mknod",
55+
"mknodat", "open", "openat", "pause", "pipe", "poll", "posix_trace_event", "pselect",
56+
"pthread_kill", "pthread_self", "pthread_sigmask", "raise", "read", "readlink",
57+
"readlinkat", "recv", "recvfrom", "recvmsg", "rename", "renameat", "rmdir", "select",
58+
"sem_post", "send", "sendmsg", "sendto", "setgid", "setpgid", "setsid", "setsockopt",
59+
"setuid", "shutdown", "sigaction", "sigaddset", "sigdelset", "sigemptyset", "sigfillset",
60+
"sigismember", "signal", "sigpause", "sigpending", "sigprocmask", "sigqueue", "sigset",
61+
"sigsuspend", "sleep", "sockatmark", "socket", "socketpair", "stat", "symlink", "symlinkat",
62+
"tcdrain", "tcflow", "tcflush", "tcgetattr", "tcgetpgrp", "tcsendbreak", "tcsetattr",
63+
"tcsetpgrp", "time", "timer_getoverrun", "timer_gettime", "timer_settime", "times", "umask",
64+
"uname", "unlink", "unlinkat", "utime", "utimensat", "utimes", "wait", "waitpid", "write"
65+
])
66+
}
67+
}
68+
69+
/**
70+
* Application defined ayncronous-safe functions
71+
*/
72+
class ApplicationAsyncSafeFunction extends AsyncSafeFunction {
73+
pragma[nomagic]
74+
ApplicationAsyncSafeFunction() {
75+
// Application-defined
76+
this.hasDefinition() and
77+
exists(this.getFile().getRelativePath()) and
78+
// Only references async-safe variables
79+
not exists(VariableAccess va |
80+
this = va.getEnclosingFunction() and not va instanceof AsyncSafeVariableAccess
81+
) and
82+
// Only calls async-safe functions
83+
not exists(Function f | this.calls(f) and not f instanceof AsyncSafeFunction)
84+
}
85+
}
86+
87+
/**
88+
* Call to function `raise` withing a signal handler with mismatching signals
89+
* ```
90+
* void int_handler(int signum) {
91+
* raise(SIGTERM);
92+
* }
93+
* int main(void) {
94+
* signal(SIGINT, int_handler);
95+
* }
96+
* ```
97+
*/
98+
class AsyncUnsafeRaiseCall extends FunctionCall {
99+
AsyncUnsafeRaiseCall() {
100+
this.getTarget().hasGlobalName("raise") and
101+
exists(SignalHandler handler | handler = this.getEnclosingFunction() |
102+
not handler.getRegistration().getArgument(0).getValue() = this.getArgument(0).getValue() and
103+
not DataFlow::localFlow(DataFlow::parameterNode(handler.getParameter(0)),
104+
DataFlow::exprNode(this.getArgument(0)))
105+
)
106+
}
107+
}
108+
109+
from FunctionCall fc, SignalHandler handler
16110
where
17-
not isExcluded(x, SignalHandlersPackage::callOnlyAsyncSafeFunctionsWithinSignalHandlersQuery()) and
18-
select
111+
not isExcluded(fc, SignalHandlersPackage::callOnlyAsyncSafeFunctionsWithinSignalHandlersQuery()) and
112+
handler = fc.getEnclosingFunction() and
113+
(
114+
not fc.getTarget() instanceof AsyncSafeFunction
115+
or
116+
fc instanceof AsyncUnsafeRaiseCall
117+
)
118+
select fc, "Asyncronous-unsafe function calls within a $@ can lead to undefined behavior.",
119+
handler.getRegistration(), "signal handler"

‎c/cert/test/rules/SIG30-C/CallOnlyAsyncSafeFunctionsWithinSignalHandlers.expected

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,4 @@
1-
No expected results have yet been specified
1+
| test.c:11:3:11:18 | call to log_local_unsafe | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:17:7:17:12 | call to signal | signal handler |
2+
| test.c:12:3:12:6 | call to free | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:17:7:17:12 | call to signal | signal handler |
3+
| test.c:48:3:48:9 | call to longjmp | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:52:7:52:12 | call to signal | signal handler |
4+
| test.c:78:7:78:11 | call to raise | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:93:7:93:12 | call to signal | signal handler |

‎c/cert/test/rules/SIG30-C/test.c

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
#include <signal.h>
2+
#include <stdio.h>
3+
#include <stdlib.h>
4+
5+
enum { MAXLINE = 1024 };
6+
char *info = NULL;
7+
8+
void log_local_unsafe(void) { fputs(info, stderr); }
9+
10+
void handler1(int signum) {
11+
log_local_unsafe(); // NON_COMPLIANT
12+
free(info); // NON_COMPLIANT
13+
info = NULL;
14+
}
15+
16+
int f1(void) {
17+
if (signal(SIGINT, handler1) == SIG_ERR) // COMPLIANT
18+
{
19+
//...
20+
}
21+
22+
log_local_unsafe(); // COMPLIANT
23+
24+
return 0;
25+
}
26+
27+
volatile sig_atomic_t eflag = 0;
28+
29+
void handler2(int signum) { eflag = 1; }
30+
31+
int f2(void) {
32+
if (signal(SIGINT, handler2) == SIG_ERR) {
33+
// ...
34+
}
35+
36+
while (!eflag) {
37+
log_local_unsafe(); // COMPLIANT
38+
}
39+
40+
return 0;
41+
}
42+
43+
#include <setjmp.h>
44+
45+
static jmp_buf env;
46+
47+
void handler3(int signum) {
48+
longjmp(env, 1); // NON_COMPLIANT
49+
}
50+
51+
int f3(void) {
52+
if (signal(SIGINT, handler3) == SIG_ERR) {
53+
// ...
54+
}
55+
log_local_unsafe();
56+
57+
return 0;
58+
}
59+
60+
int f4(void) {
61+
if (signal(SIGINT, handler2) == SIG_ERR) {
62+
// ...
63+
}
64+
65+
while (!eflag) {
66+
67+
log_local_unsafe();
68+
}
69+
70+
return 0;
71+
}
72+
73+
void term_handler(int signum) { // SIGTERM handler
74+
}
75+
76+
void int_handler(int signum) {
77+
// SIGINT handler
78+
if (raise(SIGTERM) != 0) { // NON_COMPLIANT
79+
// ...
80+
}
81+
if (raise(SIGINT) != 0) { // COMPLIANT
82+
// ...
83+
}
84+
if (raise(signum) != 0) { // COMPLIANT
85+
// ...
86+
}
87+
}
88+
89+
int f5(void) {
90+
if (signal(SIGTERM, term_handler) == SIG_ERR) {
91+
// ...
92+
}
93+
if (signal(SIGINT, int_handler) == SIG_ERR) {
94+
// ...
95+
}
96+
97+
if (raise(SIGINT) != 0) {
98+
// ...
99+
}
100+
101+
return EXIT_SUCCESS;
102+
}
103+
104+
void int_handler6(int signum) {
105+
106+
term_handler(SIGTERM); // COMPLIANT
107+
}
108+
109+
int f6(void) {
110+
if (signal(SIGTERM, term_handler) == SIG_ERR) {
111+
// ...
112+
}
113+
if (signal(SIGINT, int_handler6) == SIG_ERR) {
114+
// ...
115+
}
116+
117+
if (raise(SIGINT) != 0) // COMPLIANT
118+
{
119+
// ...
120+
}
121+
122+
return EXIT_SUCCESS;
123+
}

‎c/common/src/codingstandards/c/Signal.qll

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import cpp
2+
3+
/**
4+
* A call to function `signal`
5+
*/
6+
class SignalCall extends FunctionCall {
7+
SignalCall() { this.getTarget().hasGlobalName("signal") }
8+
}
9+
10+
/**
11+
* A signal handler
12+
*/
13+
class SignalHandler extends Function {
14+
SignalCall registration;
15+
16+
SignalHandler() {
17+
// is a signal handler
18+
this = registration.getArgument(1).(FunctionAccess).getTarget()
19+
}
20+
21+
SignalCall getRegistration() { result = registration }
22+
}
23+
24+
/**
25+
* A call to `abort` or `_Exit` or `quick_exit`
26+
*/
27+
class AbortCall extends FunctionCall {
28+
AbortCall() { this.getTarget().hasGlobalName(["abort", "_Exit", "quick_exit"]) }
29+
}

0 commit comments

Comments
 (0)