threads / patch / 8132

patchcvsserver: Don't send mixed messages to clients

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

## tl;dr

5 messages between May 13, 2007 and May 20, 2007. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Frank Lichtenheld· May 13, 2007, 17:49 UTC · lore

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(-)
Show changes to git-cvsserver.perl +1 −0
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
Junio C Hamano· May 14, 2007, 21:28 UTC · lore

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

Frank Lichtenheld <frank@lichtenheld.de> writes:
Show 19 quoted lines
> 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.

Frank Lichtenheld· May 20, 2007, 00:45 UTC · re: Junio C Hamano · lore

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

On Mon, May 14, 2007 at 02:28:29PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 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/
Junio C Hamano· May 20, 2007, 00:51 UTC · re: Frank Lichtenheld · lore

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

Frank Lichtenheld <frank@lichtenheld.de> writes:
Show 15 quoted lines
> 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.

Frank Lichtenheld· May 20, 2007, 01:10 UTC · re: Junio C Hamano · lore

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

On Sat, May 19, 2007 at 05:51:35PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> 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/

← back to recent threads