Skip to content

Commit

Permalink
Revert "Revert "tree-wide: use sizeof on static arrays""
Browse files Browse the repository at this point in the history
This reverts commit 2fb7cf0.

The problem wasn't caused by the reverted commit and was fixed in

commit 0c9b1f8 ("macro: calculate buffer lengths correctly")

The full explanation can be taken from the following irc excerpt from
the #lxc-dev channel:

│19:54:47 brauner | there was a bug in one of the standard macros we used
│19:55:01 brauner | and the changes by INTTYPE_TO_STRLEN() caused the issue to surface
│19:55:03 brauner | which is good
│19:55:16 brauner | i sent a branch and stgraber merged it that fixes it
│19:57:56  Blub\0 | so...
│19:58:31  Blub\0 | still doesn't explain how it was the sizeof() patch
│20:07:14 brauner | Blub\0: so here's the long explanation
│20:07:35 brauner | Blub\0: stgraber bumped pid_max on our jenkins test builders
│20:07:53 brauner | Blub\0: because we're running *a lot* of containers
│20:07:56 brauner | in any case
│20:08:06 brauner | there was a buffer
│20:08:12 brauner | LXC_LSMATTRLEN
│20:08:59 brauner | it used to be
│20:09:03 brauner | -/* /proc/pid-to-str/attr/current = (5 + INTTYPE_TO_STRLEN(pid_t) + 7 + 1) */
│20:09:03 brauner | -#define LXC_LSMATTRLEN (5 + INTTYPE_TO_STRLEN(pid_t) + 7 + 1)
│20:09:14 brauner | which one can see is wrong
│20:09:21 brauner | before the INTTYPE patchset
│20:09:40 brauner | INTTYPE_TO_STRLEN(pid_t) was LXC_NUMSTRLEN64
│20:09:45 brauner | which gave you 21 chars
│20:09:57 brauner | so it accounted for the missing parts
│20:10:03 brauner | because the correct macro should've been
│20:10:17 brauner | +/* /proc/        = 6
│20:10:17 brauner | + *               +
│20:10:17 brauner | + * <pid-as-str>  = INTTYPE_TO_STRLEN(pid_t)
│20:10:17 brauner | + *               +
│20:10:17 brauner | + * /attr/        = 6
│20:10:17 brauner | + *               +
│20:10:17 brauner | + * /current      = 8
│20:10:17 brauner | + *               +
│20:10:17 brauner | + * \0            = 1
│20:10:17 brauner | + */
│20:10:17 brauner | +#define LXC_LSMATTRLEN (6 + INTTYPE_TO_STRLEN(pid_t) + 6 + 8 + 1)
│20:10:24  Blub\0 | still
│20:10:31 brauner | the issue was only seen
│20:10:39 brauner | when the pid number hit a specific maximum
│20:10:50  Blub\0 | the sizeof patch only changed instances of actual char buf[A_FIXED_NUMBER] + snprintf(buf, A_FIXED_NUMBER, ...)
│20:10:54 brauner | aka exceeded the newly shortened buffer
│20:11:42 brauner | your patch was a red herring
│20:12:03  Blub\0 | I guess
│20:12:06 brauner | it didn't cause it
│20:12:14 brauner | it just surfaced at the same time it was merged
│20:12:25  Blub\0 | so we can revert the revert then? :)
│20:12:35 brauner | yes, that was th eplan all along

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
  • Loading branch information
Christian Brauner committed Sep 1, 2018
1 parent b0f3050 commit 979a0d9
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 25 deletions.
2 changes: 1 addition & 1 deletion src/lxc/caps.c
Expand Up @@ -299,7 +299,7 @@ static long int _real_caps_last_cap(void)
char buf[INTTYPE_TO_STRLEN(int)];

