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");
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)
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-boun...@openjdk.java.net] On Behalf
> Of Ivan Gerasimov
> Sent: Dienstag, 6. September 2016 22:07
> To: Artem Smotrakov <artem.smotra...@oracle.com>; security-
> d...@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-boun...@openjdk.java.net]
> >>>> On Behalf
> >>>> Of Ivan Gerasimov
> >>>> Sent: Montag, 5. September 2016 21:53
> >>>> To: security-dev@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
> >>>>
> >>
> >
> >
