[PATCH] maint: prohibit direct use of strncmp: prefer STREQ_LEN, STRNCMP_LIT

[Jim Meyering]

Here's a proposed change.
It makes the code a little cleaner by removing hard-coded
string lengths at the expense of introducing new macros:

    -              if (strncmp ("/auto/", mount_point, 6) == 0)
    +              if (STRNCMP_LIT (mount_point, "/auto/") == 0)

    -              else if (strncmp ("/tmp_mnt/", mount_point, 9) == 0)
    +              else if (STRNCMP_LIT (mount_point, "/tmp_mnt/") == 0)

And along the lines of the strcmp->STREQ conversion,
now we can do the same with strncmp:

    -  if (strncmp (s + i, DIGEST_TYPE_STRING, algo_name_len) == 0)
    +  if (STREQ_LEN (s + i, DIGEST_TYPE_STRING, algo_name_len))


>From 5e09d11701b283f5c2badcb7ac9f3d42c862a3e0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Thu, 31 Mar 2011 17:59:16 +0200
Subject: [PATCH] maint: prohibit direct use of strncmp: prefer STREQ_LEN,
 STRNCMP_LIT

* cfg.mk (sc_prohibit_strncmp): New rule, mostly from libvirt.
* src/system.h (STREQ_LEN, STRPREFIX, STRNCMP_LIT): Define.
* src/df.c (get_dev, get_point): Convert.
* src/extent-scan.c (extent_need_sync): Likewise.
* src/ls.c (is_colored, decode_switches): Likewise.
(parse_ls_color, (print_color_indicator): Likewise.
* src/md5sum.c (split_3): Likewise.
* src/split.c (main, emit_ancillary_info): Likewise.
* src/tr.c (look_up_char_class): Likewise.
* src/uname.c (main): Likewise.
* src/who.c (scan_entries): Likewise.
---
 cfg.mk            |    9 +++++++++
 src/df.c          |    6 +++---
 src/extent-scan.c |    2 +-
 src/ls.c          |   14 +++++++-------
 src/md5sum.c      |    2 +-
 src/split.c       |    4 ++--
 src/system.h      |    9 ++++++++-
 src/tr.c          |    2 +-
 src/uname.c       |    2 +-
 src/who.c         |    2 +-
 10 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 99a6e5e..48e5a47 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -315,6 +315,15 @@ sc_space_before_open_paren:
        else :;                                                         \
        fi

+# Similar to the gnulib maint.mk rule for sc_prohibit_strcmp
+# Use STREQ_LEN or STRPREFIX rather than comparing strncmp == 0, or != 0.
+sc_prohibit_strncmp:
+       @grep -nE '! *str''ncmp *\(|\<str''ncmp *\(.+\) *[!=]='         \
+           $$($(VC_LIST_EXCEPT))                                       \
+         | grep -vE ':# *define STR(N?EQ_LEN|PREFIX)\(' &&             \
+         { echo '$(ME): use STREQ_LEN or STRPREFIX instead of str''ncmp' \
+               1>&2; exit 1; } || :
+
 # Override the default Cc: used in generating an announcement.
 announcement_Cc_ = $(translation_project_), \
   address@hidden, address@hidden
diff --git a/src/df.c b/src/df.c
index 357dca5..14f0790 100644
--- a/src/df.c
+++ b/src/df.c
@@ -611,9 +611,9 @@ get_dev (char const *disk, char const *mount_point,
               /* Don't print the first directory name in MOUNT_POINT if it's an
                  artifact of an automounter.  This is a bit too aggressive to 
be
                  the default.  */
-              if (strncmp ("/auto/", mount_point, 6) == 0)
+              if (STRNCMP_LIT (mount_point, "/auto/") == 0)
                 mount_point += 5;
-              else if (strncmp ("/tmp_mnt/", mount_point, 9) == 0)
+              else if (STRNCMP_LIT (mount_point, "/tmp_mnt/") == 0)
                 mount_point += 8;
 #endif
               cell = xstrdup (mount_point);
@@ -682,7 +682,7 @@ get_point (const char *point, const struct stat *statp)
           if (best_match_len <= len && len <= resolved_len
               && (len == 1 /* root file system */
                   || ((len == resolved_len || resolved[len] == '/')
-                      && strncmp (me->me_mountdir, resolved, len) == 0)))
+                      && STREQ_LEN (me->me_mountdir, resolved, len))))
             {
               best_match = me;
               best_match_len = len;
diff --git a/src/extent-scan.c b/src/extent-scan.c
index c0a5de6..4ddf46e 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -45,7 +45,7 @@ extent_need_sync (void)
       need_sync = 0; /* No workaround by default.  */

 #ifdef __linux__
-      if (uname (&name) != -1 && strncmp (name.release, "2.6.", 4) == 0)
+      if (uname (&name) != -1 && STRNCMP_LIT (name.release, "2.6.") == 0)
         {
            unsigned long val;
            if (xstrtoul (name.release + 4, NULL, 10, &val, NULL) == LONGINT_OK)
diff --git a/src/ls.c b/src/ls.c
index c09a913..a7ea8c2 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -1131,8 +1131,8 @@ is_colored (enum indicator_no type)
   size_t len = color_indicator[type].len;
   char const *s = color_indicator[type].string;
   return ! (len == 0
-            || (len == 1 && strncmp (s, "0", 1) == 0)
-            || (len == 2 && strncmp (s, "00", 2) == 0));
+            || (len == 1 && STRNCMP_LIT (s, "0") == 0)
+            || (len == 2 && STRNCMP_LIT (s, "00") == 0));
 }

 static void
