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

[Baesken, Matthias]

Hi Thomas,  probably jplis_assert(x)        should better be named  jplis_warn(x)     .


Additionally the TRANSFORM macro  could be enhanced  by an abort()  call   (or abortJVM<http://ld8443:8080/source/s?defs=abortJVM&project=openjdk-jdk>
?)   or something similar .

Best regards, Matthias





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<mailto: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/19d73ed6/attachment.html>