From c1fdf876684e884b6d466a4635cde329777cc777 Mon Sep 17 00:00:00 2001
From: Yashodhan Joshi <yjdoc2@gmail.com>
Date: Tue, 10 Dec 2024 19:08:44 +0530
Subject: [PATCH] fix duplicate gids in container creation

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
---
 .../src/process/container_init_process.rs     | 17 ++++++---
 crates/libcontainer/src/syscall/test.rs       |  6 ++--
 .../tests/process_user/process_user_test.rs   | 36 +++++++++++++++----
 3 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs
index 0d16e6094..f09f04dcc 100644
--- a/crates/libcontainer/src/process/container_init_process.rs
+++ b/crates/libcontainer/src/process/container_init_process.rs
@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::{env, fs, mem};
@@ -761,6 +761,9 @@ fn set_supplementary_gids(
 
         let gids: Vec<Gid> = additional_gids
             .iter()
+            // this is to remove duplicate ids, so we behave similar to runc
+            .collect::<HashSet<_>>()
+            .into_iter()
             .map(|gid| Gid::from_raw(*gid))
             .collect();
 
@@ -1031,7 +1034,7 @@ mod tests {
                     .additional_gids(vec![33, 34])
                     .build()?,
                 None::<UserNamespaceConfig>,
-                vec![vec![Gid::from_raw(33), Gid::from_raw(34)]],
+                vec![Gid::from_raw(33), Gid::from_raw(34)],
             ),
             // unreachable case
             (
@@ -1052,7 +1055,7 @@ mod tests {
                     user_namespace: None,
                     ..Default::default()
                 }),
-                vec![vec![Gid::from_raw(37), Gid::from_raw(38)]],
+                vec![Gid::from_raw(37), Gid::from_raw(38)],
             ),
         ];
         for (user, ns_config, want) in tests.into_iter() {
@@ -1069,7 +1072,13 @@ mod tests {
                         .downcast_ref::<TestHelperSyscall>()
                         .unwrap()
                         .get_groups_args();
-                    assert_eq!(want, got);
+                    // set set_supplementary_gids uses hashset internally
+                    // so we cannot be sure of the order, hence compare the
+                    // length and includes
+                    assert_eq!(want.len(), got.len());
+                    for gid in &want {
+                        assert!(got.contains(gid));
+                    }
                 }
                 _ => unreachable!("setgroups value unknown"),
             }
diff --git a/crates/libcontainer/src/syscall/test.rs b/crates/libcontainer/src/syscall/test.rs
index 6e2e01977..ad218b492 100644
--- a/crates/libcontainer/src/syscall/test.rs
+++ b/crates/libcontainer/src/syscall/test.rs
@@ -369,13 +369,13 @@ impl TestHelperSyscall {
             .collect::<Vec<String>>()
     }
 
-    pub fn get_groups_args(&self) -> Vec<Vec<Gid>> {
+    pub fn get_groups_args(&self) -> Vec<Gid> {
         self.mocks
             .fetch(ArgName::Groups)
             .values
             .iter()
-            .map(|x| x.downcast_ref::<Vec<Gid>>().unwrap().clone())
-            .collect::<Vec<Vec<Gid>>>()
+            .flat_map(|x| x.downcast_ref::<Vec<Gid>>().unwrap().clone())
+            .collect::<Vec<Gid>>()
     }
 
     pub fn get_io_priority_args(&self) -> Vec<IoPriorityArgs> {
diff --git a/tests/contest/contest/src/tests/process_user/process_user_test.rs b/tests/contest/contest/src/tests/process_user/process_user_test.rs
index 5329ccbbf..1d7787d6d 100644
--- a/tests/contest/contest/src/tests/process_user/process_user_test.rs
+++ b/tests/contest/contest/src/tests/process_user/process_user_test.rs
@@ -1,4 +1,4 @@
-use anyhow::{Context, Ok, Result};
+use anyhow::{anyhow, Context, Ok, Result};
 use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder, UserBuilder};
 use rand::Rng;
 use test_framework::{test_result, Test, TestGroup, TestResult};
@@ -21,12 +21,12 @@ fn generate_unique_random_vec() -> Vec<u32> {
     ret
 }
 
-fn create_spec() -> Result<Spec> {
+fn create_spec(gids: Vec<u32>) -> Result<Spec> {
     let umask = 0o002;
     let user = UserBuilder::default()
         .uid(10u32)
         .gid(10u32)
-        .additional_gids(generate_unique_random_vec())
+        .additional_gids(gids)
         .umask(umask as u32)
         .build()?;
 
@@ -42,16 +42,38 @@ fn create_spec() -> Result<Spec> {
         .context("failed to build spec")?;
     Ok(spec)
 }
-fn process_user_test() -> TestResult {
-    let spec = test_result!(create_spec());
+
+fn process_user_test_unique_gids() -> TestResult {
+    let gids = generate_unique_random_vec();
+    let spec = test_result!(create_spec(gids));
     test_inside_container(spec, &CreateOptions::default(), &|_| Ok(()))
 }
 
+fn process_user_test_duplicate_gids() -> TestResult {
+    let mut gids = generate_unique_random_vec();
+    let duplicate = gids[0];
+    gids.push(duplicate);
+    let spec = test_result!(create_spec(gids));
+    match test_inside_container(spec, &CreateOptions::default(), &|_| Ok(())) {
+        TestResult::Passed => TestResult::Failed(anyhow!(
+            "expected test with duplicate gids to fail, but it passed instead"
+        )),
+        _ => TestResult::Passed,
+    }
+}
+
 pub fn get_process_user_test() -> TestGroup {
     let mut process_user_test_group = TestGroup::new("process_user");
 
-    let test = Test::new("process_user_test", Box::new(process_user_test));
-    process_user_test_group.add(vec![Box::new(test)]);
+    let test1 = Test::new(
+        "process_user_unique_gids_test",
+        Box::new(process_user_test_unique_gids),
+    );
+    let test2 = Test::new(
+        "process_user_duplicate_gids_test",
+        Box::new(process_user_test_duplicate_gids),
+    );
+    process_user_test_group.add(vec![Box::new(test1), Box::new(test2)]);
 
     process_user_test_group
 }