RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

[Mark Sheppard]

Hi Vyom,
     yes, I believe the consensus is to proceed with the changes.

regards
Mark

On 09/09/2015 16:23, Vyom Tewari wrote:
> Hi Mark,
>
> Is it OK to go ahead with the patch as it is, or you are expecting any 
> additional modification is required ?.
>
> Thanks,
> Vyom
>
>
> On 9/8/2015 6:35 PM, Mark Sheppard wrote:
>> that's true, the documentation is a bit nebulous on this issue.
>> but the inference is that the file descriptors are indeterminate state.
>> some older man pages allude to this
>>
>> as per solaris man pages, close will
>> " If close() is interrupted by a signal that is to be caught,
>> it  will return  -1  with errno set to EINTR and the state of fildes 
>> is unspecified"
>>
>> if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, 
>> F_DUP2FD, fd), and close is not restartable
>> then similar semantics could be inferred for dup2 in a EINTR scenario?
>> presume that subsequent call in the RESTARTABLE loop  will return 
>> another error.
>>
>>
>>
>>
>> On 08/09/2015 09:28, Chris Hegarty wrote:
>>> On 7 Sep 2015, at 17:41, Mark Sheppard <mark.sheppard at oracle.com> 
>>> wrote:
>>>
>>>> a couple of other considerations in the context of this issue perhaps?
>>>>
>>>> in this s is being duped onto fd, and part of the dup2 operation is 
>>>> the closing of fd, but
>>>> what's is the expected state of file descriptor fd in the event of 
>>>> a dup2 failure?
>>> I’m not sure that the documentation for dup2 gives us enough detail 
>>> here. The only situation I can see where fd would not be closed is 
>>> when EBADF is returned. Should not be an issue here.
>>>
>>>> s is closed in any case, but what about fd, should it be attended 
>>>> to if dup2 < 0
>>>> and closed ? is it still considered a valid fd?
>>>>
>>>> what can be said about the state of the encapsulating 
>>>> FileDescriptor associated with fd?
>>>> at this stage can it still be considered valid?
>>>> should valid() return false?
>>> Probably, but may not be worth bothering with unless there are 
>>> operations that can access it after this method throws.
>>>
>>> -Chris.
>>>
>>>> regards
>>>> Mark
>>>>
>>>> On 07/09/2015 14:29, Ivan Gerasimov wrote:
>>>>> Thanks!
>>>>>
>>>>> It looks good to me now.
>>>>>
>>>>> Sincerely yours,
>>>>> Ivan
>>>>>
>>>>> On 07.09.2015 16:08, Vyom Tewari wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Please find the latest diff, which contains the  latest fix.
>>>>>> http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/
>>>>>>
>>>>>> Thanks,
>>>>>> Vyom
>>>>>>
>>>>>>
>>>>>> On 9/7/2015 3:48 PM, Alan Bateman wrote:
>>>>>>> On 07/09/2015 10:26, Vyom Tewari wrote:
>>>>>>>> Hi everyone,
>>>>>>>> Can you please review my changes for below bug.
>>>>>>>>
>>>>>>>> Bug:
>>>>>>>>      JDK-8080402 : File Leak in 
>>>>>>>> jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/
>>>>>>>>
>>>>>>>> This change ensure that if close() fails we throw correct io 
>>>>>>>> exception and there is no file leak.
>>>>>>> close isn't restartable so I think we need to look at that while 
>>>>>>> we are here.
>>>>>>>
>>>>>>> -Alan.
>>>>>>
>>
>