libcrun: check setenv failure by eriksjolund · Pull Request #2007 · containers/crun

eriksjolund · 2026-02-09T16:01:59Z

Closes: #1998

gemini-code-assist · 2026-02-09T16:02:18Z

Summary of Changes

Hello @eriksjolund, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness of libcrun by integrating comprehensive error checking for all setenv calls across critical container setup functions. Previously, potential failures in setting environment variables like HOME or LISTEN_PID might have gone unnoticed, leading to undefined behavior. With these changes, each setenv operation is now explicitly validated, and appropriate error handling is triggered upon failure, ensuring more stable and reliable container initialization.

Highlights

Robustness for HOME environment variable: Introduced error checking for setenv calls when setting the HOME environment variable to / in setup_environment and exec_process_entrypoint functions, ensuring proper error handling if the operation fails.
Robustness for LISTEN_PID environment variable: Added error handling for setenv when setting LISTEN_PID to 1 in container_init_setup, preventing silent failures if the environment variable cannot be set.
Improved HOME environment variable detection: Implemented error checking for setenv when dynamically setting the HOME environment variable based on user information within the set_home_env utility function, enhancing reliability.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

src/libcrun/container.c
- Added error checking for setenv("HOME", "/", 1) calls in setup_environment and exec_process_entrypoint, returning a crun_make_error on failure.
- Implemented error checking for the setenv("LISTEN_PID", "1", 1) call in container_init_setup, returning a crun_make_error on failure.
src/libcrun/utils.c
- Modified set_home_env to check the return value of setenv("HOME", ret_pw->pw_dir, 1) and jump to an error handling block if the call fails.

Activity

This pull request addresses and closes issue Should setenv() return value be checked? #1998, indicating a fix for a previously identified problem.
No other specific activity (comments, reviews, progress updates) has been recorded for this pull request.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request adds necessary error checking for setenv calls, which improves the robustness of the code. The changes in src/libcrun/container.c are correct. However, the modifications in src/libcrun/utils.c to the set_home_env function introduce a bug in the error handling logic. My review includes a comment detailing this issue.

gemini-code-assist · 2026-02-09T16:04:49Z

src/libcrun/utils.c

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


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;

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

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;

}

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

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

@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.

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

set_home_env() has an unnecessary ternary operator #2010

check for errno value EINTR after fgetpwent_r() failure missing #2009

giuseppe

nit: could you wrap the checks into UNLIKELY?

You'll need to run make clang-format before committing

giuseppe · 2026-02-09T17:28:29Z

src/libcrun/utils.c

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


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

packit-as-a-service · 2026-03-08T14:04:54Z

TMT tests failed. @containers/packit-build please check.

packit-as-a-service · 2026-03-08T14:04:55Z

TMT tests failed. @containers/packit-build please check.

giuseppe · 2026-03-09T07:52:58Z

src/libcrun/utils.c

-          setenv ("HOME", ret_pw->pw_dir, 1);
-          return 0;
+          if (UNLIKELY (setenv ("HOME", ret_pw->pw_dir, 1) < 0))
+            OOM ();


can we just set errno and return -1 here?

We may look at better error reporting later, but OOM seems overkill here

setenv can fail for two reasons: EINVAL (when name is null, or empty, or contains =, which is not the case with $HOME), and ENOMEM. From the quick glance into glibc's stdlib/setenv.c it seems to be true, so OOM seems legit here.

The above theoretically means we can OOM() in other places after setenv failed, if its first argument is known to be good. Practically, I'm not sure since non-glibc implementations can possibly return other errors (although looking into current musl sources, it's the same as for glibc).

ok that is good, OOM looks correct. Thanks!

I haven't had time to look into this, but my original idea by using OOM() was to avoid that
the environment variable HOME is set to / because the system is low on memory, for example here

crun/src/libcrun/container.c

Lines 3690 to 3696 in c07aadc

ret = set_home_env (container_uid);

if (UNLIKELY (ret < 0))

{

setenv ("HOME", "/", 1);

libcrun_warning ("cannot detect HOME environment variable, setting default");

}

}

Thinking more about it, a similar problem could occur if fopen() fails with ENFILE here

crun/src/libcrun/utils.c

Lines 1457 to 1459 in c07aadc

stream = fopen ("/etc/passwd", "re");

if (stream == NULL)

return -1;

Maybe set_home_env() should take an err argument so that it is possible for the caller to differentiate between

no. home path was found in /etc/passwd

some other problems occured (for instance ENFILE or ENOMEM)

@eriksjolund this sounds good to me. I'm going to merge this PR as is though, as it fixes the issue #1998.

Feel free to a new PR, improving set_home_env error reporting.

... and also calling OOM in case setenv failed, in other places, like suggested above.

giuseppe

LGTM

kolyshkin · 2026-03-13T05:51:40Z

Ah, this needs a rebase to bring in #2044 so CI is green, and then I can merge.

@eriksjolund can you please rebase?

Closes: containers#1998 Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>

eriksjolund mentioned this pull request Feb 9, 2026

Should setenv() return value be checked? #1998

Closed

gemini-code-assist bot reviewed Feb 9, 2026

View reviewed changes

giuseppe reviewed Feb 9, 2026

View reviewed changes

eriksjolund force-pushed the check-setenv-failure branch 2 times, most recently from 5b751b8 to ffad99c Compare March 8, 2026 13:51

eriksjolund marked this pull request as ready for review March 8, 2026 14:19

giuseppe reviewed Mar 9, 2026

View reviewed changes

giuseppe approved these changes Mar 12, 2026

View reviewed changes

kolyshkin approved these changes Mar 12, 2026

View reviewed changes

libcrun: check setenv failure

4db1709

Closes: containers#1998 Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>

eriksjolund force-pushed the check-setenv-failure branch from ffad99c to 4db1709 Compare March 13, 2026 07:09

giuseppe merged commit 53938d1 into containers:main Mar 16, 2026
48 checks passed

	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;
	}

	ret = set_home_env (container_uid);
	if (UNLIKELY (ret < 0))
	{
	setenv ("HOME", "/", 1);
	libcrun_warning ("cannot detect HOME environment variable, setting default");
	}
	}

	stream = fopen ("/etc/passwd", "re");
	if (stream == NULL)
	return -1;

Conversation

eriksjolund commented Feb 9, 2026

Uh oh!

gemini-code-assist bot commented Feb 9, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

packit-as-a-service bot commented Mar 8, 2026

Uh oh!

packit-as-a-service bot commented Mar 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants