[PATCH] cvsserver: Don't send mixed messages to clients

[PATCH] cvsserver: Don't send mixed messages to clients

[Frank Lichtenheld]

After we send I HATE YOU we should probably exit and not happily
continue with I LOVE YOU and further communication.

Most clients will probably just exit and ignore everything we
send after the I HATE YOU and it is not a security problem
either because we don't really care about the user name anyway.

But it is still the right thing to do.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 git-cvsserver.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index a07c725..fcfb99d 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -105,6 +105,7 @@ if (@ARGV && $ARGV[0] eq 'pserver') {
     unless ($line eq 'anonymous') {
        print "E Only anonymous user allowed via pserver\n";
        print "I HATE YOU\n";
+       exit 1;
     }
     $line = <STDIN>; chomp $line;    # validate the password?
     $line = <STDIN>; chomp $line;
-- 
1.5.1.4

Re: [PATCH] cvsserver: Don't send mixed messages to clients

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> After looking through all the uses of
> 	die "something"
> and
> 	print "E something";
> 	exit;
> and
> 	print "error 1 something";
> 	exit;
>
> in the current code I fail to see a real pattern there, is there any?
> If yes, we should maybe document it in a comment somewhere...
>
> For the record, cvs (server side) does the following in this particular situation:
> 	buf_output0 (buf_to_net, "I HATE YOU\n");
>         buf_flush (buf_to_net, true);
>
>         /* Don't worry about server_cleanup, server_active isn't set
>            yet.  */
>         exit (EXIT_FAILURE);

Ok, I stand corrected.  If there is a pattern to emulate, that
should be found in the real cvs server.

Re: [PATCH] cvsserver: Don't send mixed messages to clients

[Frank Lichtenheld]

On Mon, May 14, 2007 at 02:28:29PM -0700, Junio C Hamano wrote:
> Frank Lichtenheld <frank@lichtenheld.de> writes:
> > For the record, cvs (server side) does the following in this particular situation:
> > 	buf_output0 (buf_to_net, "I HATE YOU\n");
> >         buf_flush (buf_to_net, true);
> >
> >         /* Don't worry about server_cleanup, server_active isn't set
> >            yet.  */
> >         exit (EXIT_FAILURE);
> 
> Ok, I stand corrected.  If there is a pattern to emulate, that
> should be found in the real cvs server.

Will you correct the patch then back to my version? master still has
your version of it AFAICT.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] cvsserver: Don't send mixed messages to clients

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> On Mon, May 14, 2007 at 02:28:29PM -0700, Junio C Hamano wrote:
>> Frank Lichtenheld <frank@lichtenheld.de> writes:
>> > For the record, cvs (server side) does the following in this particular situation:
>> > 	buf_output0 (buf_to_net, "I HATE YOU\n");
>> >         buf_flush (buf_to_net, true);
>> >
>> >         /* Don't worry about server_cleanup, server_active isn't set
>> >            yet.  */
>> >         exit (EXIT_FAILURE);
>> 
>> Ok, I stand corrected.  If there is a pattern to emulate, that
>> should be found in the real cvs server.
>
> Will you correct the patch then back to my version? master still has
> your version of it AFAICT.

Will do, but then we would also have to vet all the error exit
paths to match what the real cvsserver does, wouldn't we?

We've survived without that so far, so while I'll apply the
above change before 1.5.2, I think we do not have to have the
release wait for fixes to the other error exit paths.

Re: [PATCH] cvsserver: Don't send mixed messages to clients

[Frank Lichtenheld]

On Sat, May 19, 2007 at 05:51:35PM -0700, Junio C Hamano wrote:
> Frank Lichtenheld <frank@lichtenheld.de> writes:
> 
> > On Mon, May 14, 2007 at 02:28:29PM -0700, Junio C Hamano wrote:
> >> Frank Lichtenheld <frank@lichtenheld.de> writes:
> >> > For the record, cvs (server side) does the following in this particular situation:
> >> > 	buf_output0 (buf_to_net, "I HATE YOU\n");
> >> >         buf_flush (buf_to_net, true);
> >> >
> >> >         /* Don't worry about server_cleanup, server_active isn't set
> >> >            yet.  */
> >> >         exit (EXIT_FAILURE);
> >> 
> >> Ok, I stand corrected.  If there is a pattern to emulate, that
> >> should be found in the real cvs server.
> >
> > Will you correct the patch then back to my version? master still has
> > your version of it AFAICT.
> 
> Will do, but then we would also have to vet all the error exit
> paths to match what the real cvsserver does, wouldn't we?

Sure. But it is nothing utterly important. It just so happens
that I already have a test for this one case and would like to
see it fixed before submitting the test patches again.

> We've survived without that so far, so while I'll apply the
> above change before 1.5.2, I think we do not have to have the
> release wait for fixes to the other error exit paths.

Agreed.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/