@@ -1996,7 +1996,7 @@ decode_switches (int argc, char **argv)
         if (! (style = getenv ("TIME_STYLE")))
           style = bad_cast ("locale");

-      while (strncmp (style, posix_prefix, sizeof posix_prefix - 1) == 0)
+      while (STREQ_LEN (style, posix_prefix, sizeof posix_prefix - 1))
         {
           if (! hard_locale (LC_TIME))
             return optind;
@@ -2388,7 +2388,7 @@ parse_ls_color (void)
     }

   if (color_indicator[C_LINK].len == 6
-      && !strncmp (color_indicator[C_LINK].string, "target", 6))
+      && !STRNCMP_LIT (color_indicator[C_LINK].string, "target"))
     color_symlink_as_referent = true;
 }

@@ -4178,7 +4178,7 @@ print_color_indicator (const struct fileinfo *f, bool 
symlink_target)
         }
       else if (S_ISLNK (mode))
         type = ((!linkok
-                 && (!strncmp (color_indicator[C_LINK].string, "target", 6)
+                 && (!STRNCMP_LIT (color_indicator[C_LINK].string, "target")
                      || color_indicator[C_ORPHAN].string))
                 ? C_ORPHAN : C_LINK);
       else if (S_ISFIFO (mode))
@@ -4209,8 +4209,8 @@ print_color_indicator (const struct fileinfo *f, bool 
symlink_target)
       for (ext = color_ext_list; ext != NULL; ext = ext->next)
         {
           if (ext->ext.len <= len
-              && strncmp (name - ext->ext.len, ext->ext.string,
-                          ext->ext.len) == 0)
+              && STREQ_LEN (name - ext->ext.len, ext->ext.string,
+                            ext->ext.len))
             break;
         }
     }
