src/login_nopam.c: Use iterative list_match

[stoeckmann]

The recursive nature of list_match triggered regression during refactoring. In Linux-PAM, the same code exists which could lead to stack overflow because access.conf could be arbitrarily long.

Use an iterative approach for easier refactoring, to support long lines in the future and to stay in sync with Linux-PAM.

Please see linux-pam/linux-pam#871 for more details since Linux-PAM could at least be affected on a very weird system configuration. Definitely not a security issue, but let's stay in sync and be prepared for a switch from fgets to getline.

[alejandro-colomar]

Hi @stoeckmann !

Thanks for the patch! I have a hard time reading it, and think a couple of trivial pre-patch refactors could make it somewhat simpler.

I've pushed some commits here:
https://github.com/alejandro-colomar/shadow/tree/access_pam_sync

I'd like to force-push my branch here, if you don't mind.

Here are the changes:

range-diff

$ git range-diff shadow/master ts/access_pam_sync access_pam_sync 
-:  -------- > 1:  6bb415d9 src/login_nopam.c: list_match(): Add superfluous else
-:  -------- > 2:  f2c86df3 src/login_nopam.c: list_match(): Move code around
1:  f33eb3a0 ! 3:  3d4878dd src/login_nopam.c: Use iterative list_match
    @@ src/login_nopam.c: list_match(char *list, const char *item, bool (*match_fn)(cha
        static const char  sep[] = ", \t";
      
        char *tok;
    --  bool match = false;
     +  bool inclusion = true;
     +  bool matched = false;
     +  bool result = false;
    @@ src/login_nopam.c: list_match(char *list, const char *item, bool (*match_fn)(cha
        while (NULL != (tok = strsep(&list, sep))) {
     -          if (strcasecmp (tok, "EXCEPT") == 0) {  /* EXCEPT: give up */
     -                  break;
    --          }
    --          match = (*match_fn) (tok, item);
    --          if (match) {
    --                  break;
     +          if (strcasecmp (tok, "EXCEPT") == 0) {  /* EXCEPT: invert */
     +                  if (!matched) { /* stop processing: not part of list */
     +                          break;
     +                  }
     +                  inclusion = !inclusion;
     +                  matched = false;
    -+          } else {
    -+                  bool match = (*match_fn) (tok, item);
    -+
    -+                  if (match) {
    +           } else {
    +                   bool  match;
    + 
    +                   match = (*match_fn)(tok, item);
    +                   if (match) {
    +-                          while (   (NULL != (tok = strsep(&list, sep)))
    +-                                 && (strcasecmp (tok, "EXCEPT") != 0))
    +-                                  /* VOID */ ;
    +-                          if (tok == NULL || !list_match(list, item, match_fn)) {
    +-                                  return (match);
    +-                          }
    +-                          break;
     +                          result = inclusion;
     +                          matched = true;
    -+                  }
    +                   }
                }
        }
      
    --  /* Process exceptions to matches. */
    --  if (match) {
    --          while (   (NULL != (tok = strsep(&list, sep)))
    --                 && (strcasecmp (tok, "EXCEPT") != 0))
    --                  /* VOID */ ;
    --          if (tok == NULL || !list_match(list, item, match_fn)) {
    --                  return (match);
    --          }
    --  }
     -  return false;
     +  return result;
      }

interdiff

$ git diff ts/access_pam_sync access_pam_sync 
diff --git a/src/login_nopam.c b/src/login_nopam.c
index b2cc0286..a104a2f7 100644
--- a/src/login_nopam.c
+++ b/src/login_nopam.c
@@ -167,8 +167,9 @@ list_match(char *list, const char *item, bool (*match_fn)(char *, const char*))
                        inclusion = !inclusion;
                        matched = false;
                } else {
-                       bool match = (*match_fn) (tok, item);
+                       bool  match;
 
+                       match = (*match_fn)(tok, item);
                        if (match) {
                                result = inclusion;
                                matched = true;

Which results in the following patch:

diff --git a/src/login_nopam.c b/src/login_nopam.c
index 9edd7b05..a104a2f7 100644
--- a/src/login_nopam.c
+++ b/src/login_nopam.c
@@ -149,33 +149,35 @@ list_match(char *list, const char *item, bool (*match_fn)(char *, const char*))
        static const char  sep[] = ", \t";
 
        char *tok;
+       bool inclusion = true;
+       bool matched = false;
+       bool result = false;
 
        /*
         * Process tokens one at a time. We have exhausted all possible matches
         * when we reach an "EXCEPT" token or the end of the list. If we do find
-        * a match, look for an "EXCEPT" list and recurse to determine whether
-        * the match is affected by any exceptions.
+        * a match, look for an "EXCEPT" list and determine whether the match is
+        * affected by any exceptions.
         */
        while (NULL != (tok = strsep(&list, sep))) {
-               if (strcasecmp (tok, "EXCEPT") == 0) {  /* EXCEPT: give up */
-                       break;
+               if (strcasecmp (tok, "EXCEPT") == 0) {  /* EXCEPT: invert */
+                       if (!matched) { /* stop processing: not part of list */
+                               break;
+                       }
+                       inclusion = !inclusion;
+                       matched = false;
                } else {
                        bool  match;
 
                        match = (*match_fn)(tok, item);
                        if (match) {
-                               while (   (NULL != (tok = strsep(&list, sep)))
-                                      && (strcasecmp (tok, "EXCEPT") != 0))
-                                       /* VOID */ ;
-                               if (tok == NULL || !list_match(list, item, match_fn)) {
-                                       return (match);
-                               }
-                               break;
+                               result = inclusion;
+                               matched = true;
                        }
                }
        }
 
-       return false;
+       return result;
 }
 
 /* myhostname - figure out local machine name */

Sounds good?

[stoeckmann]

As long as it doesn't break the code, feel free to do so. ;)

[alejandro-colomar]

As long as it doesn't break the code, feel free to do so. ;)

Done. Thanks! (It hopefully shouldn't break anything.) :)

[alejandro-colomar]

While reviewing, I realized of another trivial refactor to make the last patch more readable. I'll force-pushed again.

range-diff

$ git range-diff shadow/master ts/access_pam_sync access_pam_sync 
1:  6bb415d9 = 1:  6bb415d9 src/login_nopam.c: list_match(): Add superfluous else
2:  f2c86df3 = 2:  f2c86df3 src/login_nopam.c: list_match(): Move code around
-:  -------- > 3:  1f18b1a2 src/login_nopam.c: list_match(): '(match)' is always true here
3:  3d4878dd ! 4:  d52748bf src/login_nopam.c: Use iterative list_match
    @@ Metadata
     Author: Tobias Stoeckmann <tobias@stoeckmann.org>
     
      ## Commit message ##
    -    src/login_nopam.c: Use iterative list_match
    +    src/login_nopam.c: list_match(): Use iteration intead of recursion
     
    -    The recursive nature of list_match triggered regression during
    -    refactoring. In Linux-PAM, the same code exists which could lead to
    -    stack overflow because access.conf could be arbitrarily long.
    +    The recursive nature of list_match() triggered regression during
    +    refactoring.  In Linux-PAM, the same code exists which could lead to
    +    stack overflow because <access.conf> could be arbitrarily long.
     
         Use an iterative approach for easier refactoring, to support long
         lines in the future and to stay in sync with Linux-PAM.
    @@ src/login_nopam.c: list_match(char *list, const char *item, bool (*match_fn)(cha
     -                          while (   (NULL != (tok = strsep(&list, sep)))
     -                                 && (strcasecmp (tok, "EXCEPT") != 0))
     -                                  /* VOID */ ;
    --                          if (tok == NULL || !list_match(list, item, match_fn)) {
    --                                  return (match);
    --                          }
    +-                          if (tok == NULL || !list_match(list, item, match_fn))
    +-                                  return true;
    +-
     -                          break;
     +                          result = inclusion;
     +                          matched = true;

interdiff

$ git diff ts/access_pam_sync access_pam_sync 
$

Which results in the following patch:

the meaningful patch

diff --git a/src/login_nopam.c b/src/login_nopam.c
index d2f28517..a104a2f7 100644
--- a/src/login_nopam.c
+++ b/src/login_nopam.c
@@ -149,33 +149,35 @@ list_match(char *list, const char *item, bool (*match_fn)(char *, const char*))
        static const char  sep[] = ", \t";
 
        char *tok;
+       bool inclusion = true;
+       bool matched = false;
+       bool result = false;
 
        /*
         * Process tokens one at a time. We have exhausted all possible matches
         * when we reach an "EXCEPT" token or the end of the list. If we do find
-        * a match, look for an "EXCEPT" list and recurse to determine whether
-        * the match is affected by any exceptions.
+        * a match, look for an "EXCEPT" list and determine whether the match is
+        * affected by any exceptions.
         */
        while (NULL != (tok = strsep(&list, sep))) {
-               if (strcasecmp (tok, "EXCEPT") == 0) {  /* EXCEPT: give up */
-                       break;
+               if (strcasecmp (tok, "EXCEPT") == 0) {  /* EXCEPT: invert */
+                       if (!matched) { /* stop processing: not part of list */
+                               break;
+                       }
+                       inclusion = !inclusion;
+                       matched = false;
                } else {
                        bool  match;
 
                        match = (*match_fn)(tok, item);
                        if (match) {
-                               while (   (NULL != (tok = strsep(&list, sep)))
-                                      && (strcasecmp (tok, "EXCEPT") != 0))
-                                       /* VOID */ ;
-                               if (tok == NULL || !list_match(list, item, match_fn))
-                                       return true;
-
-                               break;
+                               result = inclusion;
+                               matched = true;
                        }
                }
        }
 
-       return false;
+       return result;
 }
 
 /* myhostname - figure out local machine name */

[alejandro-colomar]

I think it's correct. Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

Still, I'd like someone else to review too. It's a tricky patch.