Align with runc: use user's HOME when HOME is empty string #3269
Conversation
bells17
force-pushed
the
fix-home-env
branch
2 times, most recently
from
96ae877 to
97516cc
Compare
Signed-off-by: bells17 <[email protected]>
YJDoc2
left a comment
YJDoc2
left a comment
There was a problem hiding this comment.
Overall LGTM, a couple of nits/questions
| fn set_home_env_if_not_exists(envs: &mut HashMap<String, String>, uid: Uid) { | ||
| if envs.get("HOME").is_none_or(|v| v.is_empty()) { | ||
| if let Some(dir_home) = utils::get_user_home(uid.into()) { | ||
| envs.insert("HOME".to_owned(), dir_home.to_string_lossy().to_string()); |
There was a problem hiding this comment.
Should we be using to_string_lossy here? If for some reason, the home path has some invalid unicode chars, it will get incorrectly converted here right? Should we instead use to_str and in case of None, silently ignore like runc?
There was a problem hiding this comment.
Thanks for pointing that out. I've fixed it according to your comment and added tests. Could you please review it again?
dd0e822
| fn test_set_home_env_if_not_exists_not_set() { | ||
| let mut envs = HashMap::new(); | ||
|
|
||
| set_home_env_if_not_exists(&mut envs, Uid::from_raw(0)); |
There was a problem hiding this comment.
Can we add one more test for non-zero uid?
There was a problem hiding this comment.
I've added it. Can you check it again, please?
dd0e822
Signed-off-by: bells17 <[email protected]>
|
Sorry, the test failed, I'll fix it. |
|
@bells17 |
|
@bells17 ping! |
3 participants
Description
Looking at runc's implementation, even when the HOME environment variable is specified, it does not add it if it's an empty string.
https://github.com/opencontainers/runc/blob/v1.3.2/libcontainer/env.go#L64
However, in youki, it appeared to be set even when it's an empty string, so I made a fix to align with runc's implementation.
Type of Change
Testing
Related Issues
Fixes #
Additional Context
none