Re: found a resource leak in file builtin-fast-export.c
- From
- Matthias Andree <matthias.andree@gmx.de>
- Date
- Jul 9, 2009, 13:36 UTC
- Message-ID
- <op.uwsyqnwt1e62zd@balu.cs.uni-paderborn.de>
- In-Reply-To
- <alpine.DEB.1.00.0907091500420.4339@intel-tinevez-2-302>
Am 09.07.2009, 15:01 Uhr, schrieb Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 36 quoted lines
> Hi,
>
> On Thu, 9 Jul 2009, Thomas Rast wrote:
>
>> Johannes Schindelin wrote:
>> > On Thu, 9 Jul 2009, Thomas Rast wrote:
>> >
>> > > Martin Ettl wrote:
>> > > > - if (ferror(f) || fclose(f))
>> > > > + if (ferror(f))
>> > > > error("Unable to write marks file %s.", file);
>> > > > + fclose(f);
>> > >
>> > > You no longer check the error returned by fclose(). This is
>> > > important, because the FILE* API may buffer writes, and a write
>> error
>> > > may only become apparent when fclose() flushes the file.
>> >
>> > Indeed. A better fix would be to replace the || by a |, but this
>> must be
>> > accompanied by a comment so it does not get removed due to overzealous
>> > compiler warnings.
>>
>> Are you allowed to do that? IIRC using | no longer guarantees that
>> ferror() is called before fclose(), and my local 'man 3p fclose' says
>> that
>>
>> After the call to fclose(), any use of stream results in
>> undefined behavior.
>
> Good point. So we really need something like
>
> err = ferror(f);
> err |= fclose(f); /* call fclose() even if there was an error */
> if (err)
> error...I've made such a patch, to appear soon on the list (sorry for not Cc:'ing it).
-- Matthias Andree