[PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Sylvestre Ledru]

[-- Attachment #1: Type: text/plain, Size: 45 bytes --]

Add missing fclose
CID 1407987, 1407986

S



[-- Attachment #2: 0005-2017-05-14-Sylvestre-Ledru-sylvestre-debian.org.patch --]
[-- Type: text/x-patch, Size: 772 bytes --]

From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
From: Sylvestre Ledru <sylvestre@debian.org>
Date: Sun, 14 May 2017 11:37:37 +0200
Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru  <sylvestre@debian.org>

	* lto-wrapper.c (copy_file): Fix resource leaks
          CID 1407987, 1407986
---
 gcc/lto-wrapper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 4b86f939ca2..832ffde3e40 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -838,6 +838,8 @@ copy_file (const char *dest, const char *src)
 	    fatal_error (input_location, "writing output file");
 	}
     }
+  fclose (d);
+  fclose (s);
 }
 
 /* Find the crtoffloadtable.o file in LIBRARY_PATH, make copy and pass name of
-- 
2.11.0

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Jeff Law]

On 05/14/2017 04:00 AM, Sylvestre Ledru wrote:
> Add missing fclose
> CID 1407987, 1407986
> 
> S
> 
> 
> 
> 0005-2017-05-14-Sylvestre-Ledru-sylvestre-debian.org.patch
> 
> 
>  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
> From: Sylvestre Ledru<sylvestre@debian.org>
> Date: Sun, 14 May 2017 11:37:37 +0200
> Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
> 
> 	* lto-wrapper.c (copy_file): Fix resource leaks
>            CID 1407987, 1407986
Doesn't this still leak in the cases were we call fatal_error?

Jeff

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Sylvestre Ledru]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

Le 15/05/2017 à 23:58, Jeff Law a écrit :
> On 05/14/2017 04:00 AM, Sylvestre Ledru wrote:
>> Add missing fclose
>> CID 1407987, 1407986
>>
>> S
>>
>>
>>
>> 0005-2017-05-14-Sylvestre-Ledru-sylvestre-debian.org.patch
>>
>>
>>  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
>> From: Sylvestre Ledru<sylvestre@debian.org>
>> Date: Sun, 14 May 2017 11:37:37 +0200
>> Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
>>
>>     * lto-wrapper.c (copy_file): Fix resource leaks
>>            CID 1407987, 1407986
> Doesn't this still leak in the cases were we call fatal_error? 

Indeed. thanks! Patch updated!

S


[-- Attachment #2: fclose.dif --]
[-- Type: video/dv, Size: 1444 bytes --]

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Sylvestre Ledru]

Le 16/05/2017 à 09:59, Sylvestre Ledru a écrit :
> Le 15/05/2017 à 23:58, Jeff Law a écrit :
>> On 05/14/2017 04:00 AM, Sylvestre Ledru wrote:
>>> Add missing fclose
>>> CID 1407987, 1407986
>>>
>>> S
>>>
>>>
>>>
>>> 0005-2017-05-14-Sylvestre-Ledru-sylvestre-debian.org.patch
>>>
>>>
>>>  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
>>> From: Sylvestre Ledru<sylvestre@debian.org>
>>> Date: Sun, 14 May 2017 11:37:37 +0200
>>> Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
>>>
>>>     * lto-wrapper.c (copy_file): Fix resource leaks
>>>            CID 1407987, 1407986
>> Doesn't this still leak in the cases were we call fatal_error? 
> Indeed. thanks! Patch updated!
>
ping ?
S

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Jakub Jelinek]

On Mon, May 15, 2017 at 03:58:29PM -0600, Jeff Law wrote:
> On 05/14/2017 04:00 AM, Sylvestre Ledru wrote:
> > Add missing fclose
> > CID 1407987, 1407986
> > 
> > S
> > 
> > 
> > 
> > 0005-2017-05-14-Sylvestre-Ledru-sylvestre-debian.org.patch
> > 
> > 
> >  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
> > From: Sylvestre Ledru<sylvestre@debian.org>
> > Date: Sun, 14 May 2017 11:37:37 +0200
> > Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
> > 
> > 	* lto-wrapper.c (copy_file): Fix resource leaks
> >            CID 1407987, 1407986
> Doesn't this still leak in the cases were we call fatal_error?

fatal_error is a noreturn function, why should we bother to do any cleanups
after it?  All that code is going to be optimized away anyway.

	Jakub

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Jeff Law]

On 06/26/2017 07:58 AM, Jakub Jelinek wrote:
> On Mon, May 15, 2017 at 03:58:29PM -0600, Jeff Law wrote:
>> On 05/14/2017 04:00 AM, Sylvestre Ledru wrote:
>>> Add missing fclose
>>> CID 1407987, 1407986
>>>
>>> S
>>>
>>>
>>>
>>> 0005-2017-05-14-Sylvestre-Ledru-sylvestre-debian.org.patch
>>>
>>>
>>>  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
>>> From: Sylvestre Ledru<sylvestre@debian.org>
>>> Date: Sun, 14 May 2017 11:37:37 +0200
>>> Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
>>>
>>> 	* lto-wrapper.c (copy_file): Fix resource leaks
>>>            CID 1407987, 1407986
>> Doesn't this still leak in the cases were we call fatal_error?
> 
> fatal_error is a noreturn function, why should we bother to do any cleanups
> after it?  All that code is going to be optimized away anyway.
But cleaning this kind of thing up does help static analyzers and such.
 ISTM that we'd need a compelling reason _not_ to accept this kind of patch.

jeff

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Jakub Jelinek]

On Mon, Jun 26, 2017 at 09:22:31AM -0600, Jeff Law wrote:
> >>>  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
> >>> From: Sylvestre Ledru<sylvestre@debian.org>
> >>> Date: Sun, 14 May 2017 11:37:37 +0200
> >>> Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
> >>>
> >>> 	* lto-wrapper.c (copy_file): Fix resource leaks
> >>>            CID 1407987, 1407986
> >> Doesn't this still leak in the cases were we call fatal_error?
> > 
> > fatal_error is a noreturn function, why should we bother to do any cleanups
> > after it?  All that code is going to be optimized away anyway.
> But cleaning this kind of thing up does help static analyzers and such.
>  ISTM that we'd need a compelling reason _not_ to accept this kind of patch.

Are the static analyzers so dumb to report something like that?

Unless we have a proof that they are, I think the original short patch is
the way to go, rather than the much more complicated later patch.

	Jakub

Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks

[Jeff Law]

On 06/26/2017 09:26 AM, Jakub Jelinek wrote:
> On Mon, Jun 26, 2017 at 09:22:31AM -0600, Jeff Law wrote:
>>>>>  From d255827a64012fb81937d6baa8534eabecf9b735 Mon Sep 17 00:00:00 2001
>>>>> From: Sylvestre Ledru<sylvestre@debian.org>
>>>>> Date: Sun, 14 May 2017 11:37:37 +0200
>>>>> Subject: [PATCH 5/5] 2017-05-14  Sylvestre Ledru<sylvestre@debian.org>
>>>>>
>>>>> 	* lto-wrapper.c (copy_file): Fix resource leaks
>>>>>            CID 1407987, 1407986
>>>> Doesn't this still leak in the cases were we call fatal_error?
>>>
>>> fatal_error is a noreturn function, why should we bother to do any cleanups
>>> after it?  All that code is going to be optimized away anyway.
>> But cleaning this kind of thing up does help static analyzers and such.
>>  ISTM that we'd need a compelling reason _not_ to accept this kind of patch.
> 
> Are the static analyzers so dumb to report something like that?
> 
> Unless we have a proof that they are, I think the original short patch is
> the way to go, rather than the much more complicated later patch.
The only thing that's complicated is how the formatting got mucked up in
the patch :(  It's literally just adding suitable fclose calls in the
proper paths.

I'm willing to go with the original in the hopes that it's enough to
silence Coverity, but if Coverity still complains we should probably
clean up the formatting and go with the latter patch.

Sylvestre, please commit your original patch with this ChangeLog entry:

	* lto-wrapper.c (copy_file) Close both file descriptors before
	exiting normally.
Jeff