[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

P J P <address@hidden> writes:

> On Wed, 11 Oct 2006, Mark D. Baushke wrote:
> > Yes, you are correct. I would rather not see a patch for any derived
> > object. You have convinced me.
> 
>    Hello Mark,
> 
> I've uploaded the *new* cvs-passwd patch at:
> 
> http://www.cdacbangalore.in/~prasad/tools/cvs-passwd.tar.bz2

Congratulations, this version of the patch applied cleanly to the
top-of-tree sources. Now, if you had only managed to actually USE some
of the patches I already sent to you the last time, I would not have to
repeat them again this time.

Comments per entry in the cvs-passwd.patch included below.

 - I am mostly ignoring the documentation for this pass through the patch.
   So, no comments about ChangeLog entries or doc/*.texi files. If someone
   else has time to read the changes, that would be great.
 
 - configure and configure.in
   You didn't bother to incorporate my fix to your configure.in file to
   use a tab instead of four spaces for the flockfile and funlockfile
   ditto for the change to configure.
 
 - NEWS
   Looks okay.
 
 - cvs.h
   Has a whitespace change that is unnecessary. There are no useful changes 
   to this file. Revert what you have done.
 
 - hash.h
   Looks okay.

 - main.c
   I would rather see the CVS_CMD_MODIFIES_REPOSITORY on the previous
   line with the "passwd" command to be consistent with the other entries
   in the table. This is a minor formatting change.

 - Makefile.am
   Looks okay.

 - Makefile.in
   This is normally a derived object, generated by autoreconf, but it
   looks like you gerated the change by hand due to the whitespace
   changes.

 - passwd.c ... formatting content
   * Missing a Copyright (C) 2006 ... line
   * 'uses vim: tabstop(ts) = 4, shiftwidth(sw) = 4' which may be
     useful for development, but should probably be removed before 
     production.
   * You still have trailing whitespace in the file. This is
     undesirable in a new file.
   * You should have two empty lines between functions on the same page.
     so adding a newline before the /* check_option: ... */ comment is
     desirable. Ditto before /* exit_pass: ... */ and /* encrypt_pass: ...*/
     and /* generate_salt: ... */ and before /* gettok: ... */ and
     /* write_pass: ... */ and /* insert: ... */
   * change_pass (). s/if(!tmp)/if (!tmp)/

 - passwd.c implementation
   * check_option
     You should not just assign NULL to rusr and dpass. Instead you should
     free any memory that might be in those pointers first. It is entirely
     possible for a conforming implementation to send more than one
     passwd command through the client/server connection OR to have more
     than one option on the command line.
   * passwd () I still do not see a check for 
     'supported_request ("passwd-encrypt")' or whatever it is to be called,
     before you allow the -e flag to be sent to the server. I think this
     has been mentioned in every code review so far. It is quite frustrating
     that you have done nothing about this to date. And yes, this means
     adding more the server.c file in aid of it. I believe this is only
     needed for the -e switch at present.
   * passwd () leaks rusr and dpass memory.
   * FYI. The CVSNT synonyms for "passwd" are "password" and "setpass"
     Options:
       -a         Add user. Adds a new user entry to the password file.
       -x         Disable user. Changes the password so that the user
                  cannot log in.
       -X         Delete user. Remove the user entry from the password file.
       -r user    Alias username to real system user. Before a virtual
                  (pserver) user can log in the system needs to know which user
                  account to use for that user.
       -R         Remove the system alias for user.
       -D domain  (Win32 only) Use the users' domain password instead of a
                  separate password. For security reasons this is not
                  recommended.
   * spasswd ()
     You need to return some kind of an error if you are not processing
     the '-D domain' request passed by a CVS/CVSNT client.
   * spasswd () leaks rusr and dpass memory.
   * init_pass () and exit_pass ()
     You really should check the return value from fclose (pass)
     and report problems that may have arisen due to filesystem troubles
     reading or writing the file.
   ... I ran out of time to continue the review with exit_pass ()
   ... I'll have to find time later to do more.

 - sanity.sh
   * As I sent in my last patch to you, the tests="${tests} passwd"
     line needs to match the indentation of the rest of the lines above it.
   * In many of your tests, you have the cd $testdir and other lines
     improperly indented. I believe I also fixed those problems in the
     last pastch I sent you. You seem to have ignored my patch back to
     you. I do not understand why you are refusing help to get this right.
   * '"${DOTSTAR} LOVE YOU' is wrong. It should be '"${DOTSTAR}I LOVE YOU'
     in all of those cases.
   * The hint about writeprox-0 was that you should FIX that test. It is
     having problems because you have added "passwd" to the REQ_LINE in
     server.c and have not properly updated the sanity.sh tests which
     are impacted by your change.
   * I have not tried to compile or run your new sanity.sh

 - server.c
   * You are still missing the extra REQ_LINE to indicate that you
     support encrypted passwords via the -e switch to the passwd
     command. How many times do I need to mention this is a problem?


> I just tried to create the minimalist patch; It does not patch any
> derived files, but the essential ones only. When run '$ make check',
> it fails at the 'writeproxy-0' test, as you pointed out earlier.
> Please try this also.

Why should I try it? You broke it when you added a new feature, so you
get to fix it...

> Thank you! :)
> -- 
> regards
>     -P J P
> PS: Please don't send me html/attachment/Fwd mails

