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

[Thomas Stüfe]

On Fri, Nov 29, 2019 at 8:59 AM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

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

Yes :-)

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

I think that makes sense, or one could even make jplis_assert abort() the
program.

But I leave this up to you, could also be done in a different issue. The
patch you posted looks fine to me as it is.

Cheer,s Thomas

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>
> 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/bca9db7a/attachment-0001.html>