What Flow Control?

The assert() macro is not a flow control mechanism

An interesting vulnerability in systemd appears to be a consequence of using the assert() macro as a flow control construct. In the code the authors had the following function:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20

 /* Extract from systemd/src/core/manager.c */
 void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const char *buf, size_t n, FDSet *fds) {
    _cleanup_strv_free_ char **tags = NULL;

    assert(m);
    assert(u);
    assert(buf);
    assert(n > 0);

    tags = strv_split(buf, "\n\r");
    if (!tags) {
        log_oom();
        return;
    }

    if (UNIT_VTABLE(u)->notify_message)
        UNIT_VTABLE(u)->notify_message(u, pid, tags, fds);
    else
        log_unit_debug(u, "Got notification message for unit. Ignoring.");
}

In line 8 of the snippet there is an assertion to check if n > 0, in this case n is the size of a buffer. Since a buffer of size 0 causes systemd to crash on some systems, this is a good check. There are however two serious problems with the code as written:

1. If systemd is compiled with the -DNDEBUG compiler flag, then the assert() macro will not generate any code so the check doesn't happen.
2. If systemd is compiled without the -DNDEBUG</span> flag, then if the assertion fails the program will abort.

For a critical software component, both problems are undesirable. The core of the problem appears to be the use of the assert() macro as a flow control mechanism. If it critically important for the value of n to be checked, then use a real comparison with explicit error handling. An additional item of concern is the use of assert() macros where there is an implicit assumption that NULL == 0 in lines 5 - 7. At a minimum, comparisons of a pointer to NULL should be done explicitly.

Assuming that the conditions implied by the assert() macro usage is correct, the code should be rewritten as:</p>

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36

/* Revised code. */
static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const char *buf, size_t n, FDSet *fds) {
    _cleanup_strv_free_ char **tags = NULL;

    assert(m != NULL);
    if (m == NULL){
        // log as appropriate
        return;
    }
    assert(u != NULL);
    if (u == NULL){
        // log as appropriate
        return;
    }
    assert(buf != NULL);
    if (buf == NULL){
        // log as appropriate
        return;
    }
    assert(n > 0);
    if (n <= 0){
        // log as appropriate
        return;
    }

    tags = strv_split(buf, "\n\r");
    if (!tags) {
        log_oom();
        return;
    }

    if (UNIT_VTABLE(u)->notify_message)
        UNIT_VTABLE(u)->notify_message(u, pid, tags, fds);
    else
        log_unit_debug(u, "Got notification message for unit. Ignoring.");
}

In keeping with good practice, this code will check important conditions regardless of the presence of the -DNDEBUG compiler flag. Additionally, in keeping with the C11 standard ISO/IEC 9899:201x NULL is not assumed to be a zero of any type.