What follows is a patch to your passwd.c to correct some, but not all of
the problems observed so far. (I didn't touch the missing
supported_request ("passwd-encrypt") mechanism that is missing. I also
did not add a Copyright statement for your code.)

Please consider incoporating these changes into your sources.

--- passwd.c~   Thu Oct 12 11:30:53 2006
+++ passwd.c    Thu Oct 12 12:05:59 2006
@@ -6,7 +6,7 @@
  * This is to facilitate user to change her cvs password from client side using
  * cvs command line interface.
  *
- * Permission is granted to use, modify, and redistribute this patch under the 
+ * Permission is granted to use, modify, and redistribute this patch under the
  * terms of GNU Gereral Public Licence as published by Free Software 
Foundation;
  * either version 2 of the licence, or (at your option) any later version.
  *
@@ -24,7 +24,7 @@
 
 #ifdef CVS_ADMIN_GROUP
 #   include <grp.h>
-#endif    
+#endif
 
 # if !HAVE_DECL_FLOCKFILE
 #  undef flockfile
@@ -47,18 +47,19 @@ static void alias (const char *, const c
 static int  gettok (const char *, char, char **);
 static void change_pass (const char *, const char *);
 
-enum op_mode { PASSWD = 0, ADD = 1, DISABLE = 2, 
+enum op_mode { PASSWD = 0, ADD = 1, DISABLE = 2,
                DELETE = 4, ALIAS = 8, RALIAS = 16 };
 static enum op_mode mode;
 static bool do_encrypt = false, tokset = false;
 static const char pass_file[] = "/CVSROOT/passwd";
 
 static List *pass_list;         /* password record list */
-static char *rusr, *dpass;      /* remote system user, domain password */
+/* remote system user, domain password */
+static char *rusr = NULL, *dpass = NULL;
 static char *cmdname;
 static char salt_set[] = "./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ" \
                          "abcdefghijklmnopqrstuvwxyz";
-                         
+
 
 static const char *const passwd_usage[] =
 {
@@ -75,6 +76,7 @@ static const char *const passwd_usage[] 
     NULL
 };
 
+
 /* check_option: check the command line options */
 static int
 check_option (int argc, char *argv[])
@@ -86,10 +88,9 @@ check_option (int argc, char *argv[])
 
     mode = PASSWD;
     opterr = optind = 0;
-    rusr = dpass = NULL;
     do_encrypt = tokset = false;
     while ((c = getopt (argc, argv, "+aD:er:RxX")) != -1)
-    {        
+    {
         switch (c)
         {
         case 'a':   /* add user */
@@ -97,14 +98,18 @@ check_option (int argc, char *argv[])
             break;
 
         case 'D':   /* use domain password */
+            if (dpass)
+                free (dpass);
             dpass = xstrdup (optarg);
             break;
 
         case 'e':   /* do local encrypt */
             do_encrypt = true;
             break;
-        
+
         case 'r':   /* (remote) real system user */
+            if (rusr)
+                free (rusr);
             rusr = xstrdup (optarg);
             mode |= ALIAS;
             break;
@@ -126,14 +131,14 @@ check_option (int argc, char *argv[])
             usage (passwd_usage);
         }
     }
-    
+
     return optind;
 }
 
 
-/* 
+/*
  * passwd: main function called by cvs-client, is supposed to read new password
- * from user and pass it on to cvs-server.    
+ * from user and pass it on to cvs-server.
  */
 int
 passwd (int argc, char *argv[])
@@ -141,18 +146,18 @@ passwd (int argc, char *argv[])
     int ret = 0, n = 0;
     char *key = NULL, *msg = NULL, *pass = NULL, *usr = NULL;
 
-    cmdname = argv[0];    
+    cmdname = argv[0];
     n = check_option (argc, argv);
     argc -= n;
     argv += n;
     if (argc)
         usr = argv[0];
 
-#ifdef CLIENT_SUPPORT    
-    if (current_parsed_root && current_parsed_root->isremote)    
+#ifdef CLIENT_SUPPORT
+    if (current_parsed_root && current_parsed_root->isremote)
     {
         start_server ();
-    
+
         if (!supported_request ("passwd"))
             error (1, 0, "server does not support %s", cmdname);
         if (!usr && (mode & ADD || mode & DISABLE || mode & DELETE
@@ -164,7 +169,7 @@ passwd (int argc, char *argv[])
         if (dpass != NULL)
             option_with_arg ("-D", dpass);
         if (do_encrypt)
-            send_arg ("-e");    
+            send_arg ("-e");
         if (mode & ALIAS)
             option_with_arg ("-r", rusr);   /* make alias */
         if (mode & RALIAS)
@@ -173,7 +178,7 @@ passwd (int argc, char *argv[])
             send_arg ("-x");    /* disable user */
         if (mode & DELETE)
             send_arg ("-X");    /* delete user */
-        if (!mode)  
+        if (!mode)
         {                       /* change password */
             usr = (usr == NULL) ? current_parsed_root->username : usr;
             printf ("Changing password for %s\n", usr);
@@ -202,13 +207,23 @@ passwd (int argc, char *argv[])
         ret = get_responses_and_close ();
     }
 
+    if (rusr)
+    {
+       free (rusr);
+       rusr = NULL;
+    }
+    if (dpass)
+    {
+       free (dpass);
+       dpass = NULL;
+    }
     if (pass) free (pass);
     if (current_parsed_root->password)
     {
         free (current_parsed_root->password);
         current_parsed_root->password = NULL;
     }
-#endif    
+#endif
 
     return ret;
 }
@@ -216,8 +231,8 @@ passwd (int argc, char *argv[])
 
 /*
  * spasswd: This is called by cvs server, is supposed to receive username,
- * password, etc. from cvs-client and update appropriately the CVSROOT/passwd 
- * file. It receives the user credentials as command line arguments in the 
+ * password, etc. from cvs-client and update appropriately the CVSROOT/passwd
+ * file. It receives the user credentials as command line arguments in the
  * following format
  * cvs server passwd [-a] [-D domain] [-e] [-r user] [-R] [-x] [-X]
  *                   [username] [passwd]
@@ -241,6 +256,9 @@ spasswd (int argc, char *argv[])
         || mode & DELETE) && !is_admin ())
         error (1, 0, "only cvs admin can do that: permission denied!");
 
+    if (dpass)
+       error (1, 0, "domain passwords are not supported by this server.");
+
     init_pass ();
     if (mode & ADD)
     {                            /* -a add user */
@@ -260,7 +278,7 @@ spasswd (int argc, char *argv[])
     {
         tmp = findnode (pass_list, usr);
         delnode (tmp);           /* -X delete user */
-    }    
+    }
     if (!mode)
     {                            /* change password */
         if (strcmp (usr, CVS_Username) && !is_admin ())
@@ -270,9 +288,19 @@ spasswd (int argc, char *argv[])
         str = descramble (argv[1]);
         pass = (do_encrypt == false) ? encrypt_pass (str) : str;
         change_pass (usr, pass);
-    }    
+    }
     exit_pass ();
-    
+
+    if (rusr)
+    {
+       free (rusr);
+       rusr = NULL;
+    }
+    if (dpass)
+    {
+       free (dpass);
+       dpass = NULL;
+    }
     return 0;
 }
 
@@ -296,6 +324,7 @@ init_pass (void)
     free (fname);
 }
 
