Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland <johan@herland.net>]

On Wednesday 08 July 2009, Johannes Sixt wrote:
> Johan Herland schrieb:
> > +	/* If rev-list --stdin encounters an unknown commit, it
> > terminates, +	 * which will cause SIGPIPE in the write loop below.
> > */
>
> Under the conditions you describe here...
>
> > +	signal(SIGPIPE, SIG_IGN);
>
> ... and SIGPIPE being ignored...
>
> > +
> > +	err = start_command(&revlist);
> > +	if (err) {
> > +		error("could not run rev-list");
> > +		return err;
> > +	}
> >
> > -	for (i = 0; argv[i]; i++)
> > -		free(argv[i]);
> > -	free(argv);
> > -	return err;
> > +	for (ref = ref_map; ref; ref = ref->next) {
> > +		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <
> > 0 || +		    write_in_full(revlist.in, "\n", 1) < 0) {
> > +			error("failed write to rev-list");
> > +			err = errno;
>
> ... don't you get this error message with errno set to EPIPE?
> Previously, there was no error message.

Indeed, you are correct. I guess the following should be added to the
patch:

 	if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
 	    write_in_full(revlist.in, "\n", 1) < 0) {
-		error("failed write to rev-list");
-		err = errno;
+		if (errno != EPIPE) {
+			error("failed write to rev-list");
+			err = errno;
+		}
 		break;
 	}

Maybe I need to do something to the close() call as well? What happens on close() after EPIPE?


Thanks,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net