RFR [XS]: 8234968: check calloc rv in libinstrument InvocationAdapter

[Thomas Stüfe]

Just read Matthias reply:

We call jplis_assert() if allocation fails. Looking at

src/java.instrument/share/native/libinstrument/JPLISAssert.h

I see that these assertions seem to be turned on all the time:

 45 #define JPLISASSERT_ENABLEASSERTIONS    (1)

and lands us in JPLISAssertCondition() (possible improvement here is to
evaluate the condition before the call):

 58 #define jplis_assert(x)             JPLISAssertCondition((jboolean)(x),
#x, THIS_FILE, __LINE__)

However, JPLISAssertCondition() is not an assert - name is misleading - but
just a printf():

 39 void
 40 JPLISAssertCondition(   jboolean        condition,
 41                         const char *    assertionText,
 42                         const char *    file,
 43                         int             line) {
 44     if ( !condition ) {
 45         fprintf(stderr, "*** java.lang.instrument ASSERTION FAILED ***:
\"%s\" at %s line: %d\n",
 46                                             assertionText,
 47                                             file,
 48                                             line);
 49     }
 50 }

Maybe I miss something but I do not see an abort.

----

I think we should add an exit(2) or abort(2) to the assertion.

But I also think this is a different issue from what Matthias tries to fix,
so I am fine with Matthias change.

Cheers, Thomas





On Fri, Nov 29, 2019 at 8:20 AM Alan Bateman <Alan.Bateman at oracle.com>
wrote:

> On 29/11/2019 06:29, Thomas Stüfe wrote:
> > Hi Matthias,
> >
> > I am not certain the callers are prepared to handle NULL.
> >
> > This is used in a chain of TRANSFORM macro calls which AFAICS do not
> > handle NULL; e.g. , at 872, we pass the returned pointer to
> > convertUft8ToPlatformString which passes it on (on Windows) to
> > MultiByteToWideChar, which does not handle NULL input.
> >
> > So I wonder whether a clear error message with an exit would be better
> > in this case. Otherwise we may get a crash just some instructions later.
> >
> Right, this needs a lot more analysis to see if it's even possible to
> continue. The main usage is VM startup where the -javaagent option
> specifies agents that have the Boot-Class-Path attribute. In that case
> it would not be unreasonable to abort the process, it's unlikely to get
> much startup in the startup if memory is exhausted. The other possible
> context is where a tool agent is loaded into a running VM, in which case
> have the attach thread return with a pending exception might be okay
> (although the VM is likely to shutdown anyway as the memory exhaustion
> will be detected/handled elsewhere).
>
> -Alan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191129/032da3f6/attachment-0001.html>