Replace fgetsx() by fgets(3) and fputsx() by fputs(3) #1056

alejandro-colomar · 2024-07-21T16:58:34Z

It seems they never worked correctly. Let's keep it simple and remove
support for escaped newlines.

Closes: #1055
Reported-by: @zeha

(Merge after #1048.)

Revisions:

v1b

Rebase

$ git range-diff c76f680fceb4^..gh/fgets string..fgets 
1:  c76f680f = 1:  d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  18db7b5a = 2:  4d66df60 lib/, src/: Consistently use NULL with fgets(3)
3:  b38c5eb0 = 3:  941c951c lib/, src/: Remove useless casts in fgets(3)
4:  2c112e69 = 4:  cd64d530 lib/, po/: Remove fgetsx() and fputsx()

v2

Use getline(3) instead of its pattern.

$ git range-diff string gh/fgets fgets 
1:  d0cdc9c8 = 1:  d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  4d66df60 = 2:  4d66df60 lib/, src/: Consistently use NULL with fgets(3)
3:  941c951c = 3:  941c951c lib/, src/: Remove useless casts in fgets(3)
4:  cd64d530 = 4:  cd64d530 lib/, po/: Remove fgetsx() and fputsx()
-:  -------- > 5:  e8036d0e lib/: Use getline(3) instead of its pattern

v2b

Remove unused include and macro

$ git range-diff string gh/fgets fgets 
1:  d0cdc9c8 = 1:  d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  4d66df60 = 2:  4d66df60 lib/, src/: Consistently use NULL with fgets(3)
3:  941c951c = 3:  941c951c lib/, src/: Remove useless casts in fgets(3)
4:  cd64d530 = 4:  cd64d530 lib/, po/: Remove fgetsx() and fputsx()
5:  e8036d0e ! 5:  0c59138d lib/: Use getline(3) instead of its pattern
    @@ Commit message
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## lib/commonio.c ##
    +@@
    + #include <utime.h>
    + 
    + #include "alloc/malloc.h"
    +-#include "alloc/reallocf.h"
    + #include "atoi/getnum.h"
    + #include "commonio.h"
    + #include "defines.h"
    +@@ lib/commonio.c: static void add_one_entry_nis (struct commonio_db *db,
    + }
    + #endif                            /* KEEP_NIS_AT_END */
    + 
    +-/* Initial buffer size, as well as increment if not sufficient
    +-   (for reading very long lines in group files).  */
    +-#define BUFLEN 4096
    + 
    +-int commonio_open (struct commonio_db *db, int mode)
    ++int
    ++commonio_open(struct commonio_db *db, int mode)
    + {
    +   char *buf;
    +   char *line;
     @@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
                return 0;
        }
    @@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
      
                line = strdup (buf);
                if (NULL == line) {
    +@@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
    +   return 0;
    + }
    + 
    ++
    + /*
    +  * Sort given db according to cmp function (usually compares uids)
    +  */
     
      ## lib/gshadow.c ##
     @@ lib/gshadow.c: void endsgent (void)

v3

Cosmetic in function declarator.
free(3) memory instead of keeping a static pointer. [@zeha]

$ git range-diff string gh/fgets fgets 
1:  d0cdc9c8 = 1:  d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  4d66df60 = 2:  4d66df60 lib/, src/: Consistently use NULL with fgets(3)
3:  941c951c = 3:  941c951c lib/, src/: Remove useless casts in fgets(3)
4:  cd64d530 = 4:  cd64d530 lib/, po/: Remove fgetsx() and fputsx()
5:  0c59138d ! 5:  decf456e lib/: Use getline(3) instead of its pattern
    @@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
     
      ## lib/gshadow.c ##
     @@ lib/gshadow.c: void endsgent (void)
    +   return &sgroup;
    + }
    + 
    ++
    + /*
    +  * fgetsgent - convert next line in stream to (struct sgrp)
    +  *
    +@@ lib/gshadow.c: void endsgent (void)
    +  * converts it to a (struct sgrp).  NULL is returned on EOF.
    +  */
    + 
    +-/*@observer@*//*@null@*/struct sgrp *fgetsgent (/*@null@*/FILE * fp)
    ++/*@observer@*//*@null@*/struct sgrp *
    ++fgetsgent(/*@null@*/FILE *fp)
    + {
        static size_t buflen = 0;
        static char *buf = NULL;
      
    @@ lib/gshadow.c: void endsgent (void)
        return (sgetsgent (buf));
      }
      
    ++
    + /*
    +  * getsgent - get a single shadow group entry
    +  */
-:  -------- > 6:  3e3b3743 lib/gshadow.c: fgetsgent(): Don't use static variables

v3b

Remove formatting fix that complicates diff.

