* [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
@ 2017-05-14 10:23 Sylvestre Ledru
2017-05-15 21:59 ` Jeff Law
0 siblings, 1 reply; 8+ messages in thread
From: Sylvestre Ledru @ 2017-05-14 10:23 UTC (permalink / raw)
To: gcc-patches
[-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-05-14 10:23 [PATCH] lto-wrapper.c (copy_file): Fix resource leaks Sylvestre Ledru
@ 2017-05-15 21:59 ` Jeff Law
2017-05-16 8:02 ` Sylvestre Ledru
2017-06-26 13:58 ` Jakub Jelinek
0 siblings, 2 replies; 8+ messages in thread
From: Jeff Law @ 2017-05-15 21:59 UTC (permalink / raw)
To: Sylvestre Ledru, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-05-15 21:59 ` Jeff Law
@ 2017-05-16 8:02 ` Sylvestre Ledru
2017-06-26 13:24 ` Sylvestre Ledru
2017-06-26 13:58 ` Jakub Jelinek
1 sibling, 1 reply; 8+ messages in thread
From: Sylvestre Ledru @ 2017-05-16 8:02 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-05-16 8:02 ` Sylvestre Ledru
@ 2017-06-26 13:24 ` Sylvestre Ledru
0 siblings, 0 replies; 8+ messages in thread
From: Sylvestre Ledru @ 2017-06-26 13:24 UTC (permalink / raw)
To: Jeff Law, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-05-15 21:59 ` Jeff Law
2017-05-16 8:02 ` Sylvestre Ledru
@ 2017-06-26 13:58 ` Jakub Jelinek
2017-06-26 15:22 ` Jeff Law
1 sibling, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2017-06-26 13:58 UTC (permalink / raw)
To: Jeff Law; +Cc: Sylvestre Ledru, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-06-26 13:58 ` Jakub Jelinek
@ 2017-06-26 15:22 ` Jeff Law
2017-06-26 15:26 ` Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Law @ 2017-06-26 15:22 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Sylvestre Ledru, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-06-26 15:22 ` Jeff Law
@ 2017-06-26 15:26 ` Jakub Jelinek
2017-06-29 23:30 ` Jeff Law
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2017-06-26 15:26 UTC (permalink / raw)
To: Jeff Law; +Cc: Sylvestre Ledru, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] lto-wrapper.c (copy_file): Fix resource leaks
2017-06-26 15:26 ` Jakub Jelinek
@ 2017-06-29 23:30 ` Jeff Law
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Law @ 2017-06-29 23:30 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Sylvestre Ledru, gcc-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-29 23:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-14 10:23 [PATCH] lto-wrapper.c (copy_file): Fix resource leaks Sylvestre Ledru
2017-05-15 21:59 ` Jeff Law
2017-05-16 8:02 ` Sylvestre Ledru
2017-06-26 13:24 ` Sylvestre Ledru
2017-06-26 13:58 ` Jakub Jelinek
2017-06-26 15:22 ` Jeff Law
2017-06-26 15:26 ` Jakub Jelinek
2017-06-29 23:30 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).