Skip to content
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

Add logger for usr.bin/mdo/mdo.c: a logger like sudo. #1512

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add logger for usr.bin/mdo/mdo.c: a logger like sudo. #1512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 55 additions & 6 deletions usr.bin/mdo/mdo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <syslog.h>
#include <unistd.h>

static void
Expand All @@ -22,10 +23,29 @@
exit(EXIT_FAILURE);
}

static char*
join_strings(char **arr, int size, const char *delimiter)
Copy link
Member

Choose a reason for hiding this comment

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

Since this function is only used once, it can be folded into main()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the separation would make the code easier to read and without loss the original code logic?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really have strong opinion here, it's more of a personal preference.

However, this approach also matches existing patterns of other similar situations: from readability point of view, while separating a more complex function is generally helpful (e.g. this program would do some complicated operations A, B, and C in order, and these operations are implemented as separate functions), it can actually hurt readability to overly wrap simple operations. My personal rule of thumb is to not extract an operation into a function if a) it's only used once, and b) the function itself plus the caller's code block would fit one page or screen (about 20-25 lines) worth of code block.

Note that there are other similar situations where it's done in place instead of in a separate function, for example in apply(1) and newsyslog(8).

{
int total_length = 0, delimiter_length = strlen(delimiter);
Copy link
Contributor

@rilysh rilysh Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are several branches in these loops, also I think it's a good idea to print the function name where it fails.
As suggested by delphij, you can also use sbuf(9) so that you can probably eliminate this wrapper function.

Slightly changed

static char *join_strings2(char **arr, int nlen, const char *delim)
{
	char *buf;
	size_t dlen, tlen, i;

	tlen = 0;
	dlen = strlen(delim);
	for (i = 0; i < nlen; i++)
		tlen += strlen(arr[i]) + dlen;

	if ((buf = malloc(tlen)) == NULL)
		err(1, "malloc()");

	*buf = '\0';
        strcpy(buf, arr[0]);
	for (i = 1; i < nlen; i++) {
		strcpy(buf + strlen(buf), delim);
		strcpy(buf + strlen(buf), arr[i]);
	}

	return (buf);
}

for (int i = 0; i < size; i++)
total_length += strlen(arr[i]) + (i < size - 1 ? delimiter_length : 0);

Check warning on line 31 in usr.bin/mdo/mdo.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
char *result = malloc(total_length + 1);
if (!result)
exit(1);
result[0] = '\0';
for (int i = 0; i < size; i++) {
strcat(result, arr[i]);
if (i < size - 1)
strcat(result, delimiter);
}
return (result);
}

int
main(int argc, char **argv)
{
struct passwd *pw;
struct passwd *pw = getpwuid(getuid());
char original_username[33];
const char *username = "root";
bool uidonly = false;
int ch;
Expand All @@ -45,32 +65,61 @@
argc -= optind;
argv += optind;

openlog("mdo", LOG_PID | LOG_CONS, LOG_USER);
strcpy(original_username, pw->pw_name);

if ((pw = getpwnam(username)) == NULL) {
if (strspn(username, "0123456789") == strlen(username)) {
const char *errp = NULL;
uid_t uid = strtonum(username, 0, UID_MAX, &errp);
if (errp != NULL)

if (errp != NULL) {
syslog(LOG_ERR, "Failed due to: %s", errp);
err(EXIT_FAILURE, "%s", errp);
}
pw = getpwuid(uid);
}
if (pw == NULL)
if (pw == NULL) {
syslog(LOG_AUTH | LOG_WARNING, "invalid username: %s", username);

Check warning on line 83 in usr.bin/mdo/mdo.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
err(EXIT_FAILURE, "invalid username '%s'", username);
}
}

if (!uidonly) {
if (initgroups(pw->pw_name, pw->pw_gid) == -1)
if (initgroups(pw->pw_name, pw->pw_gid) == -1) {
syslog(LOG_AUTH | LOG_ERR, "USER: %s; failed to call initgroups: %d",

Check warning on line 90 in usr.bin/mdo/mdo.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
original_username,
EXIT_FAILURE);
err(EXIT_FAILURE, "failed to call initgroups");
if (setgid(pw->pw_gid) == -1)
}
if (setgid(pw->pw_gid) == -1) {
syslog(LOG_AUTH | LOG_ERR, "USER: %s; failed to call setgid: %d",

Check warning on line 96 in usr.bin/mdo/mdo.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
original_username,
EXIT_FAILURE);
err(EXIT_FAILURE, "failed to call setgid");
}
}
if (setuid(pw->pw_uid) == -1)
if (setuid(pw->pw_uid) == -1) {
syslog(LOG_AUTH | LOG_ERR, "USER: %s; failed to call setuid: %d",

Check warning on line 103 in usr.bin/mdo/mdo.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
original_username,
EXIT_FAILURE);
err(EXIT_FAILURE, "failed to call setuid");
}
if (*argv == NULL) {
const char *sh = getenv("SHELL");
if (sh == NULL)
sh = _PATH_BSHELL;
syslog(LOG_AUTH | LOG_INFO,
"USER: %s; COMMAND=%s",
original_username, sh);
execlp(sh, sh, "-i", NULL);
} else {
char *command = join_strings(argv, argc, " ");
syslog(LOG_AUTH | LOG_INFO, "USER: %s; COMMAND=%s",
Copy link
Member

Choose a reason for hiding this comment

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

Oops, previous comment was lost.

Note that allocating buffer in the function and returning is more prone to memory leaks, although for this particular use case it is Okay because we are calling execvp() right after, but it may make some static analyzers or tracers unhappy (because the memory allocated in the function was indeed lost, although they don't do any real harm).

Additionally you might want to use sbuf(9) to manage buffers. Or in other words something like:

struct sbuf *cmdbuf;

if ((cmdbuf = sbuf_new_auto()) == NULL)
    err(1, "sbuf_new_auto");

for (int i=0; i < argc; i++) {
    if (sbuf_cat(cmdbuf, argv[i]) != 0)
        err(1, "sbuf_cat");
    if (i < argc - 1)
        if (sbuf_cat(cmdbuf, " ") != 0)
            err(1, "sbuf_cat");
}

if (sbuf_finish(cmdbuf) != 0)
        err(1, "sbuf_finish");

syslog(LOG_AUTH | LOG_INFO, "USER: %s; COMMAND=%s",
    original_username,
    sbuf_data(cmdbuf));
execvp(argv[0], argv);
/* NOTREACHED */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the memory block of new generated command string is allocated by malloc, maybe I could just free the memory block after logging just like this?

image

As for using sbuf, I believe if we can guarantee that the join_strings function is well-implemented, we don't have to use it because there is no possibility of buffer overflows.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Since the VM space is going to be discarded anyway soon (with a successful execvp() or the err() below), the free() can be omitted here.

sbuf(9) is a "string builder", it keeps track of the length of string and handles buffer management. On the other hand, doing it your own, especially doing strcat() in a loop would have to iterate each characters in the string built so far in order to append to them. Therefore in most cases, using sbuf(9) is preferred for this kind of scenario.

Copy link
Member

Choose a reason for hiding this comment

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

open_memstream(3) can also be used instead of sbuf(9)

original_username,
command);
free(command);
execvp(argv[0], argv);
}
err(EXIT_FAILURE, "exec failed");

Check warning on line 124 in usr.bin/mdo/mdo.c

View workflow job for this annotation

GitHub Actions / Style Checker 

Missing Signed-off-by: line
}