$ git range-diff string gh/fgets fgets 
1:  d0cdc9c8 = 1:  d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  4d66df60 = 2:  4d66df60 lib/, src/: Consistently use NULL with fgets(3)
3:  941c951c = 3:  941c951c lib/, src/: Remove useless casts in fgets(3)
4:  cd64d530 = 4:  cd64d530 lib/, po/: Remove fgetsx() and fputsx()
5:  decf456e = 5:  decf456e lib/: Use getline(3) instead of its pattern
6:  3e3b3743 ! 6:  0936efac lib/gshadow.c: fgetsgent(): Don't use static variables
    @@ lib/gshadow.c: void endsgent (void)
     +  size_t       buflen = 0;
     +  struct sgrp  *sg;
      
    --  if (NULL == fp) {
    -+  if (NULL == fp)
    -           return NULL;
    --  }
    - 
    -   if (getline(&buf, &buflen, fp) == -1)
    +   if (NULL == fp) {
                return NULL;
    +@@ lib/gshadow.c: fgetsgent(/*@null@*/FILE *fp)
        if (stpsep(buf, "\n") == NULL)
                return NULL;

v4

Globally make the uses of sizeof() consistent.
Provide semantic patches where they can be used to validate the patches, in the corresponding commit message. [@thesamesam ]
Move the white-space and brace changes, to commits where it can be hidden with git-diff(1)'s -w and/or --color-words=., so that it doesn't hurt review.

$ git range-diff string gh/fgets fgets 
1:  d0cdc9c8 = 1:  d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
-:  -------- > 2:  8f77030f contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  941c951c ! 3:  b2b9bbb3 lib/, src/: Remove useless casts in fgets(3)
    @@ Metadata
      ## Commit message ##
         lib/, src/: Remove useless casts in fgets(3)
     
    +    This patch can be replicated with the following semantic patch:
    +
    +            $ cat fgets_cast.sp
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgets(a, (int) (b), c)
    +            + fgets(a, b, c)
    +
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgets(a, (int) b, c)
    +            + fgets(a, b, c)
    +
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgetsx(a, (int) (b), c)
    +            + fgetsx(a, b, c)
    +
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgetsx(a, (int) b, c)
    +            + fgetsx(a, b, c)
    +
    +            @@
    +            expression a, b, c, p;
    +            @@
    +
    +            - p->fgets(a, (int) (b), c)
    +            + p->fgets(a, b, c)
    +
    +            @@
    +            expression a, b, c, p;
    +            @@
    +
    +            - p->fgets(a, (int) b, c)
    +            + p->fgets(a, b, c)
    +
    +    which is applied as:
    +
    +            $ find lib* src/ -type f \
    +            | xargs spatch --sp-file ~/tmp/spatch/fgets_cast.sp --in-place;
    +
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## lib/commonio.c ##
    @@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
     -                  if (db->ops->fgets (buf + len,
     -                                      (int) (buflen - len),
     -                                      db->fp) == NULL) {
    -+                  if (db->ops->fgets(buf + len, buflen - len, db->fp) == NULL)
    ++                  if (db->ops->fgets(buf + len, buflen - len, db->fp) == NULL) {
                                goto cleanup_buf;
    --                  }
    +                   }
                }
    -           stpsep(buf, "\n");
    - 
     
      ## lib/gshadow.c ##
     @@ lib/gshadow.c: void endsgent (void)
                buflen *= 2;
      
                len = strlen (buf);
    --          if (fgetsx(&buf[len], (int) (buflen - len), fp) == NULL)
    -+          if (fgetsx(&buf[len], buflen - len, fp) == NULL)
    +-          if (fgetsx (&buf[len],
    +-                      (int) (buflen - len),
    +-                      fp) != &buf[len]) {
    ++          if (fgetsx(&buf[len], buflen - len, fp) != &buf[len]) {
                        return NULL;
    +           }
        }
    -   stpsep(buf, "\n");
     
      ## lib/setupenv.c ##
     @@ lib/setupenv.c: static void read_env_file (const char *filename)
        if (NULL == fp) {
                return;
        }
    --  while (fgets(buf, (int)(sizeof(buf)), fp) != NULL) {
    -+  while (fgets(buf, sizeof(buf), fp) != NULL) {
    +-  while (fgets(buf, (int) sizeof(buf), fp) == buf) {
    ++  while (fgets(buf, sizeof(buf), fp) == buf) {
                if (stpsep(buf, "\n") == NULL)
                        break;
      
    @@ src/chgpasswd.c: int main (int argc, char **argv)
         * group entry for each group will be looked up in the appropriate
         * file (gshadow or group) and the password changed.
         */
    --  while (fgets (buf, (int) sizeof buf, stdin) != NULL) {
    +-  while (fgets(buf, (int) sizeof(buf), stdin) != NULL) {
     +  while (fgets(buf, sizeof(buf), stdin) != NULL) {
                line++;
                if (stpsep(buf, "\n") == NULL) {
2:  4d66df60 ! 4:  00db7341 lib/, src/: Consistently use NULL with fgets(3)
    @@ Commit message
     
         <stddef.h> is the header that provides NULL; add it where appropriate.
     
    +    The meat of this patch can be approximated with the following semantic
    +    patch:
    +
    +            $ cat ~/tmp/spatch/fgets_null.sp
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgets(a, b, c) == a
    +            + fgets(a, b, c) != NULL
    +
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgetsx(a, b, c) == a
    +            + fgetsx(a, b, c) != NULL
    +
    +            @@
    +            expression a, b, c, p;
    +            @@
    +
    +            - p->fgets(a, b, c) == a
    +            + p->fgets(a, b, c) != NULL
    +
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgets(a, b, c) != a
    +            + fgets(a, b, c) == NULL
    +
    +            @@
    +            expression a, b, c;
    +            @@
    +
    +            - fgetsx(a, b, c) != a
    +            + fgetsx(a, b, c) == NULL
    +
    +            @@
    +            expression a, b, c, p;
    +            @@
    +
    +            - p->fgets(a, b, c) != a
    +            + p->fgets(a, b, c) == NUL
    +
    +    Applied as
    +
    +            $ find contrib/ lib* src/ -type f \
    +            | xargs spatch --sp-file ~/tmp/spatch/fgets_null.sp --in-place;
    +
    +    The differences between the actual patch and the approximation via the
    +    semantic patch from above are includes, whitespace, braces, and a case
    +    where there was an implicit pointer-to-bool comparison which I made
    +    explicit.  When reviewing, it'll be useful to use git-diff(1) with '-w'
    +    and '--color-words=.'.
    +
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## lib/commonio.c ##
    @@ lib/gshadow.c: void endsgent (void)
                buflen *= 2;
      
                len = strlen (buf);
    --          if (fgetsx (&buf[len],
    --                      (int) (buflen - len),
    --                      fp) != &buf[len]) {
    -+          if (fgetsx(&buf[len], (int) (buflen - len), fp) == NULL)
    +-          if (fgetsx(&buf[len], buflen - len, fp) != &buf[len]) {
    ++          if (fgetsx(&buf[len], buflen - len, fp) == NULL)
                        return NULL;
     -          }
        }
    @@ lib/hushed.c: bool hushed (const char *username)
        if (NULL == fp) {
                return false;
        }
    --  for (found = false; !found && (fgets (buf, sizeof buf, fp) == buf);) {
    +-  for (found = false; !found && (fgets(buf, sizeof(buf), fp) == buf);) {
     +  for (found = false; !found && (fgets(buf, sizeof(buf), fp) != NULL);) {
                stpsep(buf, "\n");
                found = (strcmp (buf, pw->pw_shell) == 0) ||
    @@ lib/loginprompt.c: login_prompt(char *name, int namesize)
         */
      
        MEMZERO(buf);
    --  if (fgets (buf, sizeof buf, stdin) != buf) {
    +-  if (fgets(buf, sizeof(buf), stdin) != buf) {
     +  if (fgets(buf, sizeof(buf), stdin) == NULL)
                exit (EXIT_FAILURE);
     -  }
    @@ lib/setupenv.c: static void read_env_file (const char *filename)
        if (NULL == fp) {
                return;
        }
    --  while (fgets (buf, (int)(sizeof buf), fp) == buf) {
    -+  while (fgets(buf, (int)(sizeof(buf)), fp) != NULL) {
    +-  while (fgets(buf, sizeof(buf), fp) == buf) {
    ++  while (fgets(buf, sizeof(buf), fp) != NULL) {
                if (stpsep(buf, "\n") == NULL)
                        break;
      
    @@ lib/ttytype.c: void ttytype (const char *line)
                        perror (typefile);
                return;
        }
    --  while (fgets (buf, sizeof buf, fp) == buf) {
    +-  while (fgets(buf, sizeof(buf), fp) == buf) {
     +  while (fgets(buf, sizeof(buf), fp) != NULL) {
                if (buf[0] == '#') {
                        continue;
                }
     
    + ## lib/user_busy.c ##
    +@@
    + #ident "$Id: $"
    + 
    + #include <assert.h>
    ++#include <stddef.h>
    + #include <stdio.h>
    + #include <sys/types.h>
    + #include <dirent.h>
    +@@ lib/user_busy.c: static int check_status (const char *name, const char *sname, uid_t uid)
    +   if (NULL == sfile) {
    +           return 0;
    +   }
    +-  while (fgets(line, sizeof(line), sfile) == line) {
    ++  while (fgets(line, sizeof(line), sfile) != NULL) {
    +           if (strncmp (line, "Uid:\t", 5) == 0) {
    +                   unsigned long ruid, euid, suid;
    + 
    +
      ## src/login_nopam.c ##
     @@ src/login_nopam.c: login_access(const char *user, const char *from)
        if (NULL != fp) {
                int lineno = 0; /* for diagnostics */
                while (   !match
    --                 && (fgets (line, sizeof (line), fp) == line))
    +-                 && (fgets(line, sizeof(line), fp) == line))
     +                 && (fgets(line, sizeof(line), fp) != NULL))
                {
                        char  *p;
    @@ src/useradd.c: get_defaults(void)
         * Read the file a line at a time. Only the lines that have relevant
         * values are used, everything else can be ignored.
         */
    --  while (fgets (buf, sizeof buf, fp) == buf) {
    +-  while (fgets(buf, sizeof(buf), fp) == buf) {
     +  while (fgets(buf, sizeof(buf), fp) != NULL) {
                stpsep(buf, "\n");
      
    @@ src/useradd.c: set_defaults(void)
                goto skip;
        }
      
    --  while (fgets (buf, sizeof buf, ifp) == buf) {
    +-  while (fgets(buf, sizeof(buf), ifp) == buf) {
     +  while (fgets(buf, sizeof(buf), ifp) != NULL) {
                char  *val;
      
4:  cd64d530 ! 5:  c1ccbb9a lib/, po/: Remove fgetsx() and fputsx()
    @@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
                                goto cleanup_errno;
      
                        len = strlen (buf);
    --                  if (db->ops->fgets(buf + len, buflen - len, db->fp) == NULL)
    +-                  if (db->ops->fgets(buf + len, buflen - len, db->fp) == NULL) {
     +                  if (fgets(buf + len, buflen - len, db->fp) == NULL)
                                goto cleanup_buf;
    +-                  }
                }
                stpsep(buf, "\n");
    + 
     @@ lib/commonio.c: static int write_all (const struct commonio_db *db)
                                return -1;
                        }
                } else if (NULL != p->line) {
     -                  if (db->ops->fputs (p->line, db->fp) == EOF) {
    -+                  if (fputs(p->line, db->fp) == EOF) {
    ++                  if (fputs(p->line, db->fp) == EOF)
    +                           return -1;
    +-                  }
    ++
    +                   if (putc ('\n', db->fp) == EOF) {
                                return -1;
                        }
    -                   if (putc ('\n', db->fp) == EOF) {
     
      ## lib/commonio.h ##
     @@ lib/commonio.h: struct commonio_ops {
5:  decf456e = 6:  9991189d lib/: Use getline(3) instead of its pattern
6:  0936efac = 7:  51005793 lib/gshadow.c: fgetsgent(): Don't use static variables

v4b

Rebase

$ git range-diff d0cdc9c8f683^..gh/fgets string..fgets 
1:  d0cdc9c8 = 1:  8dbc8149 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  8f77030f ! 2:  ac57816a contrib/, lib/, src/: Consistently use sizeof() as if it were a function
    @@ Commit message
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## contrib/adduser.c ##
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
          printf ("\nLogin to add (^C to quit): ");
          fflush (stdout);
      
    @@ contrib/adduser.c: main(void)
      
          if (!strlen (usrname))
            {
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      
            printf ("\nFull Name [%s]: ", usrname);
            fflush (stdout);
    @@ contrib/adduser.c: main(void)
          strcpy (person, usrname);
        };
      
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
              bad = 0;
              printf ("GID [%d]: ", DEFAULT_GROUP);
              fflush (stdout);
    @@ contrib/adduser.c: main(void)
              if (!strlen (foo))
                group = DEFAULT_GROUP;
              else if (isdigit (*foo))
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
          printf ("\nIf home dir ends with a / then '%s' will be appended to it\n", usrname);
          printf ("Home Directory [%s/%s]: ", DEFAULT_HOME, usrname);
          fflush (stdout);
    @@ contrib/adduser.c: main(void)
          if (!strlen(dir))  /* hit return */
            sprintf(dir, "%s/%s", DEFAULT_HOME, usrname);
          else if (dir[strlen (dir) - 1] == '/')
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      
            printf ("\nShell [%s]: ", DEFAULT_SHELL);
            fflush (stdout);
    @@ contrib/adduser.c: main(void)
            if (!strlen (shell))
        strcpy(shell, DEFAULT_SHELL);
            else
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      #endif
          printf ("\nMin. Password Change Days [%d]: ", DEFAULT_MIN_PASS);
          fflush (stdout);
    @@ contrib/adduser.c: main(void)
          if (strlen (foo) > 1)
            min_pass = DEFAULT_MIN_PASS;
          else
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      
          printf ("Max. Password Change Days [%d]: ", DEFAULT_MAX_PASS);
          fflush (stdout);
    @@ contrib/adduser.c: main(void)
          if (strlen (foo) > 1)
            max_pass = atoi (foo);
          else
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      
          printf ("Password Warning Days [%d]: ", DEFAULT_WARN_PASS);
          fflush (stdout);
    @@ contrib/adduser.c: main(void)
          warn_pass = atoi (foo);
          if (warn_pass == 0)
      
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      
          printf ("Days after Password Expiry for Account Locking [%d]: ", DEFAULT_USER_DIE);
          fflush (stdout);
    @@ contrib/adduser.c: main(void)
          user_die = atoi (foo);
          if (user_die == 0)
            user_die = DEFAULT_USER_DIE;
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
              min_pass, max_pass, warn_pass, user_die);
            printf ("\nIs this correct? [y/N]: ");
            fflush (stdout);
     -      safeget (foo, sizeof (foo));
     +      safeget(foo, sizeof(foo));
      
    -       done = bad = correct = !!strchr("yY", foo[0]);
    +       done = bad = correct = (foo[0] == 'y' || foo[0] == 'Y');
      
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      
        *environ = NULL;
      
    @@ contrib/adduser.c: main(void)
        sprintf (cmd, "%s -g %d -d %s -s %s -c \"%s\" -m -k /etc/skel %s",
           USERADD_PATH, group, dir, shell, person, usrname);
        printf ("Calling useradd to add new user:\n%s\n", cmd);
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
                         */
        setuid (0);
      
    @@ contrib/adduser.c: main(void)
      
        /* Chage runs suid root. => we need ruid root to run it with
         * anything other than chage -l
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
        /* I want to add a user completely with one easy command --chris */
      
      #ifdef HAVE_QUOTAS
    @@ contrib/adduser.c: main(void)
        sprintf (cmd, "%s -p %s -u %s", EDQUOTA_PATH, QUOTA_DEFAULT, usrname);
        printf ("%s\n", cmd);
        if (system (cmd))
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
          printf ("\nDefault quota set.\n");
      #endif /* HAVE_QUOTAS */
      
    @@ contrib/adduser.c: main(void)
        sprintf (cmd, "%s %s", PASSWD_PATH, usrname);
        if (system (cmd))
          {
    -@@ contrib/adduser.c: main(void)
    +@@ contrib/adduser.c: main (void)
      #endif
          }
      #ifdef IMMEDIATE_CHANGE
    @@ src/lastlog.c: static void update_one (/*@null@*/const struct passwd *pw)
                                 Prog, (unsigned long)pw->pw_uid);
     
      ## src/login.c ##
    -@@ src/login.c: main(int argc, char **argv)
    +@@ src/login.c: int main (int argc, char **argv)
                unsigned int  failcount = 0;
      
                /* Make the login prompt look like we want it */
    @@ src/login.c: main(int argc, char **argv)
                        SNPRINTF(loginprompt, _("%s login: "), hostn);
                } else {
                        STRTCPY(loginprompt, _("login: "));
    -@@ src/login.c: main(int argc, char **argv)
    +@@ src/login.c: int main (int argc, char **argv)
      #ifdef HAVE_LL_HOST               /* __linux__ || SUN4 */
                        if ('\0' != ll.ll_host[0]) {
                                printf (_(" from %.*s"),
3:  b2b9bbb3 = 3:  9fdffc48 lib/, src/: Remove useless casts in fgets(3)
4:  00db7341 = 4:  d46fd02e lib/, src/: Consistently use NULL with fgets(3)
5:  c1ccbb9a = 5:  1721920a lib/, po/: Remove fgetsx() and fputsx()
6:  9991189d = 6:  664817b3 lib/: Use getline(3) instead of its pattern
7:  51005793 = 7:  a71fc6f3 lib/gshadow.c: fgetsgent(): Don't use static variables

v4c

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  8dbc8149 = 1:  8af482a6 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  ac57816a = 2:  7cae4284 contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  9fdffc48 = 3:  410f2492 lib/, src/: Remove useless casts in fgets(3)
4:  d46fd02e = 4:  a0953493 lib/, src/: Consistently use NULL with fgets(3)
5:  1721920a = 5:  9abcea85 lib/, po/: Remove fgetsx() and fputsx()
6:  664817b3 = 6:  dc3819ff lib/: Use getline(3) instead of its pattern
7:  a71fc6f3 = 7:  e49bcf8b lib/gshadow.c: fgetsgent(): Don't use static variables

v4d

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  8af482a6 = 1:  b4a37bc9 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  7cae4284 = 2:  d1abe1ac contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  410f2492 = 3:  f388b921 lib/, src/: Remove useless casts in fgets(3)
4:  a0953493 = 4:  31c6c72f lib/, src/: Consistently use NULL with fgets(3)
5:  9abcea85 = 5:  bf0a81e6 lib/, po/: Remove fgetsx() and fputsx()
6:  dc3819ff = 6:  bbbae6af lib/: Use getline(3) instead of its pattern
7:  e49bcf8b = 7:  f7d5cc12 lib/gshadow.c: fgetsgent(): Don't use static variables

v4e

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  b4a37bc9 = 1:  be73a591 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  d1abe1ac = 2:  219af975 contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  f388b921 = 3:  691673bc lib/, src/: Remove useless casts in fgets(3)
4:  31c6c72f = 4:  7cb79af6 lib/, src/: Consistently use NULL with fgets(3)
5:  bf0a81e6 = 5:  364b354d lib/, po/: Remove fgetsx() and fputsx()
6:  bbbae6af = 6:  3f398748 lib/: Use getline(3) instead of its pattern
7:  f7d5cc12 = 7:  ef91817e lib/gshadow.c: fgetsgent(): Don't use static variables

v4f

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  be73a591 = 1:  5684988b lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  219af975 = 2:  022f2da0 contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  691673bc = 3:  05fe7f89 lib/, src/: Remove useless casts in fgets(3)
4:  7cb79af6 = 4:  5991bd9c lib/, src/: Consistently use NULL with fgets(3)
5:  364b354d = 5:  5cf10040 lib/, po/: Remove fgetsx() and fputsx()
6:  3f398748 = 6:  7e3d5d58 lib/: Use getline(3) instead of its pattern
7:  ef91817e = 7:  27dd9f5c lib/gshadow.c: fgetsgent(): Don't use static variables

v4g

Rebase

$ git range-diff 5684988b^..gh/fgets string..fgets 
1:  5684988b = 1:  77e6a686 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  022f2da0 = 2:  c42d9568 contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  05fe7f89 = 3:  b67527a6 lib/, src/: Remove useless casts in fgets(3)
4:  5991bd9c = 4:  87af89d8 lib/, src/: Consistently use NULL with fgets(3)
5:  5cf10040 = 5:  375d57a0 lib/, po/: Remove fgetsx() and fputsx()
6:  7e3d5d58 = 6:  d094dc5d lib/: Use getline(3) instead of its pattern
7:  27dd9f5c = 7:  8ef8fc73 lib/gshadow.c: fgetsgent(): Don't use static variables

v4h

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  77e6a686 = 1:  8a8c6635 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  c42d9568 ! 2:  da61e23a contrib/, lib/, src/: Consistently use sizeof() as if it were a function
    @@ lib/tz.c
        if (   (NULL == fp)
     -      || (fgets (tzbuf, sizeof (tzbuf), fp) == NULL)) {
     +      || (fgets(tzbuf, sizeof(tzbuf), fp) == NULL)) {
    -           def_tz = getdef_str ("ENV_TZ");
    -           if ((NULL == def_tz) || ('/' == def_tz[0])) {
    -                   def_tz = "TZ=CST6CDT";
    +           result = "TZ=CST6CDT";
    +   } else {
    +           stpsep(tzbuf, "\n");
     
      ## lib/user_busy.c ##
     @@ lib/user_busy.c: user_busy_utmp(const char *name)
    @@ src/useradd.c: static void lastlog_reset (uid_t uid)
                return;
        }
        if (   (lseek (fd, offset_uid, SEEK_SET) != offset_uid)
    --      || (write_full (fd, &ll, sizeof (ll)) != (ssize_t) sizeof (ll))
    -+      || (write_full(fd, &ll, sizeof(ll)) != (ssize_t) sizeof(ll))
    +-      || (write_full (fd, &ll, sizeof (ll)) == -1)
    ++      || (write_full(fd, &ll, sizeof(ll)) == -1)
            || (fsync (fd) != 0)) {
                fprintf (stderr,
                         _("%s: failed to reset the lastlog entry of UID %lu: %s\n"),
3:  b67527a6 = 3:  9f934b2a lib/, src/: Remove useless casts in fgets(3)
4:  87af89d8 = 4:  8a8620eb lib/, src/: Consistently use NULL with fgets(3)
5:  375d57a0 = 5:  2e696e77 lib/, po/: Remove fgetsx() and fputsx()
6:  d094dc5d = 6:  2375cbfc lib/: Use getline(3) instead of its pattern
7:  8ef8fc73 = 7:  7d2a356c lib/gshadow.c: fgetsgent(): Don't use static variables

v4i

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  8a8c6635 = 1:  c6dabf04 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  da61e23a = 2:  6d57727a contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  9f934b2a = 3:  857da077 lib/, src/: Remove useless casts in fgets(3)
4:  8a8620eb = 4:  db2b027d lib/, src/: Consistently use NULL with fgets(3)
5:  2e696e77 = 5:  c8645b33 lib/, po/: Remove fgetsx() and fputsx()
6:  2375cbfc = 6:  8d9b55dd lib/: Use getline(3) instead of its pattern
7:  7d2a356c = 7:  b5663560 lib/gshadow.c: fgetsgent(): Don't use static variables

v4j

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  c6dabf04 = 1:  2ff6e318 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  6d57727a = 2:  6faa0944 contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  857da077 = 3:  ee315ddc lib/, src/: Remove useless casts in fgets(3)
4:  db2b027d = 4:  876c36d1 lib/, src/: Consistently use NULL with fgets(3)
5:  c8645b33 = 5:  5fc868e2 lib/, po/: Remove fgetsx() and fputsx()
6:  8d9b55dd = 6:  f7ac2b0a lib/: Use getline(3) instead of its pattern
7:  b5663560 = 7:  9fd4a0b3 lib/gshadow.c: fgetsgent(): Don't use static variables

v4k

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  2ff6e318 = 1:  2b6a1413 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  6faa0944 ! 2:  61bc9a04 contrib/, lib/, src/: Consistently use sizeof() as if it were a function
    @@ lib/log.c: void dolastlog (
      
     
      ## lib/loginprompt.c ##
    -@@ lib/loginprompt.c: login_prompt(char *name, int namesize)
    +@@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
                        (void) fclose (fp);
                }
        }
    @@ lib/loginprompt.c: login_prompt(char *name, int namesize)
        printf (_("\n%s login: "), buf);
        (void) fflush (stdout);
      
    -@@ lib/loginprompt.c: login_prompt(char *name, int namesize)
    +@@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
         */
      
        MEMZERO(buf);
3:  ee315ddc = 3:  ca165ccf lib/, src/: Remove useless casts in fgets(3)
4:  876c36d1 ! 4:  8929e11e lib/, src/: Consistently use NULL with fgets(3)
    @@ lib/loginprompt.c
      
      #include "attr.h"
      #include "defines.h"
    -@@ lib/loginprompt.c: login_prompt(char *name, int namesize)
    +@@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
         */
      
        MEMZERO(buf);
5:  5fc868e2 = 5:  fee111ca lib/, po/: Remove fgetsx() and fputsx()
6:  f7ac2b0a = 6:  08d1534f lib/: Use getline(3) instead of its pattern
7:  9fd4a0b3 = 7:  b6b6a6b5 lib/gshadow.c: fgetsgent(): Don't use static variables

v4l

Rebase

$ git range-diff gh/string..gh/fgets string..fgets 
1:  2b6a1413 = 1:  ea9f29ec lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  61bc9a04 = 2:  e628440c contrib/, lib/, src/: Consistently use sizeof() as if it were a function
3:  ca165ccf = 3:  dec51b42 lib/, src/: Remove useless casts in fgets(3)
4:  8929e11e = 4:  e63fef58 lib/, src/: Consistently use NULL with fgets(3)
5:  fee111ca = 5:  5fe14748 lib/, po/: Remove fgetsx() and fputsx()
6:  08d1534f = 6:  0dc3bf45 lib/: Use getline(3) instead of its pattern
7:  b6b6a6b5 = 7:  d3278c53 lib/gshadow.c: fgetsgent(): Don't use static variables

v5

Several rebases

$ git range-diff string..fgets gh/string..gh/fgets 
1:  ea9f29ec = 1:  bbde7826 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
2:  e628440c ! 2:  e1d0706a contrib/, lib/, src/: Consistently use sizeof() as if it were a function
    @@ lib/getdef.c: struct itemdef {
      static struct itemdef def_table[] = {
        {"CHFN_RESTRICT", NULL},
        {"CONSOLE_GROUPS", NULL},
    -@@ lib/getdef.c: static struct itemdef def_table[] = {
    -   {NULL, NULL}
    - };
    - 
    --#define NUMKNOWNDEFS      (sizeof(knowndef_table)/sizeof(knowndef_table[0]))
    -+#define NUMKNOWNDEFS  (sizeof(knowndef_table) / sizeof(knowndef_table[0]))
    - static struct itemdef knowndef_table[] = {
    - #ifdef USE_PAM
    -   PAMDEFS
     @@ lib/getdef.c: static void def_load (void)
        /*
         * Go through all of the lines in the file.
    @@ lib/log.c: void dolastlog (
      
     
      ## lib/loginprompt.c ##
    -@@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
    +@@ lib/loginprompt.c: login_prompt(char *name, int namesize)
                        (void) fclose (fp);
                }
        }
    @@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
        printf (_("\n%s login: "), buf);
        (void) fflush (stdout);
      
    -@@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
    +@@ lib/loginprompt.c: login_prompt(char *name, int namesize)
         */
      
        MEMZERO(buf);
    @@ lib/subordinateio.c: subordinate_parse(const char *line)
        strcpy (rangebuf, line);
      
     
    - ## lib/tcbfuncs.c ##
    -@@ lib/tcbfuncs.c: static /*@null@*/ char *shadowtcb_path_rel_existing (const char *name)
    -           free (path);
    -           return NULL;
    -   }
    --  ret = readlink (path, link, sizeof (link) - 1);
    -+  ret = readlink(path, link, sizeof(link) - 1);
    -   if (-1 == ret) {
    -           fprintf (shadow_logfd,
    -                    _("%s: Cannot read symbolic link %s: %s\n"),
    -
      ## lib/ttytype.c ##
     @@ lib/ttytype.c: void ttytype (const char *line)
                        perror (typefile);
    @@ lib/user_busy.c: user_busy_utmp(const char *name)
                        continue;
                }
                if (kill (utent->ut_pid, 0) != 0) {
    -@@ lib/user_busy.c: static int different_namespace (const char *sname)
    - 
    -   SNPRINTF(path, "/proc/%s/ns/user", sname);
    - 
    --  if ((llen1 = readlink (path, buf, sizeof(buf))) == -1)
    -+  if ((llen1 = readlink(path, buf, sizeof(buf))) == -1)
    -           return 0;
    - 
    --  if ((llen2 = readlink ("/proc/self/ns/user", buf2, sizeof(buf2))) == -1)
    -+  if ((llen2 = readlink("/proc/self/ns/user", buf2, sizeof(buf2))) == -1)
    -           return 0;
    - 
    -   if (llen1 == llen2 && memcmp (buf, buf2, llen1) == 0)
     @@ lib/user_busy.c: static int check_status (const char *name, const char *sname, uid_t uid)
        if (NULL == sfile) {
                return 0;
3:  dec51b42 = 3:  1cf6b14d lib/, src/: Remove useless casts in fgets(3)
4:  e63fef58 ! 4:  3fc6e9ed lib/, src/: Consistently use NULL with fgets(3)
    @@ lib/loginprompt.c
      
      #include "attr.h"
      #include "defines.h"
    -@@ lib/loginprompt.c: void login_prompt (char *name, int namesize)
    +@@ lib/loginprompt.c: login_prompt(char *name, int namesize)
         */
      
        MEMZERO(buf);
5:  5fe14748 ! 5:  79adfd9d lib/, po/: Remove fgetsx() and fputsx()
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        find_new_sub_gids.c \
        find_new_sub_uids.c \
     -  fputsx.c \
    -   get_pid.c \
    -   getdate.h \
    -   getdate.y \
    +   fs/readlink/areadlink.c \
    +   fs/readlink/areadlink.h \
    +   fs/readlink/readlinknul.c \
     
      ## lib/commonio.c ##
     @@ lib/commonio.c: int commonio_open (struct commonio_db *db, int mode)
6:  0dc3bf45 = 6:  5bac295b lib/: Use getline(3) instead of its pattern
7:  d3278c53 = 7:  b3acc480 lib/gshadow.c: fgetsgent(): Don't use static variables

alejandro-colomar · 2024-07-22T10:51:16Z

lib/gshadow.c

-	if (fgetsx(buf, buflen, fp) == buf) {
-		while (   ((cp = strrchr (buf, '\n')) == NULL)
-		       && (feof (fp) == 0)) {
-			size_t len;
-
-			cp = REALLOC(buf, buflen * 2, char);
-			if (NULL == cp) {
-				return NULL;
-			}
-			buf = cp;
-			buflen *= 2;
-
-			len = strlen (buf);
-			if (fgetsx (&buf[len],
-			            (int) (buflen - len),
-			            fp) != &buf[len]) {
-				return NULL;
-			}
+	if (fgets(buf, buflen, fp) == NULL)
+		return NULL;
+
+	while (   (strrchr(buf, '\n') == NULL)
+	       && (feof (fp) == 0)) {
+		size_t len;
+
+		cp = REALLOC(buf, buflen * 2, char);
+		if (NULL == cp) {
+			return NULL;
 		}
-		stpsep(buf, "\n");
-		return (sgetsgent (buf));
+		buf = cp;
+		buflen *= 2;
+
+		len = strlen (buf);
+		if (fgets(&buf[len], buflen - len, fp) == NULL)
+			return NULL;
 	}
-	return NULL;
+	stpsep(buf, "\n");
+	return (sgetsgent (buf));


@thesamesam

Hi Sam,

It might be worth commenting this here.

Here's an overview of the branch:

$ git log --oneline master..fgets cd64d530 (HEAD -> fgets, gh/fgets) lib/, po/: Remove fgetsx() and fputsx() 941c951c lib/, src/: Remove useless casts in fgets(3) 4d66df60 lib/, src/: Consistently use NULL with fgets(3) d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation a719bc39 (gh/string, string) lib/fields.c: Remove dead code b71d2236 contrib/, lib/, src/: Use consistent style using strchr(3) in conditionals 44df1083 lib/, src/: Use str[n]cmp(3) instead of explicit byte comparisons d3350b67 contrib/, lib/, src/: Use strchr(3) to compact comparisons 9a6e3284 lib/loginprompt.c: login_prompt(): Use strtcpy() instead of its pattern db93a222 lib/, src/: Use strsep(3) instead of strtok(3) 376e4166 src/suauth.c: check_su_auth(): Use pointers to simplify ae1c284e src/suauth.c: check_su_auth(): Use strspn(3) instead of its pattern 93362cc3 lib/gshadow.c: endsgent(): Remove dead assignment 5a46723a lib/port.c: portcmp(): Use strcmp(3) instead of its pattern 7442a127 lib/, src/: Use stpspn() instead of its pattern

This fgets branch is really only

$ git log --oneline string..fgets cd64d530 (HEAD -> fgets, gh/fgets) lib/, po/: Remove fgetsx() and fputsx() 941c951c lib/, src/: Remove useless casts in fgets(3) 4d66df60 lib/, src/: Consistently use NULL with fgets(3) d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation

but I based it on string, which corresponds to PR #1048.

Here's why. Basically, according to the bug report that it closes, the PR is about replacing fgetsx => fgets. Here are two of the calls that we replace:

$ grep -rnC4 fgetsx lib/gshadow.c 154- if (NULL == fp) { 155- return NULL; 156- } 157- 158: if (fgetsx(buf, buflen, fp) == buf) { 159- while ( ((cp = strrchr (buf, '\n')) == NULL) 160- && (feof (fp) == 0)) { 161- size_t len; 162- -- 167- buf = cp; 168- buflen *= 2; 169- 170- len = strlen (buf); 171: if (fgetsx (&buf[len], 172- (int) (buflen - len), 173- fp) != &buf[len]) { 174- return NULL; 175- }

I want to fully understand what the code is doing before doing the change, which makes me more confident that the change is correct. To that end, I need to reduce superfluous complexity.

In the code above, you may notice a dead assignment:

159- while ( ((cp = strrchr (buf, '\n')) == NULL)

I already had removed that dead assignment in #1048 in commit 93362cc. That's the only reason why I decided to base it on that PR.

Now let's see why this PR has 4 new commits, instead of 1.

Line

158: if (fgetsx(buf, buflen, fp) == buf) {

Has a conditional that spans the entire rest of the function. Commit 1 turns this into the usual thing, which is an early return on error.

Then we get to

171: if (fgetsx (&buf[len], 172- (int) (buflen - len), 173- fp) != &buf[len]) {

Why do we have that cast? It's nonsense. I'll remove it.
And also &buf[len] is very brittle, and a small typo could add a bug here. Let's use NULL there.

While doing those two changes, I'll check any other places that do similarly bad stuff, and replace it in the entire code base. Then commit 4 does what we originally wanted.

I don't reason too much the first 3 commits, which you might find frustrating while reviewing the changes a posteriori. I didn't because they're not the main purpose of the PR; they're just trivial cosmetic changes in preparation for the PR. And it's usual to apply cosmetic patches as the first patches in a patch set, to prepare for the important ones.

Do you have any suggestions for making it easier for you to review the changes later?

Maybe I could add an explicit [cosmetic] in the second line of the commit message, to mark those as unimportant commits.

Still, I need to apply those cosmetic changes, or it would be much more difficult to me to reason about the important changes of the PR.

And these cosmetic patches combined with the change fgetsx=>fgets have allowed me to see a transformation fgets=>getline too. I probably wouldn't have been able to see it without the cosmetic changes.

Did you consider my coccinelle suggestion?

I don't know how to use it. If you point me to some docs and/or show me a small shell example session showing how it's useful (the example will help more than the docs, TBH), I'll be happy to try it.

Hmmm. https://coccinelle.gitlabpages.inria.fr/website/sp.html. Sounds like it could be interesting.

That's why I mentioned it before ;)

I think it's both a good fit for the work you're interested in and eases review of it.

I'm trying to develop a coccinelle script that would do something similar to 941c951

So far, I've written

$ cat fgets_cast.sp @@ expression a, b, c; type t; @@ - fgets(a, (t) (b), c) + fgets(a, b, c) @@ expression a, b, c; type t; @@ - fgets(a, (t) b, c) + fgets(a, b, c) @@ expression a, b, c; type t; @@ - fgetsx(a, (t) (b), c) + fgetsx(a, b, c) @@ expression a, b, c; type t; @@ - fgetsx(a, (t) b, c) + fgetsx(a, b, c) @@ expression a, b, c, p; type t; @@ - p->fgets(a, (t) (b), c) + p->fgets(a, b, c) @@ expression a, b, c, p; type t; @@ - p->fgets(a, (t) b, c) + p->fgets(a, b, c)

and I'm running it as

$ find lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/fgets_cast.sp --in-place;

but it results in false positives, such as

diff --git i/lib/shadow.c w/lib/shadow.c index 44436836..5759f259 100644 --- i/lib/shadow.c +++ w/lib/shadow.c @@ -202,7 +202,7 @@ struct spwd *fgetspent (FILE * fp) return (0); } - if (fgets (buf, sizeof buf, fp) != NULL) + if (fgets(buf, sizeof buf, fp) != NULL) { stpsep(buf, "\n"); return my_sgetspent (buf);

How is that matching (t), if there are no casts?

lib/gshadow.c

thesamesam · 2024-07-22T20:38:05Z

lib/loginprompt.c

@@ -81,9 +84,8 @@ void login_prompt (char *name, int namesize)
 	 */

 	MEMZERO(buf);
-	if (fgets (buf, sizeof buf, stdin) != buf) {
+	if (fgets(buf, sizeof(buf), stdin) == NULL)


It would be helpful if formatting changes weren't mixed in.

I try to keep those to a minimum: only whitespace and (sometimes) parentheses. And only on hunks where it (subjectively, admittedly) doesn't hurt too much readability (i.e., if I'm doing some huge transformations where I need your attention somewhere, I'll try to not do it there.

I'll keep in mind your concern.

thesamesam · 2024-07-22T20:39:13Z

lib/gshadow.c

 /*
 * fgetsgent - convert next line in stream to (struct sgrp)
 *
 * fgetsgent() reads the next line from the provided stream and
 * converts it to a (struct sgrp).  NULL is returned on EOF.
 */

-/*@observer@*//*@null@*/struct sgrp *fgetsgent (/*@null@*/FILE * fp)
+/*@observer@*//*@null@*/struct sgrp *
+fgetsgent(/*@null@*/FILE *fp)


Is this null stuff really needed? Can't we use the attribute for it?

I added some patches for reducing those comments some time ago, and replaced some of them by attributes and qualifiers

I should have another look at those at some point, and send another round of patches.

thesamesam · 2024-07-22T20:39:33Z

lib/fields.c

@@ -66,7 +68,8 @@ int valid_field (const char *field, const char *illegal)
 * prompt the user with the name of the field being changed and the
 * current value.
 */
-void change_field (char *buf, size_t maxsize, const char *prompt)
+void


Really, if you must do this, can't you just do a huge clang-format pass?

Hmm, I'll have a look at using it. Thanks for the suggestion.

thesamesam · 2024-07-22T20:49:11Z

I really feel this PR is still pretty noisy.

alejandro-colomar · 2024-07-23T21:41:04Z

contrib/adduser.c

-  bzero (cmd, sizeof (cmd));
+  bzero(cmd, sizeof(cmd));


@thesamesam

If you apply the semantic patch accompanied with the corresponding commit message, and then git diff -w against the actual commit, you should be able to ignore these unrelated whitespace changes. I've written recommendations for how to better review each commit.

I hope that helps reviewing, while at the same time allowing me to apply some house style while doing the changes.

strsep(3) is stateless, and so is easier to reason about. It also has a slight difference: strtok(3) jumps over empty fields, while strsep(3) respects them as empty fields. In most of the cases where we were using strtok(3), it makes more sense to respect empty fields, and this commit probably silently fixes a few bugs. In other cases (most notably filesystem paths), contiguous delimiters ("//") should be collapsed, so strtok(3) still makes more sense there. This commit doesn't replace such strtok(3) calls. Signed-off-by: Alejandro Colomar <[email protected]>

This is simpler to read, IMO. Signed-off-by: Alejandro Colomar <[email protected]>

…onals For negative matches, use if (strchr(...) == NULL) For positive matches, use the cast-to-bool operator: if (!!strchr(...)) For positive matches, when a variable is also set, use while (NULL != (p = strchr(...))) Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

sizeof(foo) - No spaces. Not: sizeof (foo) - Parentheses. Not: sizeof foo - No parentheses wrapping sizeof itself Not: (sizeof foo) This patch can be approximated with the following semantic patch: $ cat ~/tmp/spatch/sizeof.sp @@ identifier a, b; @@ - sizeof a->b + sizeof(a->b) @@ identifier a, b; @@ - sizeof a.b + sizeof(a.b) @@ identifier x; @@ - sizeof x + sizeof(x) @@ identifier x; @@ - sizeof *x + sizeof(*x) @@ identifier x; @@ - (sizeof(x)) + sizeof(x) @@ identifier x; @@ - (sizeof(*x)) + sizeof(*x) Applied as $ find contrib/ lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/sizeof.sp --in-place; The differences between the actual patch and the approximation via the semantic patch from above are whitespace only. When reviewing, it'll be useful to diff with '-w'. Link: <https://lkml.org/lkml/2012/7/11/103> Signed-off-by: Alejandro Colomar <[email protected]>

This patch can be replicated with the following semantic patch: $ cat fgets_cast.sp @@ expression a, b, c; @@ - fgets(a, (int) (b), c) + fgets(a, b, c) @@ expression a, b, c; @@ - fgets(a, (int) b, c) + fgets(a, b, c) @@ expression a, b, c; @@ - fgetsx(a, (int) (b), c) + fgetsx(a, b, c) @@ expression a, b, c; @@ - fgetsx(a, (int) b, c) + fgetsx(a, b, c) @@ expression a, b, c, p; @@ - p->fgets(a, (int) (b), c) + p->fgets(a, b, c) @@ expression a, b, c, p; @@ - p->fgets(a, (int) b, c) + p->fgets(a, b, c) which is applied as: $ find lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/fgets_cast.sp --in-place; Signed-off-by: Alejandro Colomar <[email protected]>

fgets(3) returns either NULL or the input pointer. Checking for NULL is more explicit, and simpler. <stddef.h> is the header that provides NULL; add it where appropriate. The meat of this patch can be approximated with the following semantic patch: $ cat ~/tmp/spatch/fgets_null.sp @@ expression a, b, c; @@ - fgets(a, b, c) == a + fgets(a, b, c) != NULL @@ expression a, b, c; @@ - fgetsx(a, b, c) == a + fgetsx(a, b, c) != NULL @@ expression a, b, c, p; @@ - p->fgets(a, b, c) == a + p->fgets(a, b, c) != NULL @@ expression a, b, c; @@ - fgets(a, b, c) != a + fgets(a, b, c) == NULL @@ expression a, b, c; @@ - fgetsx(a, b, c) != a + fgetsx(a, b, c) == NULL @@ expression a, b, c, p; @@ - p->fgets(a, b, c) != a + p->fgets(a, b, c) == NUL Applied as $ find contrib/ lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/fgets_null.sp --in-place; The differences between the actual patch and the approximation via the semantic patch from above are includes, whitespace, braces, and a case where there was an implicit pointer-to-bool comparison which I made explicit. When reviewing, it'll be useful to use git-diff(1) with '-w' and '--color-words=.'. Signed-off-by: Alejandro Colomar <[email protected]>

It seems they never worked correctly. Let's keep it simple and remove support for escaped newlines. Closes: <shadow-maint#1055> Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar marked this pull request as ready for review July 21, 2024 17:01

alejandro-colomar requested review from hallyn and ikerexxe July 21, 2024 17:01

alejandro-colomar marked this pull request as draft July 21, 2024 17:01

alejandro-colomar force-pushed the fgets branch from 2c112e6 to cd64d53 Compare July 21, 2024 17:34

alejandro-colomar commented Jul 22, 2024

View reviewed changes

alejandro-colomar force-pushed the fgets branch from e8036d0 to 0c59138 Compare July 22, 2024 16:39

zeha reviewed Jul 22, 2024

View reviewed changes

lib/gshadow.c Show resolved Hide resolved

alejandro-colomar force-pushed the fgets branch from 0c59138 to 3e3b374 Compare July 22, 2024 17:36

thesamesam reviewed Jul 22, 2024

View reviewed changes

alejandro-colomar force-pushed the fgets branch 2 times, most recently from 0936efa to 5100579 Compare July 23, 2024 21:32

alejandro-colomar commented Jul 23, 2024

View reviewed changes

alejandro-colomar force-pushed the fgets branch from 5100579 to a71fc6f Compare July 30, 2024 16:19

alejandro-colomar force-pushed the fgets branch from a71fc6f to e49bcf8 Compare August 12, 2024 00:27

alejandro-colomar force-pushed the fgets branch 3 times, most recently from ef91817 to 27dd9f5 Compare September 1, 2024 13:51

alejandro-colomar force-pushed the fgets branch 5 times, most recently from 9fd4a0b to b6b6a6b Compare October 15, 2024 10:08

alejandro-colomar force-pushed the fgets branch 3 times, most recently from d1874d8 to c817cb6 Compare November 1, 2024 20:29

alejandro-colomar added 3 commits November 2, 2024 11:07

lib/, src/: Use str[n]cmp(3) instead of explicit byte comparisons

5b14222

This is simpler to read, IMO. Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar added 7 commits November 2, 2024 11:07

lib/gshadow.c: endsgent(): Invert logic to reduce indentation

bbde782

Signed-off-by: Alejandro Colomar <[email protected]>

lib/, po/: Remove fgetsx() and fputsx()

79adfd9

It seems they never worked correctly. Let's keep it simple and remove support for escaped newlines. Closes: <shadow-maint#1055> Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

lib/: Use getline(3) instead of its pattern

5bac295

Signed-off-by: Alejandro Colomar <[email protected]>

lib/gshadow.c: fgetsgent(): Don't use static variables

b3acc48

Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar force-pushed the fgets branch from c817cb6 to b3acc48 Compare November 2, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace fgetsx() by fgets(3) and fputsx() by fputs(3) #1056

Replace fgetsx() by fgets(3) and fputsx() by fputs(3) #1056

alejandro-colomar commented Jul 21, 2024 •

edited

Loading

alejandro-colomar Jul 22, 2024 •

edited

Loading

alejandro-colomar Jul 22, 2024

thesamesam Jul 22, 2024

alejandro-colomar Jul 22, 2024 •

edited

Loading

alejandro-colomar Jul 22, 2024

thesamesam Jul 23, 2024

alejandro-colomar Jul 23, 2024 •

edited

Loading

thesamesam Jul 22, 2024

alejandro-colomar Jul 22, 2024 •

edited

Loading

thesamesam Jul 22, 2024

alejandro-colomar Jul 22, 2024

thesamesam Jul 22, 2024

alejandro-colomar Jul 22, 2024

thesamesam commented Jul 22, 2024

alejandro-colomar Jul 23, 2024

Replace fgetsx() by fgets(3) and fputsx() by fputs(3) #1056

Are you sure you want to change the base?

Replace fgetsx() by fgets(3) and fputsx() by fputs(3) #1056

Conversation

alejandro-colomar commented Jul 21, 2024 • edited Loading

alejandro-colomar Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandro-colomar Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandro-colomar Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandro-colomar Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesamesam commented Jul 22, 2024

Choose a reason for hiding this comment

alejandro-colomar commented Jul 21, 2024 •

edited

Loading

alejandro-colomar Jul 22, 2024 •

edited

Loading

alejandro-colomar Jul 22, 2024 •

edited

Loading

alejandro-colomar Jul 23, 2024 •

edited

Loading

alejandro-colomar Jul 22, 2024 •

edited

Loading