diff --git a/src/md5sum.c b/src/md5sum.c
index 697689b..6f6e637 100644
--- a/src/md5sum.c
+++ b/src/md5sum.c
@@ -262,7 +262,7 @@ split_3 (char *s, size_t s_len,

   /* Check for BSD-style checksum line. */
   algo_name_len = strlen (DIGEST_TYPE_STRING);
-  if (strncmp (s + i, DIGEST_TYPE_STRING, algo_name_len) == 0)
+  if (STREQ_LEN (s + i, DIGEST_TYPE_STRING, algo_name_len))
     {
       if (s[i + algo_name_len] == ' ')
         ++i;
diff --git a/src/split.c b/src/split.c
index 364576a..3a630a0 100644
--- a/src/split.c
+++ b/src/split.c
@@ -895,12 +895,12 @@ main (int argc, char **argv)
           /* skip any whitespace */
           while (isspace (to_uchar (*optarg)))
             optarg++;
-          if (strncmp (optarg, "r/", 2) == 0)
+          if (STRNCMP_LIT (optarg, "r/") == 0)
             {
               split_type = type_rr;
               optarg += 2;
             }
-          else if (strncmp (optarg, "l/", 2) == 0)
+          else if (STRNCMP_LIT (optarg, "l/") == 0)
             {
               split_type = type_chunk_lines;
               optarg += 2;
diff --git a/src/system.h b/src/system.h
index b86e570..1c351bd 100644
--- a/src/system.h
+++ b/src/system.h
@@ -257,6 +257,13 @@ select_plural (uintmax_t n)
 }

 #define STREQ(a, b) (strcmp (a, b) == 0)
+#define STREQ_LEN(a, b, n) (strncmp (a, b, n) == 0)
+#define STRPREFIX(a, b) (strncmp(a, b, strlen (b)) == 0)
+
+/* Just like strncmp, but the first argument must be a literal string
+   and you don't specify the length.  */
+#define STRNCMP_LIT(s, literal) \
+  strncmp (s, "" literal "", sizeof (literal) - 1)

 #if !HAVE_DECL_GETLOGIN
 char *getlogin ();
@@ -607,7 +614,7 @@ emit_ancillary_info (void)
   /* Don't output this redundant message for English locales.
      Note we still output for 'C' so that it gets included in the man page.  */
   const char *lc_messages = setlocale (LC_MESSAGES, NULL);
-  if (lc_messages && strncmp (lc_messages, "en_", 3))
+  if (lc_messages && STRNCMP_LIT (lc_messages, "en_"))
     {
       /* TRANSLATORS: Replace LANG_CODE in this URL with your language code
          <http://translationproject.org/team/LANG_CODE.html> to form one of
diff --git a/src/tr.c b/src/tr.c
index 2809f87..2a729c6 100644
--- a/src/tr.c
+++ b/src/tr.c
@@ -548,7 +548,7 @@ look_up_char_class (char const *class_str, size_t len)
   enum Char_class i;

   for (i = 0; i < ARRAY_CARDINALITY (char_class_name); i++)
-    if (strncmp (class_str, char_class_name[i], len) == 0
+    if (STREQ_LEN (class_str, char_class_name[i], len)
         && strlen (char_class_name[i]) == len)
       return i;
   return CC_NO_CLASS;
diff --git a/src/uname.c b/src/uname.c
index 7ed1bc8..dd5de54 100644
--- a/src/uname.c
+++ b/src/uname.c
@@ -333,7 +333,7 @@ main (int argc, char **argv)

               /* Hack "safely" around the ppc vs. powerpc return value. */
               if (cputype == CPU_TYPE_POWERPC
-                  && strncmp (element, "ppc", 3) == 0)
+                  && STRNCMP_LIT (element, "ppc") == 0)
                 element = "powerpc";
             }
 # endif
diff --git a/src/who.c b/src/who.c
index 2a82b61..7449ab1 100644
--- a/src/who.c
+++ b/src/who.c
@@ -584,7 +584,7 @@ scan_entries (size_t n, const STRUCT_UTMP *utmp_buf)
       ttyname_b = ttyname (STDIN_FILENO);
       if (!ttyname_b)
         return;
-      if (strncmp (ttyname_b, DEV_DIR_WITH_TRAILING_SLASH, DEV_DIR_LEN) == 0)
+      if (STRNCMP_LIT (ttyname_b, DEV_DIR_WITH_TRAILING_SLASH) == 0)
         ttyname_b += DEV_DIR_LEN;      /* Discard /dev/ prefix.  */
     }

-- 
1.7.4.2.662.gcbd0