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

[Ivan Gerasimov]

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