Skip to content

librc/run/shutdown: precalc strlen moved out of bounds loop #790

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

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

librc/run/shutdown: precalc strlen moved out of bounds loop #790

Show file tree
Hide file tree
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
3 changes: 2 additions & 1 deletion src/librc/librc-daemon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ rc_find_pids(const char *exec, const char *const *argv, uid_t uid, pid_t pid)
my_ns[0] = '\0';
}

size_t len_my_ns = strlen(my_ns);
while ((entry = readdir(procdir)) != NULL) {
if (sscanf(entry->d_name, "%d", &p) != 1)
continue;
Expand All @@ -172,7 +173,7 @@ rc_find_pids(const char *exec, const char *const *argv, uid_t uid, pid_t pid)
proc_ns[0] = '\0';
}
free(buffer);
if (pid == 0 && strlen(my_ns) && strlen (proc_ns) && strcmp(my_ns, proc_ns))
if (pid == 0 && len_my_ns && strlen (proc_ns) && strcmp(my_ns, proc_ns))
Copy link
Member

@vapier vapier Mar 14, 2025

Choose a reason for hiding this comment

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

these strlen calls simply want to know if the string is empty. that can be checked by my_ns[0] and proc_ns[0] -- no need for strlen at all.

continue;
if (uid) {
xasprintf(&buffer, "/proc/%d", p);
Expand Down
2 changes: 1 addition & 1 deletion src/librc/librc-misc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,8 @@ static void rc_config_set_value(RC_STRINGLIST *config, char *value)
replaced = false;
/* In shells the last item takes precedence, so we need to remove
any prior values we may already have */
i = strlen(entry);
TAILQ_FOREACH(cline, config, entries) {
i = strlen(entry);
if (strncmp(entry, cline->value, i) == 0 && cline->value[i] == '=') {
/* We have a match now - to save time we directly replace it */
free(cline->value);
Expand Down
9 changes: 6 additions & 3 deletions src/openrc-run/openrc-run.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,9 @@ write_prefix(const char *buffer, size_t bytes, bool *prefixed)
else
ewarnv("Couldn't open the prefix lock, please make sure you have enough permissions");

size_t len_ec = strlen(ec);
size_t len_prefix = strlen(prefix);
size_t len_ec_normal = strlen(ec_normal);
for (i = 0; i < bytes; i++) {
/* We don't prefix eend calls (cursor up) */
if (buffer[i] == '\033' && !*prefixed) {
Expand All @@ -320,9 +323,9 @@ write_prefix(const char *buffer, size_t bytes, bool *prefixed)
}

if (!*prefixed) {
Copy link
Member

@vapier vapier Mar 14, 2025

Choose a reason for hiding this comment

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

this codepath is called once per-line. while calling this function with multiline strings is certainly valid, and i'm sure done in places, it's not the common case, and I have a hard time seeing strlen being called on tiny strings showing up in any perf situation.

len_ec & ec_normal will be like 7 bytes each. len_prefix will be ~length of longest service name, and that's likely to also be quite small.

ret += write(fd, ec, strlen(ec));
ret += write(fd, prefix, strlen(prefix));
ret += write(fd, ec_normal, strlen(ec_normal));
ret += write(fd, ec, len_ec);
ret += write(fd, prefix, len_prefix);
ret += write(fd, ec_normal, len_ec_normal);
ret += write(fd, "|", 1);
*prefixed = true;
}
Expand Down
3 changes: 2 additions & 1 deletion src/openrc-shutdown/broadcast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,11 @@ void broadcast(char *text)

setutxent();

size_t len_path_dev = strlen(_PATH_DEV);
while ((utmp = getutxent()) != NULL) {
if (utmp->ut_type != USER_PROCESS || utmp->ut_user[0] == 0)
continue;
if (strncmp(utmp->ut_line, _PATH_DEV, strlen(_PATH_DEV)) == 0)
Copy link
Member

@vapier vapier Mar 14, 2025

Choose a reason for hiding this comment

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

_PATH_DEV is a constant string. if the compiler can't optimize that to a constant integer at compile time, it's not worth caring about. drop this change please.

if (strncmp(utmp->ut_line, _PATH_DEV, len_path_dev) == 0)
xasprintf(&term, "%s", utmp->ut_line);
else
xasprintf(&term, "%s%s", _PATH_DEV, utmp->ut_line);
Expand Down