Skip to content

nsexec.c: don't write logs that are to be discarded #3201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kolyshkin merged 5 commits into from
Sep 13, 2021
Merged

nsexec.c: don't write logs that are to be discarded #3201

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a3577c
utils_linux: simplify newProcess
kolyshkin Jun 30, 2021
6c4a3b1
runc init: pass _LIBCONTAINER_LOGLEVEL as int
kolyshkin Jul 26, 2021
d2f49d4
libct/nsenter/nsexec.c: improve bail
kolyshkin Jul 28, 2021
d5ffe83
libct/nsenter/nsexec.c: factor out getenv_int
kolyshkin Jul 28, 2021
f1b703f
libct/nsenter/nsexec.c: honor _LIBCONTAINER_LOGLEVEL
kolyshkin Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions libcontainer/nsenter/nsexec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,21 @@ struct nlconfig_t {
size_t gidmappath_len;
};

#define PANIC "panic"
#define FATAL "fatal"
#define ERROR "error"
#define WARNING "warning"
#define INFO "info"
#define DEBUG "debug"
/*
* Log levels are the same as in logrus.
*/
#define PANIC 0
#define FATAL 1
#define ERROR 2
#define WARNING 3
#define INFO 4
#define DEBUG 5
#define TRACE 6

static const char *level_str[] = { "panic", "fatal", "error", "warning", "info", "debug", "trace" };

static int logfd = -1;
static int loglevel = DEBUG;

/*
* List of netlink message types sent to us as part of bootstrapping the init.
Expand Down Expand Up @@ -134,13 +141,13 @@ int setns(int fd, int nstype)
}
#endif

static void write_log(const char *level, const char *format, ...)
static void write_log(int level, const char *format, ...)
{
char *message = NULL, *stage = NULL, *json = NULL;
va_list args;
int ret;

if (logfd < 0 || level == NULL)
if (logfd < 0 || level > loglevel)
goto out;

va_start(args, format);
Expand All @@ -162,7 +169,8 @@ static void write_log(const char *level, const char *format, ...)
goto out;
}

ret = asprintf(&json, "{\"level\":\"%s\", \"msg\": \"%s[%d]: %s\"}\n", level, stage, getpid(), message);
ret = asprintf(&json, "{\"level\":\"%s\", \"msg\": \"%s[%d]: %s\"}\n",
level_str[level], stage, getpid(), message);
if (ret < 0) {
json = NULL;
goto out;
Expand Down Expand Up @@ -400,16 +408,18 @@ static int getenv_int(const char *name)
if (val == endptr || *endptr != '\0')
bail("unable to parse %s=%s", name, val);
/*
* Sanity check: this must be a non-negative number.
*/
if (ret < 0)
* Sanity check: this must be a small non-negative number.
* Practically, we pass two fds (3 and 4) and a log level,
* for which the maximum is 6 (TRACE).
* */
if (ret < 0 || ret > TRACE)
bail("bad value for %s=%s (%d)", name, val, ret);

return ret;
}

/*
* Sets up logging by getting log fd from the environment,
* Sets up logging by getting log fd and log level from the environment,
* if available.
*/
static void setup_logpipe(void)
Expand All @@ -422,6 +432,11 @@ static void setup_logpipe(void)
return;
}
logfd = i;

i = getenv_int("_LIBCONTAINER_LOGLEVEL");
if (i < 0)
return;
Comment on lines +437 to +438
Copy link
Member

@thaJeztah thaJeztah Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a convention on wether or not to use curly braces for these? I realise it's common in C to omit them, but (e.g.) a couple of lines up we do use curly braces (given, in that case because of the comment).

Wondering if we need to have a convention (also as a safeguard), at least for "new" code.

Copy link
Contributor Author

@kolyshkin kolyshkin Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

In Go, obligatory curly braces (and, ultimately, gofmt) were introduced in order to minimize such discussions.

In C, curly braces are optional if the body has a single statement, which sometimes leads to bugs like this

zero();

if (val > max) /* Fixup val because something something */
	val = max;
	one();

two();

It's kinda hard to spot that one() is called unconditionally here (especially if the code is lengthier than in this example).

Fortunately, here we use indent (which is a poor man's gofmt for C), which will show a warning about wrong indentation in CI. It won't prevent a bug, but at least it will be obvious.

This is why I think both styles (with and without braces) are OK in here.

Copy link
Member

@thaJeztah thaJeztah Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm ok with this (especially because it's already for quite some code), just have to think about Apple's infamous "goto fail" security issue every time I see code without the curly braces 😂 https://www.imperialviolet.org/2014/02/22/applebug.html

Copy link
Contributor Author

@kolyshkin kolyshkin Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we have some linter to take care of it (I mean real linter, not the indentation checker, although it seems to alleviate the issue so it is visible to the naked eye).

loglevel = i;
}

/* Returns the clone(2) flag for a namespace, given the name of a namespace. */
Expand Down