Override HOME if its set to the empty string

[rata]

I'm testing runc 1.3 in docker CI and I found these issues.

I've also added some e2e tests and verified that runc 1.2 behaves the same way.

Below the commit msg of the fix, but see each commit msg for more details

---

Before commit 06f1e07 ("libct: speedup process.Env handling") we were
overriding HOME if it was set to "" too1. But now we only override it
if it wasn't set at all.

This patch restores the old behavior of overriding it if it was set to
an empty value.

Docker relies on this behaviour since ages2.

[Copilot]

Pull Request Overview

This PR restores the previous behavior of overriding the HOME environment variable when it’s set to an empty string, ensuring compatibility with longstanding expectations from Docker and earlier runc versions.

• Introduces a flag to determine whether HOME should be overridden.
• Adds conditional checks to preserve non-empty HOME values.
• Updates e2e tests to verify consistency with runc 1.2 behavior.

Files not reviewed (1)

• tests/integration/env.bats: Language not supported

Comments suppressed due to low confidence (1)

libcontainer/env.go:27

• [nitpick] Consider renaming 'overrideHome' to 'shouldOverrideHome' to better indicate that it's a flag controlling whether HOME should be overridden.

overrideHome := true

[kolyshkin]

WRT tests that you add -- maybe it's easier to write those in Go and add to libcontainer/env_test.go, so all tests are in one place.

[rata]

@kolyshkin I thought about adding tests there. The unit tests are nice, but in a big refactor they might not exist anymore. In fact, prepareEnv() didn't exist before the last refactor, it might not exist after the next one.

I really think we need e2e tests for this. I want us to be able to refactor and change heavily the way we set env vars, and have tests that catch any buggy behavior. These tests are testing runc behaves in the right way, no matter how it is implemented (that is why e2e tests instead of unit tests)

[rata]

This indeed fixes docker CI and it's ready for review now :)

[kolyshkin]

@kolyshkin I thought about adding tests there. The unit tests are nice, but in a big refactor they might not exist anymore. In fact, prepareEnv() didn't exist before the last refactor, it might not exist after the next one.

In that case, the tests should migrate to a different place.

But if you modify a function which already have a unit test, it's best to amend the unit test as well.

Feel free to add bats tests, too; I understand their benefit. Just don't abandon the unit test.

[rata]

@kolyshkin Fixed all the comments and added unit tests. PTAL. I left unresolved the only comment that I think the discussion is interesting and you might want to see in more detail.

[kolyshkin]

LGTM except for a few nits (feel free to ignore). Thanks for testing runc 1.2, too, this is important.

It would be nice to add failed Docker tests links (those from this PR description) to the second commit message and/or to bats file comment.

Thanks!

[kolyshkin]

Almost forgot: please fix prepareEnv docstring accordingly, i.e. something like:

-//   - adds HOME to returned environment, if not found in the list.
+//   - adds HOME to returned environment, if not found in the list,
+//     or the value is empty.

[rata]

@kolyshkin thanks, fixed!

[kolyshkin]

Still LGTM, thanks!

[rata]

@AkihiroSuda @lifubang PTAL

[kolyshkin]

1.3 backport done (included into 1.3.0-rc.2): #4711

Fixing the labels accordingly.