snprintf(3) is hard to get right for concatenating strings

[alejandro-colomar]

unit/src/nxt_cgroup.c

Line 44 in f67a01b

len = snprintf(cgprocs + len, NXT_MAX_PATH_LEN - len, "/cgroup.procs");

unit/src/nxt_cgroup.c

Line 45 in f67a01b

if (nxt_slow_path(len >= NXT_MAX_PATH_LEN - len)) {

These two lines are very hard to get right. So much that I found a bug twice (#734 (comment)), in two different rewrites of that code, before it was finally fixed. The underlying problem is not in that code per se, but rather on the misdesign of string copying functions in the C library. There's no function that does the following operation:

"Copy from a (null-terminated) string into a null-terminated string with truncation, report truncation, and allow chaining calls to this function."

Okay, I'm being too picky. Let's loosen the requirement:

"Copy from a (null-terminated) string into a null-terminated string with truncation."

Nothing, the C standard library still doesn't have such a function. Only OpenBSD and a few other systems added strlcpy(3) to cover that hole, but it's a misdesigned function and anyway we can't use it.

Two sets of string copying functions that "copy from a (null-terminated) string into a null-terminated string with truncation" have been invented:

• The Linux kernel added strscpy(9). However, it has issues: You can't chain that function easily. And of course, it's not available in user space.

• OpenBSD added strlcpy(3) and strlcat(3). However, they also have issues: Detecting truncation is complex; they have serious performance issues; and you need to detect truncation after every call. And, we would need to depend on libbsd on GNU/Linux systems; we don't want that.

A better function, which has optimal performance, can be chained, and is difficult to misuse, is the following:

/* This code is in the public domain. */
char *
stpecpy(char *dst, char past_end[0], const char *restrict src)
{
    char *p;

    if (dst == past_end)
        return past_end;

    p = memccpy(dst, src, '\0', past_end - dst);
    if (p != NULL)
        return p - 1;

    /* truncation detected */
    past_end[-1] = '\0';
    return past_end;
}

I'll prepare a pull request that simplifies that code in the Isolation code, making it much simpler, faster, and safer.

[hongzhidao]

Just wondering if there is an issue with the cgroup isolation?

[alejandro-colomar]

Just wondering if there is an issue with the cgroup isolation?

No, the bug was fixed. This is just me wanting to remove complex code.

[ac000]

Can you change the subject line of the issue to remove the unsafe word, unless you can actually point to a problem with them in their current form.

I think you are conflating 'unsafe' with 'hard to get right' which are not really the same thing.

[alejandro-colomar]

Can you change the subject line of the issue to remove the unsafe word, unless you can actually point to a problem with them in their current form.

I think you are conflating 'unsafe' with 'hard to get right' which are not really the same thing.

Done. Sorry for any confusions :)

[hongzhidao]

Feel free to reopen it if any issue is found.

[ac000]

Hmm, not really completed.. where does that come from?, lets see...

[ac000]

Hmm, so we only have two options

"Completed"
and
"Not Planned"

That misses quite a wide gamut of possibilities...

Seems it's a relatively new thing. Can't immediately see a way of changing, adding new ones (like with issue labels...), oh well...