RFR: JDK-8145099 Better error message when SA can't attach to a process

[Staffan Larsen]

> On 10 dec. 2015, at 23:20, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
> 
> Staffan,
> 
> 1.
> 
> strerror_r comes in two versions - returning string (GNU) and returning
> int (XSI)
> 
> To make developers live more interesting GNU version doesn't guarantee
> to fill buf with appropriate string, so you can't just void return value.
> 
> Default version tends to be XSI now days and I'm not sure that build
> system add -D_GNU_SOURCE on all platforms.
> 
> So you have to add something like
> 
> #if _POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE
> use XSI strerror_r
> #else
> use GNU strerror_r
> #endif

We are getting the _GNU_SOURCE version when building. If that changes in the makefiles somewhere so that we get the XSI version, then this code should not compile - it should issue a warning when assigning an int to char*.

> 
> Or just don't care of threads and use plain strerror - it widely used in
> hotspot and it doesn't crash on a race.
> 
> 2.
> 
> err_buff remains empty if the attach fails at ll.412 of ps_proc.c,
> but see below.

It should get filled in in the ptrace_attach() call.

> 
> 3. (* just an opinion *)
> 
> Extra parameters to ptrace_attach/PGrab linux makes in different from
> other platforms.

This is all platform specific code - and all the platforms are different. Of course, it would be good if all platforms reported good error message always - and we can continue working on that.

Thanks,
/Staffan

> 
> So I would prefer to have it unchanged - you can either mimic Solaris
> behavior and add Pgrab_error() function or just move all strerror staff
> to LinuxDebuggerLocal_attach0 - errno from ptrace_attach/Pgrab is
> available there.
> 
> (except calloc case, but if we can't allocate couple of bytes for struct
> ps_prochandle we most likely will not see the error anyway).
> 
> 
> -Dmitry
> 
> 
> On 2015-12-10 18:10, Staffan Larsen wrote:
>> Thanks!
>> 
>>> On 10 dec. 2015, at 15:56, Thomas Stüfe <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>
>>> <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>> wrote:
>>> 
>>> Hi Staffan,
>>> 
>>> this looks fine now, thanks!
>>> 
>>> ..Thomas
>>> 
>>> On Thu, Dec 10, 2015 at 3:42 PM, Staffan Larsen
>>> <staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com> <mailto:staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>>> wrote:
>>> 
>>>    Hi Thomas, 
>>> 
>>>>    On 10 dec. 2015, at 15:27, Thomas Stüfe <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>
>>>>    <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>> wrote:
>>>> 
>>>>    Hi Staffan,
>>>> 
>>>>    thanks!
>>>> 
>>>>    It also just occurred to me that strerror is not considered
>>>>    threadsafe. Maybe strerror_r() would a better option (depending
>>>>    on the version you use you would have to check the return value
>>>>    for NULL, though).
>>> 
>>>    Updated here: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.03/ <http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.03/>
>>> 
>>>    Thanks,
>>>    /Staffan
>>> 
>>>> 
>>>>    sorry for this piecemeal review.
>>>> 
>>>>    Kind Regards, Thomas
>>>> 
>>>> 
>>>> 
>>>>    On Thu, Dec 10, 2015 at 3:22 PM, Staffan Larsen
>>>>    <staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com> <mailto:staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>>> wrote:
>>>> 
>>>>        Hi Thommas,
>>>> 
>>>>>        On 10 dec. 2015, at 14:49, Thomas Stüfe
>>>>>        <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com> <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>>
>>>>>        wrote:
>>>>> 
>>>>>        Hi Stefan,
>>>>> 
>>>>>        Disclaimer: not a "R"eviewer.
>>>>> 
>>>>>        http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/agent/src/os/linux/LinuxDebuggerLocal.c.udiff.html <http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/agent/src/os/linux/LinuxDebuggerLocal.c.udiff.html>
>>>>> 
>>>>>        I am not sure why you pass sizeof(err_buf) - 1 instead of
>>>>>        sizeof(err_buf) to Pgrab and sizeof(msg) - 1 instead of
>>>>>        sizeof(msg) to snprintf? snprintf will always zero terminate
>>>>>        in case of truncation, at least on posix platforms.
>>>> 
>>>>        Good point. I was just being overly cautious without thinking.
>>>> 
>>>>        Updated
>>>>        webrev: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.02/ <http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.02/>
>>>> 
>>>>        Thanks,
>>>>        /Staffan
>>>> 
>>>>> 
>>>>>        Otherwise this looks good.
>>>>> 
>>>>>        Kind Regards, Thomas
>>>>> 
>>>>> 
>>>>>        On Thu, Dec 10, 2015 at 2:19 PM, Staffan Larsen
>>>>>        <staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>
>>>>>        <mailto:staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>>> wrote:
>>>>> 
>>>>>            Please review this patch to add a better error message
>>>>>            to SA when it fails to attach to a process on linux.
>>>>>            Currently the error says "Can't attach to the process”.
>>>>>            After this change the message will look something like:
>>>>>            "Can't attach to the process: ptrace(PTRACE_ATTACH, ..)
>>>>>            failed for 28417: Operation not permitted”
>>>>> 
>>>>>            webrev: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/
>>>>>            bug: https://bugs.openjdk.java.net/browse/JDK-8145099
>>>>> 
>>>>>            Thanks,
>>>>>            /Staffan
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> 
>> 
> 
> 
> -- 
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20151211/afab9fd4/attachment-0001.html>