Skip to content

libcrun: check setenv failure #2007

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

Draft
eriksjolund wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −5
Draft

libcrun: check setenv failure #2007

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
9 changes: 6 additions & 3 deletions src/libcrun/container.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,8 +1188,9 @@ setup_environment (runtime_spec_schema_config_schema *def, uid_t container_uid,
ret = set_home_env (container_uid);
if (UNLIKELY (ret < 0 && errno != ENOTSUP))
{
setenv ("HOME", "/", 1);
libcrun_warning ("cannot detect HOME environment variable, setting default");
if (setenv ("HOME", "/", 1) < 0)
return crun_make_error (err, errno, "setenv HOME");
}
}

Expand Down Expand Up @@ -1407,8 +1408,9 @@ container_init_setup (void *args, pid_t own_pid, char *notify_socket,
/* Set primary process to 1 explicitly if nothing is configured and LISTEN_FD is not set. */
if (entrypoint_args->context->listen_fds > 0 && getenv ("LISTEN_PID") == NULL)
{
setenv ("LISTEN_PID", "1", 1);
libcrun_warning ("setting LISTEN_PID=1 since no previous configuration was found");
if (setenv ("LISTEN_PID", "1", 1) < 0)
return crun_make_error (err, errno, "setenv LISTENPID");
}

/* Attempt to chdir immediately here, before doing the setresuid. If we fail here, let's
Expand Down Expand Up @@ -3691,8 +3693,9 @@ exec_process_entrypoint (libcrun_context_t *context,
ret = set_home_env (container_uid);
if (UNLIKELY (ret < 0 && errno != ENOTSUP))
{
setenv ("HOME", "/", 1);
libcrun_warning ("cannot detect HOME environment variable, setting default");
if (setenv ("HOME", "/", 1) < 0)
return crun_make_error (err, errno, "setenv HOME");
}
}

Expand Down
7 changes: 5 additions & 2 deletions src/libcrun/utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1476,8 +1476,11 @@ set_home_env (uid_t id)

if (ret_pw && ret_pw->pw_uid == id)
{
setenv ("HOME", ret_pw->pw_dir, 1);
return 0;
ret = setenv ("HOME", ret_pw->pw_dir, 1);
if (UNLIKELY(ret < 0))
goto error;
else
return 0;
Comment on lines +1480 to +1483
Copy link

@gemini-code-assist gemini-code-assist bot Feb 9, 2026

Choose a reason for hiding this comment

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

high

The else statement is redundant and can be removed for clarity.

More critically, the reuse of the ret variable introduces a bug. The set_home_env function now incorrectly returns 0 (success) when a user is not found in /etc/passwd. This happens because fgetpwent_r sets ret to 0 on success, and if the user isn't found, the function proceeds to the error label where ret ? -errno : 0 evaluates to 0.

The original logic of returning -1 from the error label was correct for all failure cases, including 'user not found'. The new logic return ret ? -errno : 0; is flawed and should be reconsidered.

          if (UNLIKELY(ret < 0))
            goto error;
          return 0;

Copy link
Member

@giuseppe giuseppe Feb 9, 2026

Choose a reason for hiding this comment

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

this should be addressed, ret is used after the error label.

Copy link
Contributor Author

@eriksjolund eriksjolund Feb 10, 2026

Choose a reason for hiding this comment

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

Do you know why there is a ternary operator on line 1486 in the current main branch?

It seems ret could never be 0

Here is code from the current main branch.

crun/src/libcrun/utils.c

Lines 1450 to 1487 in 0bc3050

int ret = -1;
buf_size = sysconf (_SC_GETPW_R_SIZE_MAX);
if (buf_size < 0)
buf_size = 1024;
buf = xmalloc (buf_size);
stream = fopen ("/etc/passwd", "re");
if (stream == NULL)
goto error;
for (;;)
{
struct passwd *ret_pw = NULL;
ret = fgetpwent_r (stream, &pwd, buf, buf_size, &ret_pw);
if (UNLIKELY (ret != 0))
{
if (errno != ERANGE)
goto error;
buf_size *= 2;
buf = xrealloc (buf, buf_size);
continue;
}
if (ret_pw && ret_pw->pw_uid == id)
{
setenv ("HOME", ret_pw->pw_dir, 1);
return 0;
}
}
error:
/* Let callers handle the error if the user was not found. */
return ret ? -errno : 0;
}

Copy link
Contributor Author

@eriksjolund eriksjolund Feb 10, 2026

Choose a reason for hiding this comment

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

I tried another approach in
3d23abe
If you like it, I could switch to that.

(Untested code. I haven't run clang-format yet)

Copy link
Collaborator

@kolyshkin kolyshkin Feb 11, 2026

Choose a reason for hiding this comment

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

@eriksjolund I like the approach of not reusing ret for setenv checks.

Yet in 3d23abe you also refactor the set_home_env, which seems kind of out of scope for this PR. If you want to refactor it, please do so in a separate commit (so you can explain what's going on). Currently, your commit description says "check setenv failure" but you also do some other things, which is confusing.

Copy link
Contributor Author

@eriksjolund eriksjolund Feb 11, 2026

Choose a reason for hiding this comment

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

@kolyshkin yes, I will need to create separate commits and PR.
To keep better order I created

}
}

Expand Down
Loading