again:
n = read(fd, buf, INTTYPE_TO_STRLEN(int));
n = read(fd, buf, sizeof(buf));
if (n < 0 && errno == EINTR) {
goto again;
} else if (n >= 0) {
Expand Down
4 changes: 2 additions & 2 deletions src/lxc/cgroups/cgfsng.c
Expand Up @@ -321,8 +321,8 @@ static char *lxc_cpumask_to_cpulist(uint32_t *bitarr, size_t nbits)
if (!is_set(i, bitarr))
continue;

ret = snprintf(numstr, INTTYPE_TO_STRLEN(size_t), "%zu", i);
if (ret < 0 || (size_t)ret >= INTTYPE_TO_STRLEN(size_t)) {
ret = snprintf(numstr, sizeof(numstr), "%zu", i);
if (ret < 0 || (size_t)ret >= sizeof(numstr)) {
lxc_free_array((void **)cpulist, free);
return NULL;
}
Expand Down
4 changes: 2 additions & 2 deletions src/lxc/log.c
Expand Up @@ -247,8 +247,8 @@ static int lxc_unix_epoch_to_utc(char *buf, size_t bufsize, const struct timespe
seconds = (time->tv_sec - d_in_s - h_in_s - (minutes * 60));

/* Make string from nanoseconds. */
ret = snprintf(nanosec, INTTYPE_TO_STRLEN(int64_t), "%"PRId64, (int64_t)time->tv_nsec);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(int64_t))
ret = snprintf(nanosec, sizeof(nanosec), "%"PRId64, (int64_t)time->tv_nsec);
if (ret < 0 || ret >= sizeof(nanosec))
return -1;

/* Create final timestamp for the log and shorten nanoseconds to 3
Expand Down
4 changes: 2 additions & 2 deletions src/lxc/lxccontainer.c
Expand Up @@ -1039,8 +1039,8 @@ static bool do_lxcapi_start(struct lxc_container *c, int useinit, char * const a
int ret, w;
char pidstr[INTTYPE_TO_STRLEN(int)];

w = snprintf(pidstr, INTTYPE_TO_STRLEN(int), "%d", (int)lxc_raw_getpid());
if (w < 0 || (size_t)w >= INTTYPE_TO_STRLEN(int)) {
w = snprintf(pidstr, sizeof(pidstr), "%d", (int)lxc_raw_getpid());
if (w < 0 || (size_t)w >= sizeof(pidstr)) {
free_init_cmd(init_cmd);
lxc_free_handler(handler);

Expand Down
4 changes: 2 additions & 2 deletions src/lxc/monitor.c
Expand Up @@ -371,8 +371,8 @@ int lxc_monitord_spawn(const char *lxcpath)

close(pipefd[0]);

ret = snprintf(pipefd_str, INTTYPE_TO_STRLEN(int), "%d", pipefd[1]);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(int)) {
ret = snprintf(pipefd_str, sizeof(pipefd_str), "%d", pipefd[1]);
if (ret < 0 || ret >= sizeof(pipefd_str)) {
ERROR("Failed to create pid argument to pass to monitord.");
_exit(EXIT_FAILURE);
}
Expand Down
4 changes: 2 additions & 2 deletions src/lxc/tools/lxc_monitor.c
Expand Up @@ -224,8 +224,8 @@ static int lxc_tool_monitord_spawn(const char *lxcpath)

close(pipefd[0]);

ret = snprintf(pipefd_str, INTTYPE_TO_STRLEN(int), "%d", pipefd[1]);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(int)) {
ret = snprintf(pipefd_str, sizeof(pipefd_str), "%d", pipefd[1]);
if (ret < 0 || ret >= sizeof(pipefd_str)) {
ERROR("Failed to create pid argument to pass to monitord");
_exit(EXIT_FAILURE);
}
Expand Down
4 changes: 2 additions & 2 deletions src/lxc/utils.c
Expand Up @@ -1165,7 +1165,7 @@ int lxc_mount_proc_if_needed(const char *rootfs)
return -1;
}

linklen = readlink(path, link, INTTYPE_TO_STRLEN(pid_t));
linklen = readlink(path, link, sizeof(link));

ret = snprintf(path, MAXPATHLEN, "%s/proc", rootfs);
if (ret < 0 || ret >= MAXPATHLEN) {
Expand All @@ -1179,7 +1179,7 @@ int lxc_mount_proc_if_needed(const char *rootfs)
return -1;

goto domount;
} else if (linklen >= INTTYPE_TO_STRLEN(pid_t)) {
} else if (linklen >= sizeof(link)) {
link[linklen - 1] = '\0';
ERROR("readlink returned truncated content: \"%s\"", link);
return -1;
Expand Down
24 changes: 12 additions & 12 deletions src/tests/lxc-test-utils.c
Expand Up @@ -252,14 +252,14 @@ void test_lxc_safe_uint(void)
lxc_test_assert_abort((-EINVAL == lxc_safe_uint(" -123", &n)));
lxc_test_assert_abort((-EINVAL == lxc_safe_uint("-123", &n)));

ret = snprintf(numstr, INTTYPE_TO_STRLEN(uint64_t), "%" PRIu64, (uint64_t)UINT_MAX);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(uint64_t))
ret = snprintf(numstr, sizeof(numstr), "%" PRIu64, (uint64_t)UINT_MAX);
if (ret < 0 || ret >= sizeof(numstr))
exit(EXIT_FAILURE);

lxc_test_assert_abort((0 == lxc_safe_uint(numstr, &n)) && n == UINT_MAX);

ret = snprintf(numstr, INTTYPE_TO_STRLEN(uint64_t), "%" PRIu64, (uint64_t)UINT_MAX + 1);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(uint64_t))
ret = snprintf(numstr, sizeof(numstr), "%" PRIu64, (uint64_t)UINT_MAX + 1);
if (ret < 0 || ret >= sizeof(numstr))
exit(EXIT_FAILURE);

lxc_test_assert_abort((-ERANGE == lxc_safe_uint(numstr, &n)));
Expand All @@ -285,26 +285,26 @@ void test_lxc_safe_int(void)
signed int n;
char numstr[INTTYPE_TO_STRLEN(uint64_t)];

ret = snprintf(numstr, INTTYPE_TO_STRLEN(uint64_t), "%" PRIu64, (uint64_t)INT_MAX);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(uint64_t))
ret = snprintf(numstr, sizeof(numstr), "%" PRIu64, (uint64_t)INT_MAX);
if (ret < 0 || ret >= sizeof(numstr))
exit(EXIT_FAILURE);

lxc_test_assert_abort((0 == lxc_safe_int(numstr, &n)) && n == INT_MAX);

ret = snprintf(numstr, INTTYPE_TO_STRLEN(uint64_t), "%" PRIu64, (uint64_t)INT_MAX + 1);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(uint64_t))
ret = snprintf(numstr, sizeof(numstr), "%" PRIu64, (uint64_t)INT_MAX + 1);
if (ret < 0 || ret >= sizeof(numstr))
exit(EXIT_FAILURE);

lxc_test_assert_abort((-ERANGE == lxc_safe_int(numstr, &n)));

ret = snprintf(numstr, INTTYPE_TO_STRLEN(int64_t), "%" PRId64, (int64_t)INT_MIN);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(int64_t))
ret = snprintf(numstr, sizeof(numstr), "%" PRId64, (int64_t)INT_MIN);
if (ret < 0 || ret >= sizeof(numstr))
exit(EXIT_FAILURE);

lxc_test_assert_abort((0 == lxc_safe_int(numstr, &n)) && n == INT_MIN);

ret = snprintf(numstr, INTTYPE_TO_STRLEN(int64_t), "%" PRId64, (int64_t)INT_MIN - 1);
if (ret < 0 || ret >= INTTYPE_TO_STRLEN(int64_t))
ret = snprintf(numstr, sizeof(numstr), "%" PRId64, (int64_t)INT_MIN - 1);
if (ret < 0 || ret >= sizeof(numstr))
exit(EXIT_FAILURE);

lxc_test_assert_abort((-ERANGE == lxc_safe_int(numstr, &n)));
Expand Down

0 comments on commit 979a0d9

Please sign in to comment.