[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

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

P J P <address@hidden> writes:

> On Fri, 13 Oct 2006, Mark D. Baushke wrote:
> > Consider the case where a client connects to the server and sends one
> > passwd command which completes and then before disconnecting from your
> > server, sends another passwd command. Yes, the current CVS client does
> > not do this. However, the protocol allows a client to do this should it
> > so desire, so your server code needs to be paranoid about the
> > possibility.
> 
>    Hmmmn, now I understand; I've fixed it. Thanks for the explanation!

Sure. 

btw: My last patch to your passwd.c file suggested a possible method.
Basically, doing an if (dpass) free (dpass); before any dpass assignment
and likewise for rusr and also making sure to free that memory before
the end of the passwd() and spasswd() functions.

Please note that the 'static char *dpass = NULL;' in my patch is not
really needed for a true ANSI C program. A static pointer will be
initialized to NULL in any case, so it is not required to do that
initialization. I just do it to avoid warnings with some compilers that
should know better, but I have not seen such a warning in a long time.

> >      * generate_salt()
> >        I see you are still using srandom() and random(). You do know
> >     that this is not a very good random number source right? It is
> >     also not available on all platforms that CVS uses.
> >
> >        You may find this of interest:
> >     http://www.delorie.com/djgpp/v2faq/faq22_23.html
> 
>    I'm going through it!

Portability is a big deal and djg has some good advice on it.

> >      * alias()
> >        Generally, "user '%s' not found" should be "user `%s' not found"
> >     that is, use "`" (0x60 grave accent) for the opening quote and "'"
> >     (0x27 single quote) as the closing quote.
> 
>     done!

There were multiple occurances of the '%s' in your various error ()
calls which will also need to be fixed.

I am curious about one thing which you can help me understand...
in spasswd(), when '-x disable user' is used, you send a " " to
the change_pass() function. This would seem to insert a space into
the passwd file for that user. Where are the semantics of a space
defined to be disabled? The CVSNT folks seem to write "#DISABLED#"
in the case where -x is used to disable an account.

I also do not see any discussion of either " " in the doc/cvs.texi file,
but perhaps I missed it somehow?

> >      * gettok
> >     I believe this code:
> >
> >            *ret = malloc (j + 1);
> >            strcpy (*ret, buf);
> >
> >     should be replaced with
> >
> >         *ret = xstrdup (buf);
> 
>     done!
> 
> 
> >      * read_pass () & write_pass () & insert ()
> >        Please advise why these functions always returns 0. If there is
> >     never a non-zero return, why not just make them void functions?
> 
>     hmmn, you are right! I think, it was just out of habit, I use to
> 'return -1;', if 'fopen()' or any such call fails. I'll change them,
> thank you!

I see no problem with you choosing to return the return code of the
fopen() and return from read_pass() and write_pass(), but in that case,
you would also wish to check the reults of those function calls where
they are used.

I was just confused by the apparent contradiction in your
implementation.

        -- Mark

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

iD8DBQFFL314Cg7APGsDnFERAmzTAKDW5H2+bMxTdcF+4aboBO8yO52rmQCZAS7g
rlpVn0f6IlQ4g4zknSz+1Z4=
=Ic5W
-----END PGP SIGNATURE-----