libcrun: check setenv failure

[eriksjolund]

Closes: #1998

[gemini-code-assist]

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.

Footnotes

1. 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]

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;

[giuseppe]

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

[eriksjolund]

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

[eriksjolund]

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

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

[kolyshkin]

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

[eriksjolund]

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

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