+
 /* exit_pass: write password list to the file and release the list */
 static void
 exit_pass (void)
@@ -315,6 +344,7 @@ exit_pass (void)
     free (fname);
 }
 
+
 /* encrypt_pass: encrypt the given password using md5 based algorithm */
 static char *
 encrypt_pass (const char *pass)
@@ -327,12 +357,13 @@ encrypt_pass (const char *pass)
     if (!(ret = crypt (pass, salt)))
         error (1, errno, "encryption failed");
 
-    return ret;    
+    return ret;
 }
 
+
 /*
- * generate_salt: generate an 11 character salt, to be used by crypt(3) for 
- * encryption purpose     
+ * generate_salt: generate an 11 character salt, to be used by crypt(3) for
+ * encryption purpose
  */
 static void
 generate_salt(char *psalt)
@@ -342,7 +373,7 @@ generate_salt(char *psalt)
     time_t tt;
 
     assert (psalt != NULL);
-    
+
     tt = time (&tt);
     srandom (tt * getpid ());
     psalt[i++] = '$';
@@ -371,10 +402,10 @@ read_new_passwd (void)
         if (strlen (pass) < MIN_LEN)
         {
             error (0, 0, "password too short");
-            free (pass); 
+            free (pass);
             pass = NULL;
             continue;
-        }    
+        }
         pass = xstrdup (pass);
         cpass = getpass ("Retype new CVS password: ");
         if (!cpass)
