[jdk9] (S) RFR: 8165463: Native implementation of sunmscapi should use operator new (nothrow) for allocations

[Ivan Gerasimov]

Ah, yes, right.

I forgot to close the previous webrev in the browser.

With kind regards,
Ivan

On 06.09.2016 23:20, Artem Smotrakov wrote:
> I think we are talking about the same "continue", but in different 
> webrevs :) I meant this:
>
> http://cr.openjdk.java.net/~igerasim/8165463/01/webrev/src/jdk.crypto.mscapi/windows/native/libsunmscapi/security.cpp.html 
>
>
> ...
>
> 1410             if (::CertGetNameString(pCertContext,
> 1411                 CERT_NAME_FRIENDLY_DISPLAY_TYPE, 0, NULL, 
> pszNameString,
> 1412                 cchNameString) == 1) {
> 1413
> 1414                 continue; // not found
> 1415             }
>
> ...
>
> Thanks!
>
>
> Artem
>
> On 09/06/2016 01:07 PM, Ivan Gerasimov wrote:
>> Hi Artem!
>>
>> On 06.09.2016 20:45, Artem Smotrakov wrote:
>>> Hi Ivan,
>>>
>>> It's not really about your changes, but I am wondering if "delete [] 
>>> pszNameString" should be called before "continue" in line 1414? 
>>> Looks like a potential memory leak.
>>>
>> I assume you mean "continue" at the line 1430, not 1414?
>> Yes, that caught my eye too :)
>> Actually, this particular case seems to be benign.
>> If the first call to CertGetNameString() at 1413 returned > 1 then 
>> _most_likely_ the second call at 1426 should be Okay as well.
>> But I agree that this should be rewritten to make the deallocation 
>> logic more clear.
>>
>> There seems to be another, more promising memleak in 
>> loadKeysOrCertificateChains(), which I'm investigating at the moment.
>> I think these two leaks can be fixed together.
>>
>> With kind regards,
>> Ivan
>>
>>> Artem
>>>
>>>
>>> On 09/06/2016 02:54 AM, Ivan Gerasimov wrote:
>>>> Thank you Christoph for looking into this!
>>>>
>>>>
>>>> On 05.09.2016 23:52, Langer, Christoph wrote:
>>>>> Hi Ivan,
>>>>>
>>>>> this looks like a good idea.
>>>>>
>>>>> Maybe the pattern to do new (std::nothrow), then check for 0 and 
>>>>> throw OOM is a good candidate for a Macro which would keep the 
>>>>> code a bit more compact?
>>>>
>>>> Yes, I agree that this code needs some refactoring.
>>>> Let's use the overloaded 'operator new':
>>>>
>>>> http://cr.openjdk.java.net/~igerasim/8165463/01/webrev/
>>>>
>>>> With kind regards,
>>>> Ivan
>>>>
>>>>> Best regards
>>>>> Christoph
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: security-dev [mailto:security-dev-bounces at openjdk.java.net] 
>>>>>> On Behalf
>>>>>> Of Ivan Gerasimov
>>>>>> Sent: Montag, 5. September 2016 21:53
>>>>>> To: security-dev at openjdk.java.net
>>>>>> Subject: [jdk9] (S) RFR: 8165463: Native implementation of 
>>>>>> sunmscapi should
>>>>>> use operator new (nothrow) for allocations
>>>>>>
>>>>>> Hello!
>>>>>>
>>>>>> In the native layer of sunmscapi provider, for memory allocations 
>>>>>> the
>>>>>> ::operator new() is used.
>>>>>> In (a very unlikely) case of failure, it will raise a C++ 
>>>>>> exception of
>>>>>> type bad_alloc, which is bad, as we don't have handling code.
>>>>>>
>>>>>> One simple way to improve the situation would be to use 
>>>>>> ::operator new
>>>>>> (std::nothrow), which will just return zero to indicate a failure
>>>>>> instead of throwing an exception.
>>>>>> Then we can (try to) throw a Java exception of type 
>>>>>> OutOfMemoryError.
>>>>>>
>>>>>> Would you please help review the fix?
>>>>>>
>>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8165463
>>>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8165463/00/webrev/
>>>>>>
>>>>>> Comments/suggestions are very welcome.
>>>>>>
>>>>>> With kind regards,
>>>>>> Ivan
>>>>>>
>>>>
>>>
>>>
>>
>
>