[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Prasad,

You are assuming the type of the function random() which is normally
provided by <stdlib.h> AND you are NOT seeding it by calling srandom().
You do call srand() which might be useful if you were going to call
rand() rather than random()...

You would probably do better to look at main.c for how to come up with
another sizeof (long int) bytes of random number instead of using
the random() function in this manner.

You should not include a declaration for is_admin() in passwd.h, but
instead should ensure that subr.h is included as it is in cvs.h and just
ensure that the ordering of cvs.h works to get the definitions you need
in place. I am also not certain why you have chosen to move is_admin()
into passwd.c without removing it from subr.c ...

The passwd.c file should

#include <config.h>

before it includes "cvs.h"

The function alias() has the potential to be shadowed by the mkmodules.c
local variable of the same name. Some compilers warn about shadowing in
this manner. I suggest that alias() should either be a static function
confined to passwd.c or it should be called something else.

In fact, there are a great many functions defined in passwd.h as globals
that should probably not be there at all and should instead be static
functions that appear only in passwd.c

I do not understand why you need your own linked list package in
passwd.c ... Could you use the list processing in hash.c instead?
getlist(), getnode(), addnode(), delnode(), freenode(), insert_before(),
dellist(), et al? I won't worry too much about it right now as what you
have seems simple enough and might even be correct.

It may be desirable to consider unifying the password reading operation
used by login.c and passwd.c rather than having two separate sets of
routines that basically do the same thing.

I see a '#define SIZE 128' in passwd.c but no uses of it anywhere.
Remove it.

Use of BUFLEN and fgets() is a bad idea. Please consider using getline()
as an alternative. It allows for arbitrarily long lines.

You have a similar problem with read_pass().

I did not look closely. How does gettok() differ from strtok() ?

The sanity.sh stuff is still a mess. If you feel you MUST have tests that
rely upon $PWCVSROOT, you should not fail if that variable is not properly
set, instead print out the tests you are skipping. However, you really
should still exercise the server side protocol for doing the various
'pw' 'pw -a username' 'pw -r username othername' 'pw username' 'pw -R user'
'pw -x user' 'pw -X user' situations in EXACTLY the same manner as the
'pserver' tests

            cat >$CVSROOT_DIRNAME/CVSROOT/passwd <<EOF
testme:q6WV9d2t848B2:$username
dontroot:q6WV9d2t848B2:root
anonymous::$username
$username:
willfail:   :whocares
EOF

        # NOTE THIS next test shows that the passwd command does not
        # properly deal with bogus arguments. This is a bug that should
        # be fixed... the problem appears that spasswd can send garbage
        # to the descramble() function if that is what the user
        # provides. This is a bad thing.

            # Tets how the passwd command works
            dotest_fail passwd-1 \
"$servercvs --allow-root=${CVSROOT_DIRNAME} pserver" \
"$DOTSTAR LOVE YOU
error  unrecognized request `'
E Terminated with fatal signal 11
# Core dumped; preserving $DOTSTAR on server\.
E CVS locks may need cleaning up\.
error" <<EOF
BEGIN AUTH REQUEST
$CVSROOT_DIRNAME
testme
Ay::'d
END AUTH REQUEST
Root $CVSROOT_DIRNAME
passwd

EOF

Yes, I know that there are supposed to be arguments to the passwd
command. However, your server needs to deal with all possibilities
of bogus input from a remote client.

        Later,
        -- Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFFFFUgCg7APGsDnFERAkEZAKCLsOeozfYaOyrbgsla/VdOkJnSOgCfeguo
cl2VIZjqcFKkEJ+y/OVxRos=
=69F4
-----END PGP SIGNATURE-----