From: Alex Riesen <raa.lkml@gmail.com>
Date: Thu, 09 Jul 2009 08:43:38 GMT
Subject: Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line
Message-ID: <81b0412b0907090143r7c894c6fh15db309b53b9c446@mail.gmail.com>
In-Reply-To: <200907091037.41329.johan@herland.net>

On Thu, Jul 9, 2009 at 10:37, Johan Herland<johan@herland.net> wrote:
> On Thursday 09 July 2009, Alex Riesen wrote:
>> On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:
>> > On Wednesday 08 July 2009, Johannes Sixt wrote:
>> >> ... 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;
>>
>> You'll loose errno this way: error() does not save it.
>
> Not sure what you mean here. Should I move "err = errno;" outside the
> innermost "if"?

put it before error("failed write to rev-list"); or even before the
"if (err != EPIPE)".
Otherwise it is 0 after fprintf to stderr (which is the error() call).

> From my POV, if errno != EPIPE, we save it into err, and return that
> (overridden by finish_command()'s return value, if non-zero). If errno ==
> EPIPE, we're not interested in saving it, because we expect finish_command()
> to return non-zero in any case.

And you think this expectation makes the code simpler to understand?