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