@@ -386,7 +417,7 @@ read_new_passwd (void)
             error (0, 0, "passwords do not match");
             free (pass);
             pass = NULL;
-        }    
+        }
     }
     if (!flag && !pass)
     {
@@ -399,7 +430,7 @@ read_new_passwd (void)
 
 
 /*
- * change_pass: change the user password in password list created by 
+ * change_pass: change the user password in password list created by
  * read_pass
  */
 static void
@@ -409,11 +440,11 @@ change_pass (const char *usr, const char
     Node *tmp = NULL;
     char *lusr = NULL, *rec = NULL;
 
-    assert (usr != NULL); 
+    assert (usr != NULL);
     assert (pass != NULL);
 
     tmp = findnode (pass_list, usr);
-    if(!tmp)
+    if (!tmp)
         error (1, 0, "user '%s' not found", usr);
 
     /*
@@ -474,6 +505,7 @@ alias (const char *usr, const char *als)
         free (pass);
 }
 
+
 /*
  * gettok: returns a token into 'ret', from given string 's', comprising of
  * characters up to next occurrence of 'delim' character in 's'. also returns
@@ -483,7 +515,7 @@ alias (const char *usr, const char *als)
 static int
 gettok (const char *s, char delim, char **ret)
 {
-    int j = 0; 
+    int j = 0;
     char buf[BUFLEN];   /* BUFLEN = 512 */
     static int i = 0;
 
@@ -508,12 +540,12 @@ gettok (const char *s, char delim, char 
 
 /*
  * read_pass: reads the CVSROOT/passwd file and build a linked list of
- * records.        
+ * records.
  */
 static int
 read_pass (FILE *fp)
 {
-    size_t len = 0; 
+    size_t len = 0;
     char *buf = NULL, *str = NULL;
 
     assert (fp != NULL);
@@ -529,28 +561,30 @@ read_pass (FILE *fp)
     return 0;
 }
 
+
 /* write_pass: write all user credentials to CVSROOT/passwd file */
 static int
 write_pass (FILE *fp)
 {
     Node *i = NULL, *head = pass_list->list;
-    
+
     assert (fp != NULL);
-        
+
     for (i = head->next; i != head; i = i->next)
         fputs (i->data, fp);
 
     return 0;
 }
 
+
 /* insert: insert new rerord in password list    */
 static int
 insert (const char *str)
 {
     Node *tmp = NULL;
-    
+
     assert (str != NULL);
-    
+
     tmp = getnode ();
     tmp->type = PASSWDNODE;
     gettok (str, ':', &tmp->key);
@@ -558,7 +592,7 @@ insert (const char *str)
     tmp->len = strlen (str);
     addnode (pass_list, tmp);
     tokset = true;
-    
+
     return 0;
 }