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