Duplicate code? Maybe refactoring glitch?

[alejandro-colomar]

Consider the following code:

$ grepc -tfd nxt_clone_vldt_credential_gidmap
./src/nxt_clone.c:310:
nxt_int_t
nxt_clone_vldt_credential_gidmap(nxt_task_t *task,
    nxt_clone_credential_map_t *map, nxt_credential_t *creds)
{
    nxt_uint_t             base_ok, gid_ok, gids_ok;
    nxt_uint_t             i, j;
    nxt_runtime_t          *rt;
    nxt_clone_map_entry_t  m;

    ...

    base_ok = 0;
    gids_ok = 0;

    for (i = 0; i < creds->ngroups; i++) {
        gid_ok = 0;

        for (j = 0; j < map->size; j++) {
            m = map->map[j];

            if (!base_ok && creds->base_gid >= (nxt_gid_t) m.container
                && creds->base_gid < (nxt_gid_t) (m.container+m.size))
            {
                base_ok = 1;
            }

            if (creds->gids[i] >= (nxt_gid_t) m.container
                && creds->gids[i] < (nxt_gid_t) (m.container+m.size))
            {
                gid_ok = 1;
                break;
            }
        }

        if (nxt_fast_path(gid_ok)) {
            gids_ok++;
        }
    }

    if (!base_ok) {
        for (i = 0; i < map->size; i++) {
            m = map->map[i];

            if (creds->base_gid >= (nxt_gid_t) m.container
                && creds->base_gid < (nxt_gid_t) (m.container+m.size))
            {
                base_ok = 1;
                break;
            }
        }
    }

    if (nxt_slow_path(!base_ok)) {
        nxt_log(task, NXT_LOG_ERR, "\"gidmap\" field has no \"container\" "
                "entry for gid %d.", creds->base_gid);

        return NXT_ERROR;
    }

    if (nxt_slow_path(gids_ok < creds->ngroups)) {
        nxt_log(task, NXT_LOG_ERR, "\"gidmap\" field has missing "
                "suplementary gid mappings (found %d out of %d).", gids_ok,
                creds->ngroups);

        return NXT_ERROR;
    }

    return NXT_OK;
}

It's running the check for base_ok many times in a loop (once for every suplementary group), and the result will be the same always, and then once again after the loop. I believe we can keep the last check and remove the first one (the one within the supplementary groups' loop).

I'm suggesting this as a prepatch for the fix for https://github.com/nginx/unit/issues/788, since this code affects the fix. I'd like to make sure I'm not missing something.

[alejandro-colomar]

The patch would be:

diff --git a/src/nxt_clone.c b/src/nxt_clone.c
index a9b39ac1..5395ab80 100644
--- a/src/nxt_clone.c
+++ b/src/nxt_clone.c
@@ -395,12 +395,6 @@ nxt_clone_vldt_credential_gidmap(nxt_task_t *task,
         for (j = 0; j < map->size; j++) {
             m = map->map[j];

-            if (!base_ok && creds->base_gid >= (nxt_gid_t) m.container
-                && creds->base_gid < (nxt_gid_t) (m.container+m.size))
-            {
-                base_ok = 1;
-            }
-
             if (creds->gids[i] >= (nxt_gid_t) m.container
                 && creds->gids[i] < (nxt_gid_t) (m.container+m.size))
             {
@@ -414,16 +408,14 @@ nxt_clone_vldt_credential_gidmap(nxt_task_t *task,
         }
     }

-    if (!base_ok) {
-        for (i = 0; i < map->size; i++) {
-            m = map->map[i];
+    for (i = 0; i < map->size; i++) {
+        m = map->map[i];

-            if (creds->base_gid >= (nxt_gid_t) m.container
-                && creds->base_gid < (nxt_gid_t) (m.container+m.size))
-            {
-                base_ok = 1;
-                break;
-            }
+        if (creds->base_gid >= (nxt_gid_t) m.container
+            && creds->base_gid < (nxt_gid_t) (m.container+m.size))
+        {
+            base_ok = 1;
+            break;
         }
     }