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

[Ivan Gerasimov]

Thank you Christoph for the comments!


On 07.09.2016 12:14, Langer, Christoph wrote:
> Hi Ivan,
>
> I agree that the code around 1414 should be rewritten and I can also see the potential leak in loadKeysOrCertificateChains() which should definitely be fixed.
>
> However, I have further comments for your current change:
> In line 100 you should probably reduce the indent to 4 characters instead of 8 compared to the " ThrowExceptionWithMessage ..." from the line before to align with the rest of the file
>
> 99         ThrowExceptionWithMessage(env, OUT_OF_MEMORY_ERROR,
> 100                 "native memory allocation failed");
Thanks, I'll fix the indentation.

> Also, I'm still wondering if you could define a little macro like:
>
> #define CHECKED_NEW_ARRAY(_pointer, _type, _size) \
>          _pointer = new (env) _type[_size]; \
>          if (_pointer == NULL) { \
>              __leave; \
>          }
>
> Then you could have a compact line at the allocation place like:
> CHECKED_NEW_ARRAY(pszNameString, char, cchNameString)
If there's no strong objection, I would prefer to keep it the way it is now.
I think, it is important to see that the code contains the `jump` 
instruction without a need to unroll the macro.
And this is also consistent with the way the rest of the code is organized.

With kind regards,
Ivan

> But if you want to leave it like it is and don't use macros, I guess it's ok, too..
>
> Best regards
> Christoph
>
>> -----Original Message-----
>> From: security-dev [mailto:security-dev-bounces at openjdk.java.net] On Behalf
>> Of Ivan Gerasimov
>> Sent: Dienstag, 6. September 2016 22:07
>> To: Artem Smotrakov <artem.smotrakov at oracle.com>; security-
>> dev at openjdk.java.net
>> Subject: Re: [jdk9] (S) RFR: 8165463: Native implementation of sunmscapi
>> should use operator new (nothrow) for allocations
>>
>> 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
>>>>>>
>>>