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

[Artem Smotrakov]

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
>>>>>
>>>
>>
>>
>